[Mesa-dev,1/2] glcpp: Allow integer expression for #line directive.

Submitted by Carl Worth on Jan. 29, 2014, 1:56 a.m.

Details

Message ID 1390960563-15334-1-git-send-email-cworth@cworth.org
State New
Headers show

Not browsing as part of any series.

Commit Message

Carl Worth Jan. 29, 2014, 1:56 a.m.
The GLSL specification explicitly allows for an in integer expression here,
not just a literal integer. The specification says:

	#line must have, after macro substitution, one of the following forms:

		#line line
		#line line source-string-number

	where line and source-string-number are constant integer expressions.

Previously, the implementation required a literal integer (after substitution)
so things like "#line (25)" or "#line +15" were allowed by the specification
but rejected by the implementation.

With this change to the grammar, #line will accept integer expressions like
these.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72273
---
 src/glsl/glcpp/glcpp-parse.y | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
index 17bc649..2d9008a 100644
--- a/src/glsl/glcpp/glcpp-parse.y
+++ b/src/glsl/glcpp/glcpp-parse.y
@@ -221,7 +221,7 @@  expanded_line:
 |	ELIF_EXPANDED expression NEWLINE {
 		_glcpp_parser_skip_stack_change_if (parser, & @1, "elif", $2);
 	}
-|	LINE_EXPANDED integer_constant NEWLINE {
+|	LINE_EXPANDED expression NEWLINE {
 		parser->has_new_line_number = 1;
 		parser->new_line_number = $2;
 		ralloc_asprintf_rewrite_tail (&parser->output,
@@ -229,7 +229,7 @@  expanded_line:
 					      "#line %" PRIiMAX "\n",
 					      $2);
 	}
-|	LINE_EXPANDED integer_constant integer_constant NEWLINE {
+|	LINE_EXPANDED expression integer_constant NEWLINE {
 		parser->has_new_line_number = 1;
 		parser->new_line_number = $2;
 		parser->has_new_source_number = 1;

Comments

On Tue, Jan 28, 2014 at 5:56 PM, Carl Worth <cworth@cworth.org> wrote:
> The GLSL specification explicitly allows for an in integer expression here,
> not just a literal integer. The specification says:
>
>         #line must have, after macro substitution, one of the following forms:
>
>                 #line line
>                 #line line source-string-number
>
>         where line and source-string-number are constant integer expressions.

If this is the case, then

>
> Previously, the implementation required a literal integer (after substitution)
> so things like "#line (25)" or "#line +15" were allowed by the specification
> but rejected by the implementation.
>
> With this change to the grammar, #line will accept integer expressions like
> these.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72273
> ---
>  src/glsl/glcpp/glcpp-parse.y | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
> index 17bc649..2d9008a 100644
> --- a/src/glsl/glcpp/glcpp-parse.y
> +++ b/src/glsl/glcpp/glcpp-parse.y
> @@ -221,7 +221,7 @@ expanded_line:
>  |      ELIF_EXPANDED expression NEWLINE {
>                 _glcpp_parser_skip_stack_change_if (parser, & @1, "elif", $2);
>         }
> -|      LINE_EXPANDED integer_constant NEWLINE {
> +|      LINE_EXPANDED expression NEWLINE {
>                 parser->has_new_line_number = 1;
>                 parser->new_line_number = $2;
>                 ralloc_asprintf_rewrite_tail (&parser->output,
> @@ -229,7 +229,7 @@ expanded_line:
>                                               "#line %" PRIiMAX "\n",
>                                               $2);
>         }
> -|      LINE_EXPANDED integer_constant integer_constant NEWLINE {
> +|      LINE_EXPANDED expression integer_constant NEWLINE {

shouldn't this should be

LINE_EXPANDED expression expression NEWLINE
Matt Turner <mattst88@gmail.com> writes:
>> -|      LINE_EXPANDED integer_constant integer_constant NEWLINE {
>> +|      LINE_EXPANDED expression integer_constant NEWLINE {
>
> shouldn't this should be
>
> LINE_EXPANDED expression expression NEWLINE

Yes. Thanks for the catch. I'll expand the testing and follow up with a
new set of patches.

-Carl
Carl Worth <cworth@cworth.org> writes:
> Matt Turner <mattst88@gmail.com> writes:
>>
>> LINE_EXPANDED expression expression NEWLINE
>
> Yes. Thanks for the catch. I'll expand the testing and follow up with a
> new set of patches.

Except that that's totally bogus. We can't parse two adjacent
expressions with no intervening separator (assuming whitespace is valid
within each <expression>).

This looks to me like a bug in the GLSL specification. Real
preprocessors for languages like C do not accept expressions for #line,
(they do perform macro substitution, but the final result must be an
integer constant, not an expression). And that's what is in the C
specifications.

A deviation to accept an expression here, (together with the replacement
of a source-file string, with a source-file number), gives us something
unparseable, (or at least under-specified).

It feels like something specified without an implementation in hand,
(which can lead to problems like this).

Ian, I asked in the bug[*] already, but I'll ask again here. Where did
this bug report come from? And can we fix the problem by pushing to have
the specification updated?

If not, (that is, if there are real-world users with non-integer-literal
#line directives), then it would be nice to see what those actually are
so we could, at the very least, resolve the grammar ambiguity in a
direction that will solve the real-world cases.

[*] https://bugs.freedesktop.org/show_bug.cgi?id=72273
On 01/29/2014 02:15 PM, Carl Worth wrote:
> Carl Worth <cworth@cworth.org> writes:
>> Matt Turner <mattst88@gmail.com> writes:
>>>
>>> LINE_EXPANDED expression expression NEWLINE
>>
>> Yes. Thanks for the catch. I'll expand the testing and follow up with a
>> new set of patches.
> 
> Except that that's totally bogus. We can't parse two adjacent
> expressions with no intervening separator (assuming whitespace is valid
> within each <expression>).

Agreed.  In many cases, it's impossible to know where the first ends and
the second starts.  I suppose you could require both be wrapped in
parenthesis, but...this seems to be a feature of dubious value at best.

> This looks to me like a bug in the GLSL specification. Real
> preprocessors for languages like C do not accept expressions for #line,
> (they do perform macro substitution, but the final result must be an
> integer constant, not an expression). And that's what is in the C
> specifications.

If the C preprocessor disallows this, I am strongly in favor of not
deviating from that.

If GLSL wants to support extra non-trivial preprocessor features, the
spec authors are going to have to start specifying a lot of behavior,
rather than saying "like the C++ preprocessor."  As we discovered when
writing glcpp, there are a /lot/ of fiddly cases in preprocessing; given
the GLSL specification's usual level of rigor, I'm afraid details might
be missed somehow and the two languages would start diverging...and
that's not good for anybody.

Okay, so I've taken this a bit far, but...if cpp doesn't do it, I really
don't think we should.

> A deviation to accept an expression here, (together with the replacement
> of a source-file string, with a source-file number), gives us something
> unparseable, (or at least under-specified).
> 
> It feels like something specified without an implementation in hand,
> (which can lead to problems like this).
> 
> Ian, I asked in the bug[*] already, but I'll ask again here. Where did
> this bug report come from? And can we fix the problem by pushing to have
> the specification updated?
> 
> If not, (that is, if there are real-world users with non-integer-literal
> #line directives), then it would be nice to see what those actually are
> so we could, at the very least, resolve the grammar ambiguity in a
> direction that will solve the real-world cases.
> 
> [*] https://bugs.freedesktop.org/show_bug.cgi?id=72273