https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #2 from romulasry@gmail.com --- 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

https://bugs.freedesktop.org/show_bug.cgi?id=63579

romulasry@gmail.com changed:

What |Removed |Added ---------------------------------------------------------------------------- Attachment #78046|text/plain |image/png mime type| |

https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #4 from Vadim Girlin ptpzz@yandex.ru --- 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.

https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #6 from Vadim Girlin ptpzz@yandex.ru --- Created attachment 78052 --> https://bugs.freedesktop.org/attachment.cgi?id=78052&action=edit [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>

https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #8 from Ian Romanick idr@freedesktop.org --- 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. :)

https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #10 from Erik Faye-Lund kusmabite@gmail.com --- 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.

https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #12 from Erik Faye-Lund kusmabite@gmail.com --- Well, Khronos meetings don't define the spec, the specification does. And the specification is pretty clear here.

https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #14 from Ian Romanick idr@freedesktop.org --- (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 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. :)

https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #16 from Ian Romanick idr@freedesktop.org --- (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.

https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #18 from Ian Romanick idr@freedesktop.org --- (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.

https://bugs.freedesktop.org/show_bug.cgi?id=63579

--- Comment #20 from Alex Deucher agd5f@yahoo.com --- yes: http://cgit.freedesktop.org/mesa/mesa/commit/?id=f732036f12d67a96f546c11236f...