[Mesa-dev] Bogus use of '#' in a macro definition (was: Re: Mesa (master): 29 new commits)

[Carl Worth]

Ian Romanick <idr at freedesktop.org> writes:
> On 07/31/2014 12:14 AM, Michel Dänzer wrote:
>> 
>> FYI, this change broke the game Reaction Quake, see the failure output
>> below. I don't know if this is a bug in this change or in the game, so
>> I'm reporting it here.

Hi Michel,

Thanks very much for reporting this failure. It's certainly an
interesting one.

>> #define USE_DISCARD#line 0
>
> Something is seriously wrong here.

The above line in the shader does seem to be what is triggering the
error you noticed:

>> 0:46(20): preprocessor error: syntax error, unexpected HASH_TOKEN, expecting NEWLINE

Before the referenced commit, "#define foo#bar" would be accepted, (for
some definition of "accepted"[*]) while after the commit, the same input
causes the above error.

It's fairly obvious that that line is not what was intended by the
author of the shader, so a bug should be reported to the game
authors. Michel, can you follow up with that?

Meanwhile, I've looked at the GLSL specification and it's quite clear
that no '#' character is allowed except for introducing a directive, (so
at the beginning of a line, ignoring horizontal whitespace and
comments), or as part of a token-pasting operator in the replacement
list of a macro.

So I do think it's legitimate to generate a syntax error here.

But I also thought it would be worthwhile to look at what other
implementations do in a case like this. For example, given:

	#define foo#bar

gcc's preprocessor emits a diagnostic:

	warning: missing whitespace after the macro name [enabled by default]

and then goes on to behave as if what had been presented was:

	#define foo #bar

(where "#bar" is literal text, and never interpreted as a directive).

For GLSL, we could do that, and if the foo macro is ever actually used,
the appearance of '#' should then generate a syntax error in the
compiler.

This is similar to "#define foo;bar" where gcc also generates the
warning and then defines "foo" to ";bar". For that case, glcpp is
already compatible with gcc, (though not generating the warning).

I also checked that Nvidia's implementation does accept the following
line without an error:

        #define USE_DISCARD#line 0

It appears to be defining a macro USE_DISCARD to a value of "0",
(ignoring the "#line" portion). This isn't compatible with gcc, but
again, we're entirely outside the bounds of any specification with this
input.

I've experimented, and I've come up with a fairly simple patch that
would treat '#' in the same way that ';' is currently treated in a case
like this. That is:

     #define USE_DISCARD#line 0

will define the USE_DISCARD macro to expand to "#line 0", (but not
treating #line as a directive).

Again, like I said above, I don't think that's strictly necessary
here. I believe the syntax error is legitimate, (and perhaps even
helpful---it pointed us to this bug in Reaction Quake's shader, for
example). But it is a possibility.

I'll follow up with that patch.

-Carl

[*] For reference, here's what previous versions of mesa would do with a
line like:

	#define USE_DISCARD#line 0

1. Print a '#' character to stdout while parsing this shader

2. Define the USE_DISCARD macro to the text "line 0"

-- 
carl.d.worth at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140731/248b3b31/attachment-0001.sig>