Bug 72686 - GLSL preprocessor does not eat comments correctly
Summary: GLSL preprocessor does not eat comments correctly
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Carl Worth
QA Contact: Intel 3D Bugs Mailing List
URL: http://patchwork.freedesktop.org/patc...
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-13 18:18 UTC by Ian Romanick
Modified: 2014-01-31 18:05 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Ian Romanick 2013-12-13 18:18:17 UTC
The GLSL spec says "Comments are each replaced by one space character," as do the C and C++ specifications.  This means that C code such as

#define X a/*
*/b

X

will produce

# 1 "/tmp/foo.c"
# 1 "<command-line>"
# 1 "/tmp/foo.c"



a b

It also means that GLSL code such as

#version 120
#define X gl_Position/*
*/ =

void main()
{
   X vec4(0.);
}

should compile as if it were

#version 120
void main()
{
   gl_Position = vec4(0.);
}

On Mesa, this generates an error:

0:3(3): error: syntax error, unexpected '='
Comment 1 Carl Worth 2013-12-16 23:21:51 UTC
Thanks for the bug report, Ian.

It looks like the glcpp code originally performed as desired, but
broke as of commit bd55ba568b301d0f764cd1ca015e84e1ae932c8b

I think what we want to do here is to fix the lexer so that it no
longer emits the NEWLINE tokens, but instead counts these, and then
when it's emitting a NEWLINE token for a newline read outside of a
comment it can emit extra NEWLINE tokens (or a #line directive) to
preserve the correct line number.

-Carl
Comment 2 Kenneth Graunke 2013-12-16 23:57:26 UTC
I agree with this plan.
Comment 3 Carl Worth 2013-12-20 00:34:37 UTC
I've sent a patch to the mesa-dev mailing this that adds a test case based
on Ian's example, and also fixes the bug.

-Carl
Comment 4 Petri Latvala 2014-01-20 12:47:13 UTC
commit 6005e9cb283214cd57038c7c5e7758ba72ec6ac2
Author: Carl Worth <cworth@cworth.org>
Date:   Thu Dec 19 16:06:31 2013 -0800

    glcpp: Replace multi-line comment with a space (even as part of macro definition)


The above commit, which I believe is the fix you mentioned, causes this code to produce a preprocessor error:

/*
*/  // a comment
void main()
{
  gl_FragColor=vec4(0.0);
}

0:2(60): preprocessor error: syntax error, unexpected $end

This happens only if the /* */ contains newlines, and only if a single-line comment follows the multi-line comment. Two multi-line comments works fine.
Comment 5 Carl Worth 2014-01-29 22:30:32 UTC
(In reply to comment #4)
 
> The above commit, which I believe is the fix you mentioned, causes this code
> to produce a preprocessor error:
> 
> /*
> */  // a comment
...
> 0:2(60): preprocessor error: syntax error, unexpected $end
> 
> This happens only if the /* */ contains newlines, and only if a single-line
> comment follows the multi-line comment. Two multi-line comments works fine.

Yes, Petri, that's the commit I made to fix the originally-reported bug.

And thanks for finding this regression in my commit!

It's an extremely narrow problem, and you described it precisely. It can only
with a multi-line comment, (spanning more than one line), followed immediately
by a single-line comment without any intervening newline.

I've just sent a patch to the list to fix the bug, and also a patch that
adds a test case based on your description of the problem.

Thanks again! This was a ''fun'' problem to solve. (I was stumped for quite
a while. Then once I had a working fix in hand, went through three different
fixes as I realized that the descriptions I was writing for the fix didn't
match the precise symptoms of the bug. Each time, I had to look closer to
find that I was fixing some non-problem that only accidentally resolved the
real bug.)

It's nice to see in the final version that the fix, its description, and the
test case all match each other quite precisely.

-Carl


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.