https://github.com/shader-slang/slang
Revision 922cd543d7b852fac4bd0b93d1a0c1a889c5c7e6 authored by Tim Foley on 31 January 2020, 17:37:00 UTC, committed by GitHub on 31 January 2020, 17:37:00 UTC
* Fix for a macro expansion bug

The work in #1177 added a notion of a macro being "busy" to prevent errors around recursively-defined macros:

    #define BAR(X) BAR(0)

    BAR(A) /* should yield BAR(0) and not infinite-loop */

The fix was to place an `isBusy` flag on each macro, setting it to `true` when an expansion of that macro gets pushed onto the stack of input streams, and back to `false` when the expansion gets popped from the stack of input streams.

That approach meant that we had to pre-expand macro arguments to avoid incorrectly treating a macro as busy for nested-but-not-recursive invocations:

    #define NEG(X) (-X)

    NEG(NEG(3)) // should expand to (-(-3)) and not (-NEG(3))

The subtle bug that arose with the `isBusy` flag is that the current preprocessor design doesn't always pop input streams when we might expect. For input like the following:

    BAR(A) BAR(B)

The preprocessor can be in a state where the stack of input streams look like:

    // top input stream:
    BAR(X) => BAR(0)
    ----------------^

    // bottom input stream
    BAR(A) BAR(B)
    ------^

That is, the first macro expansion is still "open," even though it is at the end of its input. When the preprocesor looks ahead and sees `BAR` as the next token and asks whether it should be expanded, the `isBusy` state on the BAR macro hasn't yet been unset, so expansion gets skipped.

Aside: it is completely reasonable to ask at this point whether we should just "fix" the behavior of not popping input streams that are at their end. We should consider doing that, but the current code goes to some lengths to preserve the current behavior and I do *not* recall why we had found it necessary. Any attempt to change that behavior should come along with lots of testing.

This change instead adds a different approach for tracking the busy-ness of macros that doesn't rely on mutable state in the macro, and thus works even without pre-expansion of macro arguments.

In the Slang preprocessor, macro names are always looked up in *environments*, where each environment maps names to macros, and has an option parent environment.
There is a global environment used for most cases, but when expanding a function-like macro we would create an "argument environment" for it that maps the parameter names to their argument tokens.

By expanding the environment type to include an (optional) field for a macro that is made "busy" by that environment, we can check if a macro is busy in a given environment by checking if the given macro has been associated with any of the environments on the chain of parent environments.

A function-like macro expansion could then attach the macro to the "argument environment" and automatically make itself busy for the purposes of expansion of its body.
To extend the same functionality to *all* macros, the "argument environment" was changes to an "expansion environment" that always gets used for a macro expansion and always has the identity of the macro attached.

Because the arguments to a function-like macro have their own environment for expansion that bypasses the argument/expansion environment of the function-like macro, the macro will show as busy in its own body, but not in the expansion of its arguments. Thus the pre-expansion of macro arguments is not needed with this change.

Aside: it might not be clear how this design avoids the problem of the unpopped input streams that aren't at their end. The answer is that the "current" environment for the preprocessor is already taken as the environment of the top-most input stream that is *not at its end*. This means that the new test for busy-ness benefits from the existing logic that was in place to deal with non popping input streams as soon as then end. (This is not to say that the current input-stream behavior isn't questionable)

This change includes a new test case for the behavior that was broken, and expands the test case from #1177 to also test object-like macros.

* Fix to handle the mutually-recursive case

The revised "busy" logic was unable to handle a mutually-recursive macro like:

    #define ABC XYZ
    #define XYZ ABC

This change restores the ability to handle mutually recursive cases, and makes sure that we test that case.

The crux of the fix relies on splitting the environment and busy-macro tracking into more separate data structures.
Rather than the list of environments directly trakcing busy-ness via the chain of parent environments, an environment now stores a separate linked list of macros that should be considered busy in that environment.

Decoupling the two lists allows for an important change: when expanding an ordinary macro (either object-like or function-like, but *not* a function argument) the parent *environment* comes from the point where the macro was defined (this is required for lookup to make sense), but the parent list of *busy macros* comes from the current input stream at the point where expansion takes place. That change ensures that non-argument macros always properly "stack up" the list of busy macros.
1 parent 52b78ad
History
Tip revision: 922cd543d7b852fac4bd0b93d1a0c1a889c5c7e6 authored by Tim Foley on 31 January 2020, 17:37:00 UTC
Fix for a macro expansion bug (#1192)
Tip revision: 922cd54
File Mode Size
docs
examples
external
prelude
source
tests
tools
.editorconfig -rw-r--r-- 937 bytes
.gitattributes -rw-r--r-- 95 bytes
.gitignore -rw-r--r-- 480 bytes
.gitmodules -rw-r--r-- 774 bytes
.travis.yml -rw-r--r-- 1.7 KB
CODE_OF_CONDUCT.md -rw-r--r-- 3.1 KB
LICENSE -rw-r--r-- 1.1 KB
README.md -rw-r--r-- 7.1 KB
appveyor.yml -rw-r--r-- 4.0 KB
premake5.lua -rw-r--r-- 29.4 KB
slang-com-helper.h -rw-r--r-- 4.8 KB
slang-com-ptr.h -rw-r--r-- 4.8 KB
slang-tag-version.h -rw-r--r-- 36 bytes
slang.h -rw-r--r-- 123.9 KB
slang.sln -rw-r--r-- 9.9 KB
test.bat -rw-r--r-- 1.4 KB
travis_build.sh -rw-r--r-- 460 bytes
travis_test.sh -rw-r--r-- 435 bytes

README.md

back to top