Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arithmetic in macros only works with signed integers #2515

Open
devshgraphicsprogramming opened this issue Jan 29, 2021 · 13 comments
Open

Arithmetic in macros only works with signed integers #2515

devshgraphicsprogramming opened this issue Jan 29, 2021 · 13 comments
Assignees

Comments

@devshgraphicsprogramming

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

@greg-lunarg
Copy link
Contributor

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

@AnastaZIuk
Copy link

AnastaZIuk commented Jan 31, 2021

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

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 greg-lunarg reopened this Feb 17, 2021
@greg-lunarg
Copy link
Contributor

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

@greg-lunarg
Copy link
Contributor

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

@greg-lunarg
Copy link
Contributor

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
Copy link
Author

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

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

@greg-lunarg greg-lunarg self-assigned this Feb 17, 2021
@gnl21
Copy link
Contributor

gnl21 commented Feb 18, 2021

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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Contributor

Changing back to bug to address cryptic error message above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment