-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[UBSan] Move type:*=sanitize handling. #142006
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
[UBSan] Move type:*=sanitize handling. #142006
Conversation
Created using spr 1.3.6
|
@llvm/pr-subscribers-clang Author: Qinkun Bao (qinkunbao) ChangesFull diff: https://github.com/llvm/llvm-project/pull/142006.diff 2 Files Affected:
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index e71928ec0dc1c..5044d7c33ec3c 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -875,8 +875,7 @@ ASTContext::insertCanonicalTemplateTemplateParmDeclInternal(
bool ASTContext::isTypeIgnoredBySanitizer(const SanitizerMask &Mask,
const QualType &Ty) const {
std::string TyName = Ty.getUnqualifiedType().getAsString(getPrintingPolicy());
- return NoSanitizeL->containsType(Mask, TyName) &&
- !NoSanitizeL->containsType(Mask, TyName, "sanitize");
+ return NoSanitizeL->containsType(Mask, TyName);
}
TargetCXXABI::Kind ASTContext::getCXXABIKind() const {
diff --git a/clang/lib/Basic/NoSanitizeList.cpp b/clang/lib/Basic/NoSanitizeList.cpp
index a3ca463fc8efb..61ee19555c0ca 100644
--- a/clang/lib/Basic/NoSanitizeList.cpp
+++ b/clang/lib/Basic/NoSanitizeList.cpp
@@ -34,7 +34,11 @@ bool NoSanitizeList::containsGlobal(SanitizerMask Mask, StringRef GlobalName,
bool NoSanitizeList::containsType(SanitizerMask Mask, StringRef MangledTypeName,
StringRef Category) const {
- return SSCL->inSection(Mask, "type", MangledTypeName, Category);
+ auto NoSan = SSCL->inSectionBlame(Mask, "type", FileName, Category);
+ if (NoSan == llvm::SpecialCaseList::NotFound)
+ return false;
+ auto San = SSCL->inSectionBlame(Mask, "type", FileName, "sanitize");
+ return San == llvm::SpecialCaseList::NotFound || NoSan > San;
}
bool NoSanitizeList::containsFunction(SanitizerMask Mask,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR centralizes type sanitization logic by moving handling of the sanitize category into NoSanitizeList::containsType and simplifying the corresponding check in ASTContext.
- Relocates and extends sanitizer checks in
NoSanitizeList::containsTypeusinginSectionBlameand ordering comparison. - Updates
ASTContext::isTypeIgnoredBySanitizerto a single call to the revisedcontainsType.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| clang/lib/Basic/NoSanitizeList.cpp | Refactored containsType to use inSectionBlame and compare sections |
| clang/lib/AST/ASTContext.cpp | Simplified isTypeIgnoredBySanitizer to delegate to containsType |
Comments suppressed due to low confidence (2)
clang/lib/Basic/NoSanitizeList.cpp:37
- [nitpick] Variable names
NoSanandSanare terse and may be unclear; consider more descriptive names likenoSanitizePosandsanitizePosto improve readability.
auto NoSan = SSCL->inSectionBlame(Mask, "type", FileName, Category);
clang/lib/Basic/NoSanitizeList.cpp:35
- The updated logic in
containsType, including the new sanitizer ordering behavior, should be covered by unit tests to validate correct handling of presence and ordering across different sections.
bool NoSanitizeList::containsType(SanitizerMask Mask, StringRef MangledTypeName,
Created using spr 1.3.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors sanitizer handling by moving type-specific “sanitize” checks into NoSanitizeList::containsType and simplifying ASTContext::isTypeIgnoredBySanitizer to delegate fully to that method.
- Update
containsTypeto use blame indices for combined “type” and “sanitize” sections - Remove redundant checks in
ASTContext::isTypeIgnoredBySanitizerto rely on the new combined logic
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| clang/lib/Basic/NoSanitizeList.cpp | Implement combined sanitizer handling in containsType using inSectionBlame |
| clang/lib/AST/ASTContext.cpp | Simplify isTypeIgnoredBySanitizer to delegate to the updated containsType |
Comments suppressed due to low confidence (2)
clang/lib/Basic/NoSanitizeList.cpp:34
- Consider adding unit tests for
containsTypecovering both cases where the type is whitelisted, blacklisted, or overridden by thesanitizesection to ensure the new logic behaves as expected.
bool NoSanitizeList::containsType(SanitizerMask Mask, StringRef MangledTypeName,
clang/lib/AST/ASTContext.cpp:878
- The
containsTypemethod signature expects a thirdCategoryargument. Either provide a default value in the declaration or update this call to pass the appropriate category string.
return NoSanitizeL->containsType(Mask, TyName);
Created using spr 1.3.6
|
It does not let to add contributors into reviewers so just CC @JustinStitt |
| bool NoSanitizeList::containsType(SanitizerMask Mask, StringRef MangledTypeName, | ||
| StringRef Category) const { | ||
| return SSCL->inSection(Mask, "type", MangledTypeName, Category); | ||
| auto NoSan = SSCL->inSectionBlame(Mask, "type", MangledTypeName, Category); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this code pattern can be extracted into private helper and used across other prefixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had the same thought when I copied the code from "containsType" to "containsFile".
I am going to publish a followup PR to address the issue.
thanks for CC. taking a look. |
JustinStitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (but I don't have official powers to Approve).
| if (NoSan == llvm::SpecialCaseList::NotFound) | ||
| return false; | ||
| auto San = SSCL->inSectionBlame(Mask, "type", MangledTypeName, "sanitize"); | ||
| return San == llvm::SpecialCaseList::NotFound || NoSan > San; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good to me. Only quirk is NoSan > San. The autos slightly obscure the fact these are pair types. Thankfully, the FileIdx portion of the pair is properly mapped left-to-right alongside all the -fsanitize-ignorelist= arguments within SpecialCaseList::createInternal. This means we'll get the obvious behavior of most recent SCL file which is then further delineated by line number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…#142027) See #142006 and #139128 --------- Co-authored-by: Vitaly Buka <[email protected]>
…ted logics. (#142027) See llvm/llvm-project#142006 and llvm/llvm-project#139128 --------- Co-authored-by: Vitaly Buka <[email protected]>
As discussed in #139128, this PR moves =sanitize handling from
ASTContext::isTypeIgnoredBySanitizertoNoSanitizeList::containsType.Before this PR: "=sanitize" has priority regardless of the order
After this PR: If multiple entries match the source, than the latest entry takes the precedence.