[ubsan] Assert that each check only has one SanitizerKind #122392
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
The `Checked` parameter of `CodeGenFunction::EmitCheck` is of type `ArrayRef<std::pair<llvm::Value *, SanitizerMask>>`. In the general case, SanitizerMask can denote that zero or more sanitizers are enabled, but I believe (from tests and inspecting the code) that `EmitCheck` assumes exactly one sanitizer enabled per SanitizerMask. This patch adds an assertion for this invariant. This is not intended to change the functionality of the code, but will make it easier for maintainers to reason about and extend the `EmitCheck` function.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Thurston Dang (thurstond) ChangesThe This is not intended to change the functionality of UBSan, but will make it easier for maintainers to reason about and extend the Full diff: https://github.com/llvm/llvm-project/pull/122392.diff 1 Files Affected:
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 1bad7a722da07a..792fe05025e393 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3603,6 +3603,8 @@ void CodeGenFunction::EmitCheck(
llvm::Value *TrapCond = nullptr;
bool NoMerge = false;
for (int i = 0, n = Checked.size(); i < n; ++i) {
+ assert(Checked[i].second.isPowerOf2());
+
llvm::Value *Check = Checked[i].first;
// -fsanitize-trap= overrides -fsanitize-recover=.
llvm::Value *&Cond =
|
vitalybuka
reviewed
Jan 10, 2025
| @@ -3603,6 +3603,8 @@ void CodeGenFunction::EmitCheck( | |||
| llvm::Value *TrapCond = nullptr; | |||
| bool NoMerge = false; | |||
| for (int i = 0, n = Checked.size(); i < n; ++i) { | |||
There was a problem hiding this comment.
Maybe instead switch SanitizerMask -> SanitizerOrdinal?
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Jan 10, 2025
The `Checked` parameter of `CodeGenFunction::EmitCheck` is of type `ArrayRef<std::pair<llvm::Value *, SanitizerMask>>`, which is overly generalized: SanitizerMask can denote that zero or more sanitizers are enabled, but `EmitCheck` requires that exactly one sanitizer is specified in the SanitizerMask (e.g., `SanitizeTrap.has(Checked[i].second)` enforces that). This patch replaces SanitizerMask with SanitizerOrdinal for `EmitCheck` and code that calls it transitively. This should not affect the behavior of UBSan, but it has the advantage that specifying the wrong number of sanitizers in `Checked[i].second` will be detected as a compile-time error, rather than a runtime assertion failure. Suggested by Vitaly in llvm#122392 as an alternative to adding an explicit runtime assertion that the SanitizerMask contains exactly one sanitizer.
thurstond
added a commit
that referenced
this pull request
Jan 10, 2025
…heck (exactly one sanitizer is required) (#122511) The `Checked` parameter of `CodeGenFunction::EmitCheck` is of type `ArrayRef<std::pair<llvm::Value *, SanitizerMask>>`, which is overly generalized: SanitizerMask can denote that zero or more sanitizers are enabled, but `EmitCheck` requires that exactly one sanitizer is specified in the SanitizerMask (e.g., `SanitizeTrap.has(Checked[i].second)` enforces that). This patch replaces SanitizerMask with SanitizerOrdinal in the `Checked` parameter of `EmitCheck` and code that transitively relies on it. This should not affect the behavior of UBSan, but it has the advantages that: - the code is clearer: it avoids ambiguity in EmitCheck about what to do if multiple bits are set - specifying the wrong number of sanitizers in `Checked[i].second` will be detected as a compile-time error, rather than a runtime assertion failure Suggested by Vitaly in #122392 as an alternative to adding an explicit runtime assertion that the SanitizerMask contains exactly one sanitizer.
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 10, 2025
…k for EmitCheck (exactly one sanitizer is required) (#122511) The `Checked` parameter of `CodeGenFunction::EmitCheck` is of type `ArrayRef<std::pair<llvm::Value *, SanitizerMask>>`, which is overly generalized: SanitizerMask can denote that zero or more sanitizers are enabled, but `EmitCheck` requires that exactly one sanitizer is specified in the SanitizerMask (e.g., `SanitizeTrap.has(Checked[i].second)` enforces that). This patch replaces SanitizerMask with SanitizerOrdinal in the `Checked` parameter of `EmitCheck` and code that transitively relies on it. This should not affect the behavior of UBSan, but it has the advantages that: - the code is clearer: it avoids ambiguity in EmitCheck about what to do if multiple bits are set - specifying the wrong number of sanitizers in `Checked[i].second` will be detected as a compile-time error, rather than a runtime assertion failure Suggested by Vitaly in llvm/llvm-project#122392 as an alternative to adding an explicit runtime assertion that the SanitizerMask contains exactly one sanitizer.
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
…heck (exactly one sanitizer is required) (llvm#122511) The `Checked` parameter of `CodeGenFunction::EmitCheck` is of type `ArrayRef<std::pair<llvm::Value *, SanitizerMask>>`, which is overly generalized: SanitizerMask can denote that zero or more sanitizers are enabled, but `EmitCheck` requires that exactly one sanitizer is specified in the SanitizerMask (e.g., `SanitizeTrap.has(Checked[i].second)` enforces that). This patch replaces SanitizerMask with SanitizerOrdinal in the `Checked` parameter of `EmitCheck` and code that transitively relies on it. This should not affect the behavior of UBSan, but it has the advantages that: - the code is clearer: it avoids ambiguity in EmitCheck about what to do if multiple bits are set - specifying the wrong number of sanitizers in `Checked[i].second` will be detected as a compile-time error, rather than a runtime assertion failure Suggested by Vitaly in llvm#122392 as an alternative to adding an explicit runtime assertion that the SanitizerMask contains exactly one sanitizer.
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.
The
Checkedparameter ofCodeGenFunction::EmitCheckis of typeArrayRef<std::pair<llvm::Value *, SanitizerMask>>. In the general case, SanitizerMask is used to denote that zero or more sanitizers are enabled, but I believe thatEmitCheckassumes there is exactly one sanitizer enabled per SanitizerMask (e.g.,SanitizeTrap.has(Checked[i].second)is called, whereby.haschecks that there is only one sanitizer enabled). This patch adds an assertion for this invariant.This is not intended to change the functionality of UBSan, but will make it easier for maintainers to reason about and extend the
EmitCheckfunction.