Bug 65112 - glcpp hangs parsing line continuations
glcpp hangs parsing line continuations
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: glsl-compiler
git
x86-64 (AMD64) All
: medium normal
Assigned To: Carl Worth
: have-backtrace, regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-29 05:52 UTC by Vinson Lee
Modified: 2013-06-30 22:27 UTC (History)
1 user (show)

See Also:


Attachments
Proposed fix (1.18 KB, patch)
2013-06-03 19:53 UTC, Carl Worth
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Vinson Lee 2013-05-29 05:52:54 UTC
mesa: 4dea6cf21557bcd3bbab3402e19e19f665d7e177 (master)

Some forms of line continuations in GLSL shaders, even in comments, can cause the GLSL compiler to hang.

For example, this test shader causes a hang in remove_line_continuations.

<comment.vert>
// \
// a
// \n
void main()
{
  gl_Position = gl_Vertex;
}
</comment.vert>

$ ./glcpp comment.vert 

(gdb) bt
#0  0x00007fff8ce616db in strlen ()
#1  0x000000010f9908c3 in cat (dest=0x7fff50268d78, str=0x10f9923e8 "\n", n=1) at ralloc.c:348
#2  0x000000010f990a12 in ralloc_strcat (dest=0x7fff50268d78, str=0x10f9923e8 "\n") at ralloc.c:364
#3  0x000000010f98f94e in remove_line_continuations (ctx=0x7fccd9c03c00, shader=0x7fccda00323a "// \\n\n\nvoid main()\n{\n  gl_Position = gl_Vertex;\n}\n") at pp.c:101
#4  0x000000010f98fabc in glcpp_preprocess (ralloc_ctx=0x7fccd9c03b80, shader=0x7fff5027f518, info_log=0x7fff5027f520, extensions=0x0, gl_ctx=0x7fff50268e40) at pp.c:143
#5  0x000000010f981993 in main (argc=2, argv=0x7fff5027f570) at glcpp.c:167


This works.
$ ./glcpp --disable-line-continuations comment.vert
Comment 1 Vinson Lee 2013-05-31 07:28:14 UTC
2483039aca78078f21a57e8e950a0bc0181fbd3d is the first bad commit
commit 2483039aca78078f21a57e8e950a0bc0181fbd3d
Author: Carl Worth <cworth@cworth.org>
Date:   Thu Nov 29 14:49:46 2012 -0800

    glcpp: Rewrite line-continuation support to act globally.
    
    Previously, we were only supporting line-continuation backslash characters
    within lines of pre-processor directives, (as per the specification). With
    OpenGL 4.2 and GLES3, line continuations are now supported anywhere within a
    shader.
    
    While changing this, also fix a bug where the preprocessor was ignoring
    line continuation characters when a line ended in multiple backslash
    characters.
    
    The new code is also more efficient than the old. Previously, we would
    perform a ralloc copy at each newline. We now perform copies only at each
    occurrence of a line-continuation.
    
    This commit fixes the line-continuation.vert test in piglit.
    
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

:040000 040000 35d086bb98380598fee42c2d45c4f43e8f4ffba4 3abdae53c4494f0d170a2a11b9a65a7f6f17d159 M	src
bisect run success
Comment 2 Carl Worth 2013-05-31 17:20:02 UTC
Thanks for the bug report.

It's surprising how many bugs have been in the implementation of such a seemingly-simple feature.

I'll get right on working on a fix for this one.

-Carl
Comment 3 Carl Worth 2013-06-03 19:53:03 UTC
Created attachment 80246 [details] [review]
Proposed fix

This patch should fix the bug.

I've just submitted this to the mesa list.

Thanks again for the bug report, and the nice small test case.

-Car
Comment 4 Vinson Lee 2013-06-30 22:27:17 UTC
Fixed by d8eeb1d330c52aad734cc553e41b457e6727275c.

commit d8eeb1d330c52aad734cc553e41b457e6727275c
Author: Carl Worth <cworth@cworth.org>
Date:   Mon Jun 3 11:35:43 2013 -0700

    glcpp: Fix post-decrement underflow in loop-control variable
    
    This loop-control condition with a post-decrement operator would lead to
    an underflow of collapsed_newlines. This in turn would cause a subsequent
    execution of the loop to labor inordinately trying to return the loop-control
    variable to a value of 0 again.
    
    Fix this by dis-intertwining the test and the decrement.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65112
    
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>