longlong: Add sanitizer builds, fix revealed problems. #17709
Closed
Conversation
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
This fixes the following two flavors of diagnostics:
```
../../py/objint_longlong.c:215:30: runtime error:
left shift of negative value -10000000000000000
../../py/objint_longlong.c:215:30: runtime error:
left shift of 72623859790382856 by 8 places cannot
be represented in type 'long long int'
```
This formulation compiles to identical code (gcc 15 -m32 -O3).
In fact, the compiler is so sure the original and modified versions
are identical that building the following:
```
long long f1(long long lhs_val, long long rhs_val, bool *overflow) {
long long result = lhs_val * (1ll << (int)rhs_val);
*overflow = rhs_val > 0 && ((lhs_val >= 0 && result < lhs_val)
|| (lhs_val < 0 && result > lhs_val));
return result;
}
long long f2(long long lhs_val, long long rhs_val, bool *overflow) {
long long result = lhs_val << ((int)rhs_val);
*overflow = rhs_val > 0 && ((lhs_val >= 0 && result < lhs_val)
|| (lhs_val < 0 && result > lhs_val));
return result;
}
```
under `-Os` makes the body of f2 just say `jmp f1`!
Signed-off-by: Jeff Epler <jepler@gmail.com>
This diagnostic occurs when `nlr_push` is marked with the gcc/clang attribute "returns_twice". Signed-off-by: Jeff Epler <jepler@gmail.com>
This might affect codegen adversely in other cases, so the attribute is only enabled in the case where the address sanitizer is enabled. Availabilty of the standard gcc/clang attribute syntax is assumed when __SANITIZE_ADDRESS__ is defined. Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17709 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 171 171
Lines 22208 22209 +1
=======================================
+ Hits 21863 21864 +1
Misses 345 345 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Interestingly, I don't see these stack errors locally. under UBSan, this test and import/import_broken.py fail. Under asan, this failure comes up: If I can't resolve these, the "fixes" themselves are probably not worth taking either. Shifting to draft status. |
|
Code size report: |
|
See also #17735. |
|
Sorry @jepler, I missed that you'd opened this PR as well as the related issue (which I did see, hence the other PR). Your idea to also run the longlong build under ASan in CI seems worthwhile. |
|
If there's still a part of this PR that is not done by your #17735 then I'd be happy to revive this PR or start a fresh one. |
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.
Summary
I decided to try the longlong build under the sanitizers. This exposed a pair of problems:
Testing
I built the branch locally with the sanitizers until it passed.
Trade-offs and Alternatives
It's odd that the fixed version of left shift doesn't produce a diagnostic, because there is still an overflowing operation (now a multiplication, previously a shift). Is the sanitizer deliberately not checking this kind of overflow, or is recognizing a specific overflow check pattern in the following line? I was not able to find any documentation about this, and the documentation seemed to imply that -fsanitize=signed-integer-overflow was enabled by -fsanitize=undefined; explicitly enabling it made no difference.
It's unclear why adding the
returns_twicequalification tonlr_pushhelps the sanitizer to function properly, but it's true thatnlr_pushshould have this annotation. Hence, maybe it should be enabled whenever the compiler supports it? (any plausible gcc & clang versions, probably)