KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
3.01k stars 829 forks source link

Arithmetic in macros only works with signed integers #2515

Open devshgraphicsprogramming opened 3 years ago

devshgraphicsprogramming commented 3 years ago

As soon as any contant has a u suffix the compiler "explodes"

greg-lunarg commented 3 years ago

@devshgraphicsprogramming Can you please supply a sample shader that exhibits the problem?

AnastaZIuk commented 3 years ago

@greg-lunarg

// Our API picked and put there
#define NBL_GLSL_EVAL(X) X
#define NBL_GLSL_AND(X,Y) (NBL_GLSL_EVAL(X)&NBL_GLSL_EVAL(Y))
#define NBL_GLSL_EQUAL(X,Y) (NBL_GLSL_EVAL(X)==NBL_GLSL_EVAL(Y))
#define NBL_GLSL_NOT_EQUAL(X,Y) (NBL_GLSL_EVAL(X)!=NBL_GLSL_EVAL(Y))

#define NBL_GLSL_ADD(X,Y) (NBL_GLSL_EVAL(X)+NBL_GLSL_EVAL(Y))
#define NBL_GLSL_SUB(X,Y) (NBL_GLSL_EVAL(X)-NBL_GLSL_EVAL(Y))

// comment it out to make it not working
// #define SYNTAX_ERROR_ENABLE

#ifdef SYNTAX_ERROR_ENABLE
#define AN_EXAMPLE_OF_EXPLODE_MAX 256u
#define AN_EXAMPLE_OF_EXPLODE_MIN 2u
#else
#define AN_EXAMPLE_OF_EXPLODE_MAX 256
#define AN_EXAMPLE_OF_EXPLODE_MIN 2
#endif

#if NBL_GLSL_NOT_EQUAL(NBL_GLSL_AND(NBL_GLSL_SUB(AN_EXAMPLE_OF_EXPLODE_MAX,AN_EXAMPLE_OF_EXPLODE_MIN),255),0)
    #define test 0 // there will be a syntax error if SYNTAX_ERROR_ENABLE is defined
#endif

you can put it wherever in vertex shader, it doesn't work when it gets executed with u postfix

greg-lunarg commented 3 years ago

When you say "explodes", do you mean "gives a large number of error messages but terminates cleanly"? Or do you mean "segfaults, crashes or otherwise terminates out of control"?

I am seeing the following:

ERROR: t4.frag:28: 'preprocessor evaluation' : bad expression ERROR: t4.frag:28: '#if' : unexpected tokens following directive ERROR: t4.frag:28: '' : missing #endif ERROR: 3 compilation errors. No code generated.

greg-lunarg commented 3 years ago

Looking at the glslang source code and tests, it seems fairly clear that the implementers did not intend to support unsigned integer expressions in the preprocessor. So when the GLSL spec says that it supports "expressions operating on literal integer constants", I am presuming by "integer" the community has interpreted it as "int" or "signed integer". In other words, I do not consider this a case where the implementation is deficient of the specification. As such, it is not within my current charter to address this.

Nonetheless if you or someone else wishes to add support for unsigned integer here, I am going to guess that no one would complain, and that no change in the spec would be necessary to allow it. If someone does intend to add this support, it might be good to check with a few others in the community first. So please check in with me first.

As such I will close this issue for now. If indeed glslang is terminating uncleanly in the presence of uint constants, please reopen and I will address that issue.

devshgraphicsprogramming commented 3 years ago

It would be good if we got an error saying "unsigned integer literal constant in preprocessor expression" instead of the cryptic

ERROR: t4.frag:28: 'preprocessor evaluation' : bad expression
ERROR: t4.frag:28: '#if' : unexpected tokens following directive
greg-lunarg commented 3 years ago

OK. I have re-opened with this latest request.

greg-lunarg commented 3 years ago

@devshgraphicsprogramming Are you planning to make this improvement or shall I?

greg-lunarg commented 3 years ago

You may also wish to open a request to update the GLSL spec to specify that #if expressions must be signed integer. I will ping the spec editor @gnl21 to get his feedback.

devshgraphicsprogramming commented 3 years ago

OK. I have re-opened with this latest request.

I dont think I've ever touched that part of glslang

gnl21 commented 3 years ago

You may also wish to open a request to update the GLSL spec to specify that #if expressions must be signed integer.

This is not how I had interpreted the GLSL spec, but we should clarify whether it is intended to work or not. I'll create a GLSL issue to confirm what the spec intends and will clarify once it's confirmed.

pdaniell-nv commented 3 years ago

@devshgraphicsprogramming we discussed changing the GLSL spec to support unsigned, but it will likely take a while since it'll need CTS coverage and all implementations to support it. How urgent is this for you? I assume you can work-around the glslang issue in the meantime?

devshgraphicsprogramming commented 3 years ago

@devshgraphicsprogramming we discussed changing the GLSL spec to support unsigned, but it will likely take a while since it'll need CTS coverage and all implementations to support it. How urgent is this for you? I assume you can work-around the glslang issue in the meantime?

dont worry I've already converted all my macros to use signed, so I can wait.

greg-lunarg commented 3 years ago

Changing back to bug to address cryptic error message above.

marauder2k7 commented 5 days ago

We are having a similar issue we are passing macros as preamble but using safeties.

#ifndef DEBUGVIZ_ATTENUATION
#define DEBUGVIZ_ATTENUATION 0
#endif
...
#if DEBUGVIZ_ATTENUATION == 1
      float contribAlpha = 1;
      for (i = 0; i < numProbes; i++)
      {
         contribAlpha -= contribution[i];
      }

      return float4(1 - contribAlpha, 1 - contribAlpha, 1 - contribAlpha, 1);
#endif

get these errors:

Failed to Parse Shader: reflectionProbeArrayP.hlsl ERROR: ../../lighting.hlsl:480: 'preprocessor evaluation' : bad expression ERROR: ../../lighting.hlsl:480: '#if' : unexpected tokens following directive ERROR: 0:10: 'texture_BRDFTexture' : redefinition ERROR: 0:10: '' : missing #endif ERROR: 0:10: 'BRDFTexture' : redefinition ERROR: 0:10: ';' : Expected ERROR: 0:10: 'declaration' : Expected

Line 480 relates to the #if DEBUGVIZ_ATTENUATION == 1 line. The file lighting.hlsl is included across a few shaders.