Bug 63579

Created attachment 78047 [details]
Top of screen *sky* renders white

I did a git bisect for this and got:

02b808b08acc73b9b3d31832a7f137a9aae4bdd9 is the first bad commit
commit 02b808b08acc73b9b3d31832a7f137a9aae4bdd9
Author: Francisco Jerez <currojerez@riseup.net>
Date:   Sun Apr 7 18:31:06 2013 +0200

    clover: Fix usage of incorrect object as destination in clEnqueueCopyBufferToImage.
    
    Signed-off-by: Francisco Jerez <currojerez@riseup.net>

:040000 040000 ab0699e5d6ff9b4ebf721a6a750e96e15d76b209 e28c39023f81b08b323a1062f957ceb3824bd860 M    src

I believe the git before is incorrect, I did another:
The merge base 2d2f1fd164218eacf2b142bc808be1f25f66e72c is bad.
This means the bug has been fixed between 2d2f1fd164218eacf2b142bc808be1f25f66e72c and [1c1b4244081f2ad2c84a80a6f68a6b7fd1aefbe9].

Looks like some shaders related to sky rendering are not compiled successfully. The problem is that there is a '\' (line-continuation) character in the comment, followed by the line that is not expected to be a part of comment:

#version 120
....
//=============================================================================\
uniform vec4    vColor;

AFAICS the line-continuation character was only introduced in the GLSL 4.20 spec, and according to the spec next line should be considered a part of the comment in this case. I'm not sure though how this should be handled for previous GLSL versions.

So either this is incorrectly handled by the GLSL compiler, or the shader itself is incorrect. In any case it's not a bug of the r600g driver.

I think the easiest solution would be for game devs to remove the '\' character from that comment to avoid this uncertainty and undefined behavior, probably it was added accidentally anyway. You might want to report this to the game developers.

(In reply to comment #4)
> Looks like some shaders related to sky rendering are not compiled
> successfully. The problem is that there is a '\' (line-continuation)
> character in the comment, followed by the line that is not expected to be a
> part of comment:
> 
> #version 120
> ....
> //
> =============================================================================
> \
> uniform vec4    vColor;
> 
> AFAICS the line-continuation character was only introduced in the GLSL 4.20
> spec, and according to the spec next line should be considered a part of the
> comment in this case. I'm not sure though how this should be handled for
> previous GLSL versions.
> 
> So either this is incorrectly handled by the GLSL compiler, or the shader
> itself is incorrect. In any case it's not a bug of the r600g driver.
> 
> I think the easiest solution would be for game devs to remove the '\'
> character from that comment to avoid this uncertainty and undefined
> behavior, probably it was added accidentally anyway. You might want to
> report this to the game developers.

The shader is incorrect. There is a drirc workaround for this already. I suppose it's just not wired up for Gallium.

Created attachment 78052 [details] [review]
[PATCH] gallium: handle drirc disable_glsl_line_continuations option

This patch will allow to use the workaround with gallium drivers, either by setting the application-specific option value in the drirc config file (e.g. with driconf gui), or by using the environment variable:

disable_glsl_line_continuations=true <app_cmd_line>

Is the shader really incorrect? I notice that it declares "#version 120" in the first line, and the GLSL 1.2 spec explicitly states that there is no line-continuation character (http://www.opengl.org/registry/doc/GLSLangSpec.Full.1.20.8.pdf - section 3.1... "There is no line continuation character.").

Yes, this has changed in later GLSL-versions (the change happend between the 4.10 spec and the 4.20 spec), so I believe the line-continuation parsing should only be enabled in the presenece of a "#version NNN" where NNN >= 420.

The problem is that you can have comments *before* the #version line.  Because of that (and every other vendor has implemented line continuation in some form since forever), Khronos decided to make the change retroactive.  This shader in this game is *the* reason we added the driconf option. :)

I don't know where you have the retroactive-story from, but the specification does not mention it being retroactive, and even goes as far as to say:

"This document specifies only version 4.20 of the OpenGL Shading Language. It requires __VERSION__ to substitute 420, and requires #version to accept only 420. If #version is declared with a smaller number, the language accepted is a previous version of the shading language, which will be supported depending on the version and type of context in the OpenGL API. See the OpenGL Graphics System Specification, Version 4.2, for details on what language versions are supported. Previous versions of the OpenGL Shading Language, as well as the OpenGL ES Shading Language, are not strict subsets of the version specified here, particularly with respect to precision, name-hiding rules, and treatment of interface variables. See the specification corresponding to a particular language version for details specific to that version of the language."

And new revisions of those older shader-languages specifications have not been issued. You are right in saying that the specs allows for comments before the #version-string, but IMO the only reasonable thing to do in such a case is to not support line-continuation characters until a version declaration has been defined.

As for what "every other vendor has implemented line continuation in some form since forever", I can tell you that at least my NVIDIA OpenGL 4.3 driver does *not* implement line continuation, seemingly in *any* form. Not even when the shader starts with "#version 420".

Another data-point: My AMD driver also does not support line continuation characters by default, although they do *after* a "#version 420" line.

So in conclusion, I think blindly supporting line-continuation without a "#version 420"-line is both in violation of the spec as well as diverging from other implementations in a way that can only create problems.

(In reply to comment #9)
> I don't know where you have the retroactive-story from, but the
> specification does not mention it being retroactive, and even goes as far as
> to say:

I have the story from being in the Khronos meetings.

Well, Khronos meetings don't define the spec, the specification does. And the specification is pretty clear here.

Comment on attachment 78052 [details] [review]
[PATCH] gallium: handle drirc disable_glsl_line_continuations option

You'll probably want review from someone that actually works on Gallium code, but this patch is

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

You should also add

NOTE: This is a candidate for the 9.1 branch.

to the commit message.

(In reply to comment #12)
> Well, Khronos meetings don't define the spec, the specification does. And
> the specification is pretty clear here.

The discussion was at the time of the vote, and I stand by that.  I can't make the meeting minutes public, and I'm not sure that's the most important thing here.  However, I can explain the rationale behind our position for Mesa's compiler.  Since you won't be satisfied by the short version of the story, prepare for a long, twisting tale...

The problem is that, following the C model of compilation, line continuation is a separate compilation phase, and it occurs before preprocessing.  As a result, it's a shocking amount of work to *correctly* dynamically switch behaviors depending on the contents of the shader.  The implementation is also likely to have compilation overhead for every shader.  Consider a pathological shader like:

// \
/*
#version 120
// */ \
#version 420

Shaders similar to this one were discussed by Khronos during OpenGL ES 3.0 and OpenGL 4.2 development.  Suffice to say, more time than seems reasonable was spent trying to decide 1. what that shader should do, and 2. how to craft spec language to describe that.  This is also why issue 12.19 in the GLSL ES 3.0 spec is resolved, "Line-continuation to be made optional in GLSL ES 1.00."

Mesa's current GLSL compiler originally had line continuation, but, after having a problem with this game, we removed it.  I can't find a specific commit, so this may have been removed before the preprocessor was merged.  We encountered a small number of shaders that wanted continuation in preprocessor lines (#define and #line), and these shaders all compile on both NVIDIA and AMD.  We added continuation support for those cases in 2010 (commit bc1097d).

Both 3.00 ES and 4.20 require continuation, so we had to add it back for all cases.  In doing so, of course, we noticed that the Savage2 shader stopped compiling.  We thought about just supporting continuation in ES contexts, but the problem still exists since we expose GL_ARB_ES3_compatibility.

We searched our internal database of a few thousand shaders from shipping applications (open- and closed-source).  This was the only shader didn't want continuation behavior.  There was a very small number of shaders that used continuation.  If it weren't for the Savage2 shader, frankly, we wouldn't have implemented the non-continuation behavior at all... and nobody would have ever noticed.

There were zero shaders that both wanted continuation and non-continuation behavior, and there were zero applications that contained both a continuation shader and a non-continuation shader.  We just couldn't justify either the effort or the added code complexity to support dynamic selection.  We also couldn't justify breaking a shipping application that previously worked.  The right compromise was to have a workaround for the one case that required the non-continuation behavior, and this game works fine on the i965 driver.  The Gallium drivers just need to use the workaround, and attachment #78052 [details] [review] should do that.

We probably could have done a better job telling the other driver authors that they needed to support this option when it was added.  We were overwhelmingly busy trying to get ES3 support finished, so some things like that fell through the cracks.  We put the patches on the mailing list (and the commit message in 4b00ece specifically says it fixes Savage2), so it's not like we were being sneaky.  We can only do better in the future.

The part you didn't mention from section 3.1 of the 1.20 spec is that \ is not listed as part of the character set.  If we really want to argue the letter of the spec, the shader should fail to compile because it contains an invalid character. :)

Thanks for the clarification, but I'm still not entirely convinced.

I agree that this per-spec for OpenGL ES 3.0 (although I'm a bit disappointed that the ES3-group missed that we in the ES2-group had made it easy for you by requiring #version to be the first bytes, if present), and that there are spec-justification to rejecting the shader in question to compile (due to the character being outside of the character set). And I think we both agree that doing the latter would be a bad idea.

But I don't agree that this is per-spec for OpenGL nor OpenGL ES 2.0. It's the ratified spec that is the standard, not whatever discussions were held during the meeting. And even though you have a large collection of shaders that does not use it, I don't think breaking existing (unknown) applications is a good idea. How many shaders besides syntetic glsparsertest-shaders requires line-continuation to work correctly? My guess would be zero; shaders like these would not compile on AMD, NVIDIA, nor Intel's Windows drivers. I've just tested the latter. So apparently, Mesa is the only major OpenGL implementation that currently implements this.

By the way, the WebGL conformance tests also checks that line continuation does not work. So there are at least two known, publically available shaders that depends on no line-continuation to work. Of course, the latter is synthetic, but at least it's based on wording in a specification.

I'm not trying to be a pain here, I just think you're pushing for a direction that just leads to even more fragmentation and pain.

(In reply to comment #15)
> Thanks for the clarification, but I'm still not entirely convinced.
> 
> I agree that this per-spec for OpenGL ES 3.0 (although I'm a bit
> disappointed that the ES3-group missed that we in the ES2-group had made it
> easy for you by requiring #version to be the first bytes, if present), and
> that there are spec-justification to rejecting the shader in question to
> compile (due to the character being outside of the character set). And I
> think we both agree that doing the latter would be a bad idea.
> 
> But I don't agree that this is per-spec for OpenGL nor OpenGL ES 2.0. It's
> the ratified spec that is the standard, not whatever discussions were held
> during the meeting. And even though you have a large collection of shaders
> that does not use it, I don't think breaking existing (unknown) applications
> is a good idea. How many shaders besides syntetic glsparsertest-shaders
> requires line-continuation to work correctly? My guess would be zero;
> shaders like these would not compile on AMD, NVIDIA, nor Intel's Windows
> drivers. I've just tested the latter. So apparently, Mesa is the only major
> OpenGL implementation that currently implements this.

They all support it in preprocessor directives.  We verified this in 2010 when we added that level of support back.  As I said before, we've encountered shaders in games that use line continuation for multi-line macros.

#define foo(a, b) \
    do { \
        b = bar(a); \
    } while(0)

Making the support general (instead of just for preprocessor directives) simplified the code greatly.  Since I'm responsible for maintaining this code base as my job, that's a strong incentive for me.

> By the way, the WebGL conformance tests also checks that line continuation
> does not work. So there are at least two known, publically available shaders
> that depends on no line-continuation to work. Of course, the latter is
> synthetic, but at least it's based on wording in a specification.

I'm not going to add complexity or overhead to the preprocessor for this case.  If WebGL tests non-continuation behavior, we can add the browsers to the workaround list.

> I'm not trying to be a pain here, I just think you're pushing for a
> direction that just leads to even more fragmentation and pain.

(In reply to comment #16)
> (In reply to comment #15)
> > Thanks for the clarification, but I'm still not entirely convinced.
> > 
> > I've just tested the latter. So apparently, Mesa is the only major
> > OpenGL implementation that currently implements this.
> 
> They all support it in preprocessor directives.  We verified this in 2010
> when we added that level of support back.  As I said before, we've
> encountered shaders in games that use line continuation for multi-line
> macros.
> 
> #define foo(a, b) \
>     do { \
>         b = bar(a); \
>     } while(0)

That might very well be the case. But this ticket is not about line continuation in pre-processor directives. My test were in comments, as is the issue with this ticket. And none of them implementations listed above supports them in comments before a "#version 420" statement (if supported at all).

> Making the support general (instead of just for preprocessor directives)
> simplified the code greatly.  Since I'm responsible for maintaining this
> code base as my job, that's a strong incentive for me.

I'm sorry if this is a bit blunt, but wow. That's one of the least appealing arguments I've heard in a long time. Not only is someone paying you for your time, you think that's a *justification* for not supporting the standard? That's pretty much the oppositte of how this usually works.

So, as a paying customer of Intel, where would I file a bug-report that Intel will deal with in a responsible way?

> > By the way, the WebGL conformance tests also checks that line continuation
> > does not work. So there are at least two known, publically available shaders
> > that depends on no line-continuation to work. Of course, the latter is
> > synthetic, but at least it's based on wording in a specification.
> 
> I'm not going to add complexity or overhead to the preprocessor for this
> case.  If WebGL tests non-continuation behavior, we can add the browsers to
> the workaround list.

Fair enough. At least when considered in isolation.

(In reply to comment #17)
> (In reply to comment #16)
> > Making the support general (instead of just for preprocessor directives)
> > simplified the code greatly.  Since I'm responsible for maintaining this
> > code base as my job, that's a strong incentive for me.
> 
> I'm sorry if this is a bit blunt, but wow. That's one of the least appealing
> arguments I've heard in a long time. Not only is someone paying you for your
> time, you think that's a *justification* for not supporting the standard?
> That's pretty much the oppositte of how this usually works.
> 
> So, as a paying customer of Intel, where would I file a bug-report that
> Intel will deal with in a responsible way?

What are you talking about?  This game works on the i965 driver.  I'm sorry that you don't think the maintainability of a multimillion line code base is an appealing argument.

Is this patch upsteam in git?

yes:
http://cgit.freedesktop.org/mesa/mesa/commit/?id=f732036f12d67a96f546c11236fa635b3eda6e9c

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/433.