Fix a preprocessor bug affecting X-macros (shader-slang#1436)

* Fix a preprocessor bug affecting X-macros

Fixes shader-slang#1435

This bug exhibited as nondeterministic output from the preprocessor in release builds, but using a debug build it was narrowed down to a use-after-free issue.

The core problem is subtle, but relates to how we set up the linked list that represents the "busy" status of macros in a particular expansion environment.

Consider this scenario:

```hlsl
X(A)
```

The flow we expect from the preprocessor is something like:

1. Read the `X` token in `X(A)` and recognize the start of a function-like macro invocation. Create an expansion environment for `X`, with the global environment as a parent, read in the arguments (just `A`), and push that expansion onto the stack.

2. Read the `M` token that starts the expansion of `M`, and recognize it as an invocation of the object-like macro representing the argument `M`. Create an expansion environment for the definition of `M` (which is just `A`), and push it onto the stack.

3. Read the token `A` from the expansion for the argument `M`, and recognize it as an invocation of the function-like macro `A`. Create an expansion environemnt for `A`, with the current environment as its parent, read in the arguments (just `0`), and push that expansion onto the stack.

4. Read the token `y` from the expansion for `A`, and recognize it as an invocation of the object-like macro representing the argument `y`. Create an expansion environment for the definition of `y` (which is just `0`) and push it onto the stack.

5. Read `0`.

6. Read a bunch of end-of-file tokens that cause all of these expansions to be popped.

That all looks fine as written, but the gotcha is that the input stream for the expansion in step (2) is only a single token (`A`), which means that during step (3) the current input stream at the time we *create* the macro expansion for `A` is at the end of its input, and by the time we've read in the macro arguments that expansion will have been popped.

The problem, then, is that the logic for setting up the stack of "busy" macros was being performed at the beginning of the expansion (the part referred to as "create an expansion" above), when it should only have been set up as part of pushing the xpansion onto the stack (since at that point we have a guarantee that the parent expansion cannot be popped until the child expansion has been).

The fix here is thus pretty simple: we already have distinct operations for `initializeMacroExpansion()` and `pushMacroExpansion()`, and I simply moved the logic for setting up the "busy" state from the former to the latter.

* fixup: typo

Author: Tim Foley

source/slang/slang-preprocessor.cpp

@@ -586,7 +586,7 @@ static bool _isMacroBusy(PreprocessorMacro* macro, PreprocessorEnvironment* env)
 // Reading Tokens With Expansion
 //
 
-static void InitializeMacroExpansion(
+static void initializeMacroExpansion(
     Preprocessor*       preprocessor,
     MacroExpansion*     expansion,
     PreprocessorMacro*  macro)
@@ -627,6 +627,16 @@ static void InitializeMacroExpansion(
     // macro, that environment will be the environment where
     // the function-like macro was *invoked*, which might be
     // in the context of another macro expansion.
+}
+
+static void pushMacroExpansion(
+    Preprocessor*   preprocessor,
+    MacroExpansion* expansion)
+{
+    // Before pushing a macro as an input stream,
+    // we need to set the appropraite "busy" state
+    // that will be used during expansions of that
+    // macro's definition.
 
     // A macro is always busy in its own expansion environment,
     // to prevent recursive expansion. Here we construct a
@@ -639,6 +649,7 @@ static void InitializeMacroExpansion(
     // expansion. We could try to avoid the extra step at
     // the cost of a bit more code complexity here.
     //
+    auto macro = expansion->macro;
     expansion->busy.macro = macro;
     expansion->expansionEnvironment.busyMacros = &expansion->busy;
 
@@ -653,23 +664,19 @@ static void InitializeMacroExpansion(
         // This happens to be what is stored in the parent
         // environment.
         //
+        auto parentEnvironment = expansion->expansionEnvironment.parent;
         expansion->busy.next = parentEnvironment->busyMacros;
     }
     else
     {
-        // For the other cases (function-like and objet-like
+        // For the other cases (function-like and object-like
         // macros), the busy list should include anything
         // that was already busy in the environment that
         // is beginning to expand a macro.
         //
         expansion->busy.next = preprocessor->inputStream->environment->busyMacros;
     }
-}
 
-static void PushMacroExpansion(
-    Preprocessor*   preprocessor,
-    MacroExpansion* expansion)
-{
     PushInputStream(preprocessor, expansion);
 }
 
@@ -930,7 +937,7 @@ static void MaybeBeginMacroExpansion(
             }
 
             MacroExpansion* expansion = new MacroExpansion();
-            InitializeMacroExpansion(preprocessor, expansion, macro);
+            initializeMacroExpansion(preprocessor, expansion, macro);
 
             // Consume the opening `(`
             Token leftParen = AdvanceRawToken(preprocessor);
@@ -963,7 +970,7 @@ static void MaybeBeginMacroExpansion(
             // Now that the arguments have been parsed and validated,
             // we are ready to proceed with expansion of the macro body.
             //
-            PushMacroExpansion(preprocessor, expansion);
+            pushMacroExpansion(preprocessor, expansion);
         }
         else
         {
@@ -972,8 +979,8 @@ static void MaybeBeginMacroExpansion(
 
             // Object-like macros are the easy case.
             MacroExpansion* expansion = new MacroExpansion();
-            InitializeMacroExpansion(preprocessor, expansion, macro);
-            PushMacroExpansion(preprocessor, expansion);
+            initializeMacroExpansion(preprocessor, expansion, macro);
+            pushMacroExpansion(preprocessor, expansion);
         }
     }
 }

tests/preprocessor/x-macro.slang

@@ -0,0 +1,13 @@
+// x-macro.slang
+//TEST:SIMPLE:
+
+// Test the case of an "X macro" that takes another macro as a parameter
+
+#define X(M) M(0) M(1) M(2) M(3) M(4) M(5) M(6) M(7)
+
+#define A(x) + x + x + x
+
+int sum()
+{
+	return X(A);
+}