Skip to content
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

change xsnap to require all compiler builtins #7834

Open
warner opened this issue May 24, 2023 · 0 comments
Open

change xsnap to require all compiler builtins #7834

warner opened this issue May 24, 2023 · 0 comments
Labels
xsnap the XS execution tool

Comments

@warner
Copy link
Member

warner commented May 24, 2023

In #7829 (comment) we identified a consensus divergence in XS, between platforms that had __has_builtin() and ones that did not (specifically gcc-9, the default in Ubuntu-20.04-LTS, and gcc-10/11 in Ubuntu-22.04-LTS).

We chose to have xsnap to require that __has_builtin() is available, which was the more conservative approach in the moment, but it still leaves us open to valid compiler behavior creating consensus problems. It only helped us in this case because all the compilers we know about provide all the actual builtins, like __builtin_mul_overflow, and the specific problem was that XS couldn't (portably) determine whether or not the builtin was available.

With the approach we used, a compiler which has a__has_builtin() that returns false for __builtin_mul_overflow will cause XS to use the non-optimized path, and create an XS_NUMBER_KIND, where a more capable environment would create XS_INTEGER_KIND, causing divergence. If XS acquires more compile-time optimization options, these will be other opportunities for platform-triggered divergence.

@michaelfig and I developed a different approach, in which we edit the build process to use -D to hardwire __has_builtin(x) with a version that always returns true regardless of x. This will cause a linker error on a platform that lacks all of the builtins XS wants to use, which might be harder to diagnose, but would remove this degree of freedom and reduce the chances for divergence correspondingly. We didn't have the time to think through (or audit for) the consequences of this change, so we went with the other approach.

I think, in the vaults+1 timeframe, we should switch to the -D/always-true approach, as I think it'll be safer in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
xsnap the XS execution tool
Projects
None yet
Development

No branches or pull requests

1 participant