Bug 94447 - glsl/glcpp/tests/glcpp-test-cr-lf regression
Summary: glsl/glcpp/tests/glcpp-test-cr-lf regression
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: x86-64 (AMD64) All
: medium normal
Assignee: Kenneth Graunke
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords: bisected, regression
Depends on:
Blocks:
 
Reported: 2016-03-08 23:26 UTC by Vinson Lee
Modified: 2016-03-10 19:28 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Vinson Lee 2016-03-08 23:26:08 UTC
mesa: a100a57e30010da49c96f84a661cec9c57f9eebe (master 11.3.0-devel)

$ make check
[...]
PASS: glsl/glcpp/tests/glcpp-test
FAIL: glsl/glcpp/tests/glcpp-test-cr-lf
PASS: glsl/tests/blob-test
PASS: glsl/tests/general-ir-test
PASS: glsl/tests/optimization-test
PASS: glsl/tests/sampler-types-test
PASS: glsl/tests/uniform-initializer-test
PASS: nir/tests/control_flow_tests

07ec67d85ca652b76fb91abd68b6ad98e56a49df is the first bad commit
commit 07ec67d85ca652b76fb91abd68b6ad98e56a49df
Author: Kenneth Graunke <kenneth@whitecape.org>
Date:   Fri Mar 4 18:26:00 2016 -0800

    glcpp: Implicitly resolve version after the first non-space/hash token.
    
    We resolved the implicit version directive when processing control lines,
    such as #ifdef, to ensure any built-in macros exist.  However, we failed
    to resolve it when handling ordinary text.
    
    For example,
    
            int x = __VERSION__;
    
    should resolve __VERSION__ to 110, but since we never resolved the implicit
    version, none of the built-in macros exist, so it was left as is.
    
    This also meant we allowed the following shader to slop through:
    
            123
            #version 120
    
    Nothing would cause the implicit version to take effect, so when we saw
    the #version directive, we thought everything was peachy.
    
    This patch makes the lexer's per-token action resolve the implicit
    version on the first non-space/newline/hash token that isn't part of
    a #version directive, fulfilling the GLSL language spec:
    
    "The #version directive must occur in a shader before anything else,
     except for comments and white space."
    
    Because we emit #version as HASH_TOKEN then VERSION_TOKEN, we have to
    allow HASH_TOKEN to slop through as well, so we don't resolve the
    implicit version as soon as we see the # character.  However, this is
    fine, because the parser's HASH_TOKEN NEWLINE rule does resolve the
    version, disallowing cases like:
    
            #
            #version 120
    
    This patch also adds the above shaders as new glcpp tests.
    
    Fixes dEQP-GLES2.functional.shaders.preprocessor.predefined_macros.
    {gl_es_1_vertex,gl_es_1_fragment}.
    
    Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Comment 1 Ian Romanick 2016-03-09 00:23:45 UTC
That is odd.  Since Ken added some tests, I would think he would have seen such a failure.

Vinson, what were you building this on?
Comment 2 Vinson Lee 2016-03-09 00:28:35 UTC
(In reply to Ian Romanick from comment #1)
> That is odd.  Since Ken added some tests, I would think he would have seen
> such a failure.
> 
> Vinson, what were you building this on?

I can reproduce the make check failure on both Linux and Mac OS X.
Comment 3 Kenneth Graunke 2016-03-09 02:17:19 UTC
I didn't run "make check".  I ran glcpp-test directly.  I didn't know glcpp-test-cr-lf existed.

It looks like the new 146-* tests pass normal testing, but fail glcpp-test-cr-lf, with different newline terminators.  Presumably it outputs some different number of newlines.  I can't tell because apparently running that script directly doesn't work anymore.

I'll have to look into it later.
Comment 4 Kenneth Graunke 2016-03-09 02:57:03 UTC
My test actually exposed another preprocessor bug.  Namely, the character location is wrong for hashes on a single line.

#
#error foo

currently generates:

0:1(3): preprocessor error: #error foo

which has the character position as 3.  In the cr-lf mode, it generates 4.  It ought to be 1, however...
Comment 5 Kenneth Graunke 2016-03-09 03:10:15 UTC
Patch on mailing list (passes "make check"; still running it through Piglit):
https://lists.freedesktop.org/archives/mesa-dev/2016-March/109456.html
Comment 6 Kenneth Graunke 2016-03-10 19:28:49 UTC
commit e032e4ad5a3f73dffb1a8babcfd333954a574ffa
Author: Kenneth Graunke <kenneth@whitecape.org>
Date:   Tue Mar 8 19:03:11 2016 -0800

    glcpp: Fix locations when encounting "#<NEWLINE>".


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.