🍒 Move trap reasons for indexing to the new trap reasons infrastructure #11679
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
…sSafetyTrapCheck` This is the first step in adopting the new trap reasons infrastructure. This patch introduces the `trap_bs_fallback` diagnostic and this just uses the existing infrastructure for computing trap reason reasons. Thus there is no user visible change in behavior with this patch. Future patches will start introducing new diagnostics so more specific trap reason messages can be created. rdar://158623471 (cherry picked from commit 7159768)
…frastructure This patch moves generation of the trap reason strings for indexing into pointers into the new trap reason infrastructure. We will need to move many other trap reasons over to the new infrastructure but this is the first we are moving over. Previously array indexing emitted these very unspecifc trap reason messages: - `Dereferencing above bounds` - `Deferencing below bounds` Now we emit trap reasons that look like - `indexing above upper bound in '<expr>'` - `indexing below lower bound in '<expr>'` - `indexing overflows address space in '<expr>'` where `<expr>` is the ArraySubscriptExpr printed as a string (see test cases for example). There are several improvements here: 1. We say indexing rather than dereferencing which is more specific. 2. We emit a specific trap reason for address space overflow. Previously there was no distinction between the upper bound trap and the address space overflow trap. 3. We emit the textual representation of the ArraySubscriptExpr that triggered the bounds check failed. This makes the message very specific. This new approach to emitting trap reasons will likely increase the size of debug info. To give users control a new flag `-fbounds-safety-debug-trap-trap-reasons` has been added which is analogous to `-fsanitize-debug-trap-reasons` for UBSan. The flag takes three values: * `none` - Dont' emit any trap reasons * `detailed` - Emit the new more detailed trap reasons (the default) * `basic` - Emit the less descriptive trap reasons using the legacy infrastructure. While working on this it became clear that emission of trap diagnostics is more complicated than for UBSan become some of the context for where we are emitting the trap exists in a different stackframe than the location where we can actually emit the trap diagnostic. In `EmitWidePtrArraySubscriptExpr` we don't know if we are emitting a lower/upper bound/address space check. In `EmitBoundsSafetyBoundsCheck` we don't know we are emitting a check for an ArraySubscriptExpr so there isn't a function where we can emit the trap diagnostic that has all the necessary information. The solution used in this patch is to re-use the `PartialDiagnostic` class which essentially lets us partially construct a diagnostic in one function and then pass it along to another function to the actual where the trap diagnostic can be fully constructed. Note in this implementation the "detailed" trap reason is always constructed even if it later gets thrown away. There are several reasons for doing this: * For clang's diagnostics normally we typically don't write guards around them to check they are enabled (e.g. the warning might be actually disabled). * While technically we could write guards around all the code that builds the TrapReason objects this will become repetitive very quickly. It's cleaner to just put the guard in this function. * I'm also planning to use these TrapReason objects for the upcoming soft trap mode and I didn't want to put guards around their creation until I've figured out exactly how this is going to be implemented. * This is also how its implemented for UBSan's trapping diagnostics right now. This is not a particularly strong argument because I'm the one who implemented that but at least upstream didn't object to me doing it this way. rdar://158623471 (cherry picked from commit a4898c2)
…ap.test` When swiftlang#11621 (a4898c2) landed it broke this test because the trap reason string changed and this test wasn't updated. rdar://162886933 (cherry picked from commit 434475a)
delcypher
added
the
clang:bounds-safety
|
@swift-ci test |
|
@swit-ci test llvm |
|
@swift-ci test llvm |
|
@swift-ci Please test Windows platform |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
rdar://158623471