[BoundsSafety] Fix bug validating soft traps under debugger and then make lower_upper_check.c pass again - #13576
Merged
Conversation
…gger Soft traps (`-fbounds-safety-soft-traps=...`) are non-fatal: when a bounds check fails the compiler calls `__bounds_safety_soft_trap()`, which reports the violation and returns, and execution continues to a normal exit. The `expect_trap.py` "direct" runner (`run_direct_soft_trap`) already checks this end-to-end: it runs the binary to completion and requires both the soft-trap marker and a zero exit code. The debugger runner (`run_debugger`, used for `--use-debugger` configurations) did not. For a soft trap it stopped at the first `__bounds_safety_soft_trap`, validated the trap's location and message, and returned immediately. The process was left stopped and subsequently killed by `LLDBProcessContextManager` on teardown. As a result the debugger path only verified that a soft trap *fires* at the expected location; it never checked the property that actually distinguishes a soft trap from a hard trap, namely that the process resumes and exits cleanly. This patch teaches `run_debugger` to resume the process after a soft trap has been validated and to require a clean exit. A new `resume_and_expect_clean_exit()` helper calls `process.Continue()` (which runs synchronously because the debugger is created with `SetAsync(False)`) and then requires the process to reach `eStateExited` with an exit status of 0. If the process stops again or exits with a non-zero status the check fails and a backtrace is logged. This mirrors the clean-exit check already performed by the direct runner and by `expect_no_trap.py`'s `check_no_trap()`. Hard traps (`unique-traps` and `merged-traps`) are unaffected: for those, stopping at the trap is the entire check, so `run_debugger` returns as before without resuming. The soft-trap handling works for both older LLDB (which lacks the bounds-safety instrumentation-runtime plugin and relies on a manual breakpoint on `__bounds_safety_soft_trap`) and newer LLDB (which stops with `eStopReasonInstrumentation`). This assumes a single soft trap fires before the process exits, which is sufficient for the current tests. Supporting multiple soft traps per run would require supporting multiple verify expectations, which the harness does not yet do; until then a second stop is reported as a failure rather than silently ignored. Assisted-by: Claude Code rdar://183573672
…`lower_upper_check.c` test In optimized builds with soft traps, LLVM can inline `bad_read()` into `main()`, prove that the out-of-bounds access `local[-1]` is undefined behavior, and emit a hard trap (`brk`) on the path following the `__bounds_safety_soft_trap()` call. Building the test case and running it without `-fbounds-safety` (requires removing the `__bidi_indexable` to build) doesn't reproduce a trap, which implies `-fbounds-safety` is unintentionally causing the optimizer to add a trap. This is arguably a miscompile. A soft trap is meant to be non-fatal, so reporting the soft trap and then hard trapping anyway is undesirable. We are technically executing UB, so the optimizer is within its rights to do this, but we should investigate whether it's possible to avoid this miscompile. Doing that is tracked in rdar://183581715. `bidi_indexable/lower_upper_check.c` hits this in the `soft-traps` + `optimized` configuration. To keep the test meaningful while the compiler bug is open, this patch makes two changes: * Mark `bad_read()` in `lower_upper_check.c` with `__attribute__((noinline))`. Preventing inlining stops LLVM from proving the access is UB in this translation, so the test reliably passes and continues to provide coverage that soft-trap mode is non-fatal in optimized builds. * Add `bidi_indexable/lower_upper_check_inline_bad_read.c`, a variant that forces inlining with `__attribute__((always_inline))` and thereby reproduces the miscompile. It is marked `XFAIL: soft-traps && optimized`, so the testsuite passes again while still explicitly exercising and tracking the miscompile. Assisted-by: Claude Code rdar://183581715
Author
|
Supersedes #13526 |
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.
This PR consists of two commits:
[BoundsSafety] Verify soft-trapping tests exit cleanly under the debugger
Soft traps (
-fbounds-safety-soft-traps=...) are non-fatal: when a boundscheck fails the compiler calls
__bounds_safety_soft_trap(), which reports theviolation and returns, and execution continues to a normal exit. The
expect_trap.py"direct" runner (run_direct_soft_trap) already checks thisend-to-end: it runs the binary to completion and requires both the soft-trap
marker and a zero exit code.
The debugger runner (
run_debugger, used for--use-debuggerconfigurations)did not. For a soft trap it stopped at the first
__bounds_safety_soft_trap,validated the trap's location and message, and returned immediately. The
process was left stopped and subsequently killed by
LLDBProcessContextManageron teardown. As a result the debugger path onlyverified that a soft trap fires at the expected location; it never checked
the property that actually distinguishes a soft trap from a hard trap, namely
that the process resumes and exits cleanly.
This patch teaches
run_debuggerto resume the process after a soft trap hasbeen validated and to require a clean exit. A new
resume_and_expect_clean_exit()helper callsprocess.Continue()(which runssynchronously because the debugger is created with
SetAsync(False)) and thenrequires the process to reach
eStateExitedwith an exit status of 0. If theprocess stops again or exits with a non-zero status the check fails and a
backtrace is logged. This mirrors the clean-exit check already performed by the
direct runner and by
expect_no_trap.py'scheck_no_trap().Hard traps (
unique-trapsandmerged-traps) are unaffected: for those,stopping at the trap is the entire check, so
run_debuggerreturns as beforewithout resuming. The soft-trap handling works for both older LLDB (which lacks
the bounds-safety instrumentation-runtime plugin and relies on a manual
breakpoint on
__bounds_safety_soft_trap) and newer LLDB (which stops witheStopReasonInstrumentation).This assumes a single soft trap fires before the process exits, which is
sufficient for the current tests. Supporting multiple soft traps per run would
require supporting multiple verify expectations, which the harness does not yet
do; until then a second stop is reported as a failure rather than silently
ignored.
Assisted-by: Claude Code
rdar://183573672
[BoundsSafety] Work around a soft trap triggering a hard trap in the
lower_upper_check.ctestIn optimized builds with soft traps, LLVM can inline
bad_read()intomain(), prove that the out-of-bounds accesslocal[-1]is undefinedbehavior, and emit a hard trap (
brk) on the path following the__bounds_safety_soft_trap()call. Building the test case and runningit without
-fbounds-safety(requires removing the__bidi_indexabletobuild) doesn't reproduce a trap, which implies
-fbounds-safetyisunintentionally causing the optimizer to add a trap. This is arguably a
miscompile.
A soft trap is meant to be non-fatal, so reporting the soft trap and then
hard trapping anyway is undesirable. We are technically executing UB, so
the optimizer is within its rights to do this, but we should investigate
whether it's possible to avoid this miscompile. Doing that is tracked in
rdar://183581715.
bidi_indexable/lower_upper_check.chits this in thesoft-traps+optimizedconfiguration. To keep the test meaningful while the compiler bugis open, this patch makes two changes:
Mark
bad_read()inlower_upper_check.cwith__attribute__((noinline)).Preventing inlining stops LLVM from proving the access is UB in this
translation, so the test reliably passes and continues to provide coverage
that soft-trap mode is non-fatal in optimized builds.
Add
bidi_indexable/lower_upper_check_inline_bad_read.c, a variant thatforces inlining with
__attribute__((always_inline))and thereby reproducesthe miscompile. It is marked
XFAIL: soft-traps && optimized, so the testsuitepasses again while still explicitly exercising and tracking the miscompile.
Assisted-by: Claude Code
rdar://183581715