-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent __has_builtin
compiler differences from causing divergence
#7836
Conversation
06c6951
to
d22c9f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I see how it's a more reliable fix than the "assert __has_builtin()
is available" that we added to xsnap-worker.c
(a compiler which has __has_builtin()
but lacks __builtin_add_overflow()
would diverge under the xsnap-worker.c
hack, but would fail to link with this PR, and a linker failure is the better outcome). And it's more reliable than a batch of assert __has_builtin(__builtin_add_overflow)
checks at the start of xsnap-worker.c
, because XS might start using new builtins in the future.
I think __has_builtin()
is only used for potentially-builtin functions whose behavior and arguments are well-defined by the C specification. So if our override "return true" is at all different from the native version, the only difference will cause a linker error, not e.g. make XS call the function differently. It would not be safe to override a different sort of introspection function that had something to say about the argument signature.
|
||
const hexHash = hash.digest('hex'); | ||
t.log(hexHash); | ||
t.snapshot(hexHash, description); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a note here to remind future test-runners of what we're asserting.
This test snapshot was created from a version of xsnap that was manually observed to avoid the spill (because it has the __builtin
and can safely tell when it remains within the safe range of integers). We cannot test for the spill/not-spill directly without either parsing the xsnap
heap snapshot, or writing C test code that performs a multiplication and then inspects the .kind
field of the returned value. But we know that any change to the spill/non-spill behavior will change the heap snapshot hash.
The snapshot will change (and the test will fail) if either:
- XS is changed in a way that changes the spill behavior (e.g. maybe they give up on that optimization for some reason)
- XS is changed in any other way that changes the heap snapshot
It probably can't change in response to the compiler having/not-having the builtins, because the -D
change means the not-having-builtins cases will simply fail to link, well before we get to this test case. And it won't change in response to the compiler having/not-having __has_builtin()
, because the -D
means we completely ignore the normal __has_builtin()
.
So as automated tests go, it's a bit too sensitive to things that 1: will happen, eventually, and 2: aren't what we're trying to assert. That's fine, especially since checking the real thing is too hard, but we should add a note, so when the test fails and the developer is considering ava's --update-snapshots
, they can tell why this test was using a snapshot in the first place.
d22c9f8
to
a2b48e1
Compare
Prevent `__has_builtin` compiler differences from causing divergence
Prevent `__has_builtin` compiler differences from causing divergence
Prevent `__has_builtin` compiler differences from causing divergence
Prevent `__has_builtin` compiler differences from causing divergence
refs: #7829
refs: #7834
Description
As per #7829 (comment), I conducted an audit of
packages/xsnap
used by the Agoric SDK, and found that of the 6 uses of__has_builtin
, all of them were inpackages/xsnap/moddable
, used to test for the presence of__builtin_XXX_overflow
.Rather than feature-detection using
__has_builtin
to choose between different implementations ofxsnap
functionality, this PR redefines xsnap's__has_builtin(x)
to always return nonzero, so that xsnap does not compile successfully, but with behaviour that has proven to diverge between different compilers.This change means the xsnap build will assume that all builtins are present, and thus it will fail-safe (with a linker error) if the specified builtin symbol is not defined by the compiler rather than fail-unsafe by using a different, potentially incompatible code path.
Security Considerations
n/a
Scaling Considerations
Slightly improves performance under GCC 9 (and some versions of clang), which omits
__has_builtin
but does implement__builtin_XXX_overflow
.Documentation Considerations
May be able to document looser compiler chain/OS version requirements, if desirable.
Testing Considerations
Introduces a unit test to verify known problematic code does not cause the current platform's xsnap snapshots to diverge from expected hashes (generated on Ubuntu 22.04, amd64).
To run this test on your current platform:
(cd packages/xsnap && yarn test test/test-xsnap.js -m 'produce golden snapshot hashes')
Before this
__has_builtin
change, the test passed on ubuntu-2022.4 (GCC 11, amd64 and arm64) and Darwin (clang 14, arm64) but failed the multiplication test on ubuntu-2020.4 (GCC 9, amd64 and arm64). After the__has_builtin
change, the test passed on all the platforms listed above.