Fix to allow using the glslang_shader_parse from C API without preprocess

[antaalt]

Hello,

I went into problem with the C API of glslang. Here is a small repro sample of the issue:

#version 450
#extension GL_EXT_debug_printf : require
void main() {
    debugPrintfEXT("global thread id %v3u\n", gl_GlobalInvocationID);
}

// Simple C code to run 
glslang_shader_t* shader = glslang_shader_create(&shader_input);
glslang_shader_preprocess(shader, &shader_input);
glslang_shader_parse(shader.handle.as_ptr(), &shader_input);

This will produce an End of line in string error for \n within string, but glslangValidator does not produce this error. After digging into it, it seems calling preprocess will replace \n string by a newline feed character (Code at MachineIndependant/preprocessor/PpScanner.cpp:1136), which will then be used by parse & produce this error.

With this CL, I try to fix this by not forcing the parse function to have preprocess executed before. Right now the C API dont allow parsing without preprocessing while the C++ API allow it. This issue should probably be solved more deeply to allow preprocess & parsing aswell but as its a very minor issue, I dont think it should really be a problem.

[dnovillo]

Is there a way to add this fragment shader as a test case?

[antaalt]

I will try to look into it

[dnovillo]

I'm not sure this fix does what you meant. If the code is not pre-processed, we still fail to parse with:

ERROR: 0:4: 'gl_GlobalInvocationID' : undeclared identifier
ERROR: 0:4: '' : compilation terminated                                                                                 
ERROR: 2 compilation errors.  No code generated.     

Which I guess it's expected because gl_GlobalInvocationID is not expanded properly.

[dnovillo]

@antaalt are you still working on this PR? Thanks.

[antaalt]

Sorry, did not had the time to look into it, it compile fine for me if i specify the compute shader stage as gl_GlobalInvocationID is a compute only variable.

For me the issue is mostly due to the C API that requires to call preprocess before calling parse (it will crash if you dont), while with the C++ API, simply calling parse is enough. It looks like parse is doing the preprocessing steps aswell. Calling preprocess before calling parse will preprocess the code twice, which is the reason of the bug in this changelist.

Ideally, we should avoid preprocessing twice, this is a small fix to just allow us to run parse without calling preprocess before.

I will try to look to add some test for this.

[dnovillo]

Thanks for the new test. Would you consider adding a couple more tests that check for other escape sequences and macro expansion?

Also, if pre-processing has occurred, we should now just use the pre-processed string, right? Perhaps a test that validates this makes sense?

Thanks.

[dnovillo]

nit: s/C Api/C API/

[dnovillo]

No need to have this commented-out code here.

[dnovillo]

Well, yes, but why have it here commented out?

[antaalt]

Mostly for keeping track of the issue, but that might be overkill, I will remove it

[antaalt]

That is what the code was doing, but it failed if we did not preprocessed. I will see if i can add this, but maybe it would be more interesting to try to look into why parse get to the preprocessing pass even if it was already preprocessed (maybe per safety as we cant really tell if it input to parse was preprocessed), as its the root of the issue.

[dnovillo]

That is what the code was doing, but it failed if we did not preprocessed. I will see if i can add this, but maybe it would be more interesting to try to look into why parse get to the preprocessing pass even if it was already preprocessed (maybe per safety as we cant really tell if it input to parse was preprocessed), as its the root of the issue.

Apologies. I'm not sure what you mean by this. You're saying that the parser is pre-processing even when it does not need to?

[antaalt]

Well from what I saw, I think the parser preprocess the input string without caring if we called preprocess before and if we passed a preprocess string or a non preprocessed one, which make sense as with the c++ api, we can just call parse without preprocess, and preprocess will still be done.

So the issue in the example is that the "/n" string is transformed into a '/n' character by the preprocess step. And the parse step is then outputting an error as it again try to preprocess the string, as it does not understand why the string has a '\n' character at the end. But it was not a '\n' character before the preprocessing step but a "\n" string.

So skipping the preprocess fix this because it does not change the string to a character in the input.

Logic is here https://github.com/KhronosGroup/glslang/blob/main/glslang%2FMachineIndependent%2Fpreprocessor%2FPpScanner.cpp#L1206

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}