read the error after the call that sets it - #3
Merged
Merged
Conversation
Every fallible call in the header reads checked(zu_something(..., &err), err, "zu_something") and the order a compiler evaluates those two arguments in is unspecified. On one that reads err first, it reads it before the call beside it has run, which is to say it reads the null it was initialised to. Everything went on throwing and everything thrown was empty: the right status, no code, no condition, no position, and the name of the C function where the engine's sentence should have been. GCC picks that order. Clang picks the other one, which is why a header that has never been anything but wrong here has looked fine. The fix is not to hoist the call into a variable at forty three call sites, because that leaves the mistake available to the forty fourth. checked takes the slot by address now. What may be evaluated early is then where err lives rather than what it holds, and what it holds is read inside the call, after the call that fills it has certainly returned. It also clears the slot, so a caller who looks afterwards finds nothing to free twice. test_errors already had a case for this and was failing on it. It has one more now that says the whole sentence: a statement that does not parse carries a five character code, a position, a severity and a message that is not the name of the C function. The case above it allows a missing position, because a variable that is not defined has no token to point at, and an allowance is how this got through. The other failures on this branch are the engine having moved to ABI 0.14 and having reserved two more words, and are the next commit.
22 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every fallible call in the header reads
checked(zu_something(..., &err), err, "zu_something")and the order a compiler evaluates those two arguments in is unspecified. On one that reads
errfirst, it reads it before the call beside it has run, which is to say it reads the null it was initialised to. Everything went on throwing and everything thrown was empty: the right status, no code, no condition, no position, and the name of the C function where the engine's sentence should have been. GCC picks that order. Clang picks the other one, which is why a header that has never been anything but wrong here has looked fine.Through the wrapper, before this:
and through the C ABI with the same library, at the same moment:
The fix is not to hoist the call into a variable at forty three call sites, because that leaves the mistake available to the forty fourth.
checkedtakes the slot by address now. What may be evaluated early is then whereerrlives rather than what it holds, and what it holds is read inside the call, after the call that fills it has certainly returned. It also clears the slot, so a caller who looks afterwards finds nothing to free twice.test_errorsalready had a case for this and was failing on it. It has one more now that says the whole sentence: a statement that does not parse carries a five character code, a position, a severity and a message that is not the name of the C function. The case above it allows a missing position, because a variable that is not defined has no token to point at, and an allowance is how this got through.Measured on Linux with gcc 13 against libzu at the engine's HEAD:
test_errorsandtest_errors_cxx20go from 7 of 11 to 11 of 12, andtest_statementgoes green.The failures left over are the engine having moved to ABI 0.14 and having reserved two more words, and they are the next PR.