GDShader has several syntax issues when you #define function-like macros

[geekley]

Tested versions

• Reproducible in v4.3.stable.flathub [77dcf97]

System information

Godot v4.3.stable (77dcf97) - Freedesktop SDK 23.08 (Flatpak runtime) - X11 - Vulkan (Forward+) - integrated Intel(R) HD Graphics 5500 (BDW GT2) - Intel(R) Core(TM) i5-5300U CPU @ 2.30GHz (4 Threads)

Issue description

I've identified several issues when defining macros with arguments in GDShader preprocessor.
Issues are confirmed by comparing expected behavior to GLSL (ShaderToy / glslang) and C Preprocessor (GCC).

• 1. Preprocessor must report error on duplicate parameter names in macros
• 2. Preprocessor should leave names of function-like macros alone when used without parentheses
• 3. Preprocessor should allow function-like macros with no parameters (empty parentheses)
• 4. Preprocessor should not replace parameter names inside strings when expanding macros - (won't fix)
• 5. Macro expansion could disallow recursion to avoid issues
• 6. Expand macro itself before (not after) its arguments
• 8. Replace macro arguments all at once, not sequentially
• 9. Forbid defining or undefining a macro named "defined"

Issue 1: Report duplicate macro parameters

GLSL in ShaderToy gives the following error in this case:

'myParam' : duplicate macro parameter name

In the C preprocessor (GCC) the error looks like this:

test.c:1:14: error: duplicate macro parameter "myParam"

Issue 2: Leave function-like macro name alone when used without parentheses

There is another inconsistency between GDShader and GLSL/C preprocessors.
If you define a function-like macro with parentheses, GLSL and C will still allow that name to appear without parentheses and leave it alone (it won't try to expand it). GDShader, however, doesn't do this, and its way of handling it causes issues (not entirely sure to what extent, but I found a very weird issue in my tests).

Issue 3: Support function-like macros with zero parameters

GLSL replaces macroName() (no arguments, but including the parentheses) when it's defined with parentheses, and just macroName if it's defined without parentheses. I tested the C preprocessor (GCC) and it works the same way.
This should be allowed in GDShader as well.

Issue 4: Do not expand macro parameters defined inside strings

Strings are not supported in GLSL, but since they are going to be supported in GDShader, their handling in the preprocessor should match C. I assume it's still an issue, but note that I didn't test this against master (please confirm).
Parameters in macros are replaced wherever they appear in their definition. But if they appear inside a string, it should of course not be replaced. That's how the C preprocessor handles it.

Other issues: see comments below

Steps to reproduce

Hint: Using an invalid token like $ logs the preprocessor output (even on otherwise "valid" code) to help testing.

issue1.gdshader

shader_type spatial;
#define join(x, x) x ## x
// it seems to be simply using just the first parameter name
// it must raise a preprocessor error like "duplicate macro parameter name"
const int join(a,b) = join(1,2); // becomes: `const int  aa  =  11 ;`
$ // voluntary error to log preprocessor output

issue2.gdshader

shader_type spatial;
#define bar(x) x ## x
const int a = bar a b / c d bar(12);
// incorrectly results in this code: `const int a =  1212 ;`
// no idea what the preprocessor is doing here; ignoring everything between macro name and `(` perhaps?
// should be: `const int a = bar a b / c d 1212 ;`
$ // voluntary error to log preprocessor output

issue3.gdshader

shader_type spatial;
#define foo() whatever()
// raises "invalid argument name" error
// empty parentheses should be allowed; this form must require parentheses to expand it
foo foo() foo // should become: foo whatever() foo

issue4.gdshader

shader_type spatial;
#define str(x) "x"
uniform int a: hint_enum(str(etc)); // should be "x", not "etc"
$ // voluntary error to log preprocessor output

Minimal reproduction project (MRP)

N/A

[geekley]

Interestingly, I also found 2 more GDShader preprocessor behaviors that differ from C/GLSL.
These are w.r.t. macro expansion. Should we consider these as bugs too?

The preprocessor in C and GLSL:

• apparently has no issues with recursion, as the same macro name is not expanded again in its result
• expands the macro call itself before its args

The preprocessor in GDShader:

• has issues with macro recursion "Macro expansion limit exceeded"
• expands macro call args before the macro call itself

Test 5: does it have no macro recursion issues?

#define selfCall(x) x(x)+x(X)

selfCall(selfCall);
// C does: selfCall(selfCall)+selfCall(X);
// GLSL does: selfCall(selfCall) + selfCall(X);
// GDShader reports error: Macro expansion limit exceeded.

Test 6: does it expand the macro itself before its arguments?

#define Twice(x) x ## x
#define kJoin(x) k ## x(1)

kJoin(Twice(j));
// C does: kTwice(j)(1);
// GLSL does: kTwice(j) (1);
// GDShader differs: kjj(1) ; // also kjj(1) even if we #define kJoin(x) above Twice(x)

C tests: gcc -E macroExpansions.c
GLSL tests: glslang -E macroExpansions.frag.glsl
GDShader tests: add shader_type spatial; in Godot's Shader Editor and check logged output

[geekley]

Found another inconsistency. Not sure if it's to be considered a bug, should I open a new issue?
EDIT 3: moved to new issue #96504 since this not directly related to macros

In GDShader #if conditions, you can use several operators and seemingly even functions.
Which ones are undocumented.
E.g. ternary operator doesn't work (like in GLSL, unlike in C), but bitwise AND does.
It's not all of the math functions listed here either -- some functions like inversesqrt don't work in a simple test, whereas sqrt and sin seem to work.

In any case, I'm surprised things like functions work at all. I think dealing with integers and booleans would be enough.
In fact, if the intention is to match C/GLSL, then GDShader is doing way more than it should in the preprocessor #if directives. Not even C deals with float or boolean constants in the preprocessor, let alone math functions. GLSL doesn't either. They do handle integers, but not uint.
C does handle arbitrary names, treating them like 0 (even true), but GLSL doesn't allow them on #if.
GDShader allows most literals (bool, int, intHex, uint, float), except for uintHex like 0x0u.

---

EDIT:
Seems like it's evaluating GDScript code (EDIT 2: actually not sure now, it doesn't allow functions in @GDScript) so it's accepting functions in @GlobalScope as well as some constructors like bool etc. It allows even functions very weird to allow, like randf, instance_from_id and rid_from_int64. Void functions are not allowed. I don't know to which extent it allows functions with side-effects and whether it affects the editor. This also explains why C-syntax ternary ? : is not allowed, as it works in GDScript syntax #if a if b else c (EDIT 2: actually no, it seems to be just ignoring the invalid if ... part after the first expression, hence the confusion).
EDIT 4: presumably, it's likely using something like Godot core's Expression

In any case, this feels like an oversight, is this intended?

[pirey0]

Good findings!

I've addressed issues 1-3 in #96327

4 - expansion of macros in strings):
is clearly unhandled until now: Macro identifiers are matched literally without any context-aware checks for strings.
I can't think of any non-hacky fix with the way things are implemented now..

5 - recursion:
The recursion is clearly intentional in the code, but I personally agree that we should match the c/glsl preprocessor as closely as possible unless there is a really good reason not to.

6 - expansion order:
macros are expanded in definition-order which can cause a lot of unexpected behaviour.. Probably should be cleared up in a rendering meeting before taking action.

7 - function calls in #if directives:
Really interesting stuff! Looks like it could be very useful and at the same time very dangerous. Again probably a topic for a rendering meeting (unless this was already discussed in the past.)

[geekley]

I found another issue related to macro expansion and listed it above.

Issue 8: Replace macro arguments all at once, not sequentially

If macro arguments are replaced sequentially, then an argument containing a parameter name that is yet to be replaced will cause that replacement to incorrectly affect it again.

In the code below, the 1st expansion argument (assigned to parameter a) contains b, which is the name of a posterior parameter. On the current implementation, replacement of arguments seems to be done sequentially (multiple passes, one for each argument), causing the inserted b to be matched for replacement as well. This doesn't happen on C and GLSL.

issue8.gdshader

shader_type spatial;
#define f(a, b, c) a+b*c
f(b,2,b)
$
// C does: b+2*b
// GLSL does: b + 2 * b
// GDShader differs: 2+2*b

[pirey0]

I brought up this Issue in the 2024-09-03 Rendering Meeting, these are the key points:

• Aim for glsl-like behavior, so that shaders can be ported over very easily
• Do not bloat the preprocessor codebase
• May need to scope back some of the eval() stuff in #if directives

Conclusions for me are:

• Address issues 5, 6 and 8 to match glsl and add more tests.
• To not bloat the codebase leave issue 4 in, since strings in shaders aren't a matter of concern
• move discussions about issue 7 to the dedicated issue geekley opened

Update: I fixed 5,6 and 8 in my PR above

[geekley]

Found another minor inconsistency. Unsure if we should bother with it, but reporting it here at least for the memes.

Issue 9: Forbid defining or undefining a macro named "defined"

C and GLSL both disallow using the name of the preprocessor function defined as a macro name in #define (with or without parameters) or #undef. GDShader doesn't treat it specially.

issue9.gdshader

shader_type spatial;
#define f(define, defined) g(define, defined)
#define defined(defined) h(defined)
#define define(define) i(define)
#if defined(defined)
#define test is_defined
#endif
test
f(1,2)
defined(1)
define(1)

C: error: "defined" cannot be used as a macro name
GLSL: ERROR: '#define' : "defined" can't be (un)defined: defined
GDShader:

is_defined
g(1, 2)
h(1)
i(1)

Interestingly, they seem to allow defined as a parameter name just fine.

shader_type spatial;
#define f(define, defined) g(define, defined)
#define d(defined) h(defined)
#define define(define, defined) i(define, defined)
#if defined(d) && defined(define)
#define test is_defined
#endif
test
f(1,2)
d(1)
define(1,2)

C, GLSL and GDShader have a similar output:

is_defined
g(1, 2)
h(1)
i(1, 2)

Alternative name for the issue: "We can't #define a macro as defined. We can't #undefine a defined macro. We can't do anything here! A defined macro is not supposed to be defined at all!"