-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Clang] Warn when std::atomic_thread_fence is used with fsanitize=thread
#166542
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
base: main
Are you sure you want to change the base?
Changes from all commits
8b4ff4f
2558549
30835da
8a1b662
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |
| #include "clang/AST/ExprObjC.h" | ||
| #include "clang/AST/FormatString.h" | ||
| #include "clang/AST/IgnoreExpr.h" | ||
| #include "clang/AST/Mangle.h" | ||
| #include "clang/AST/NSAPI.h" | ||
| #include "clang/AST/NonTrivialTypeVisitor.h" | ||
| #include "clang/AST/OperationKinds.h" | ||
|
|
@@ -45,6 +46,7 @@ | |
| #include "clang/Basic/IdentifierTable.h" | ||
| #include "clang/Basic/LLVM.h" | ||
| #include "clang/Basic/LangOptions.h" | ||
| #include "clang/Basic/NoSanitizeList.h" | ||
| #include "clang/Basic/OpenCLOptions.h" | ||
| #include "clang/Basic/OperatorKinds.h" | ||
| #include "clang/Basic/PartialDiagnostic.h" | ||
|
|
@@ -4100,6 +4102,7 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall, | |
| CheckAbsoluteValueFunction(TheCall, FDecl); | ||
| CheckMaxUnsignedZero(TheCall, FDecl); | ||
| CheckInfNaNFunction(TheCall, FDecl); | ||
| CheckUseOfAtomicThreadFenceWithTSan(TheCall, FDecl); | ||
|
|
||
| if (getLangOpts().ObjC) | ||
| ObjC().DiagnoseCStringFormatDirectiveInCFAPI(FDecl, Args, NumArgs); | ||
|
|
@@ -9822,6 +9825,65 @@ void Sema::CheckMaxUnsignedZero(const CallExpr *Call, | |
| << FixItHint::CreateRemoval(RemovalRange); | ||
| } | ||
|
|
||
| //===--- CHECK: Warn on use of `std::atomic_thread_fence` with TSan. ------===// | ||
| void Sema::CheckUseOfAtomicThreadFenceWithTSan(const CallExpr *Call, | ||
| const FunctionDecl *FDecl) { | ||
| // Thread sanitizer currently does not support `std::atomic_thread_fence`, | ||
| // leading to false positives. Example issue: | ||
| // https://github.com/llvm/llvm-project/issues/52942 | ||
|
|
||
| if (!Call || !FDecl) | ||
| return; | ||
|
|
||
| if (!IsStdFunction(FDecl, "atomic_thread_fence")) | ||
| return; | ||
|
|
||
| // Check that TSan is enabled in this context | ||
| const auto EnabledTSanMask = | ||
| Context.getLangOpts().Sanitize.Mask & (SanitizerKind::Thread); | ||
| if (!EnabledTSanMask) | ||
| return; | ||
|
|
||
| // Check that the file isn't in the no-sanitize list | ||
| const auto &NoSanitizeList = Context.getNoSanitizeList(); | ||
| if (NoSanitizeList.containsLocation(EnabledTSanMask, | ||
| Call->getSourceRange().getBegin())) | ||
| return; | ||
|
|
||
| std::unique_ptr<MangleContext> MC(Context.createMangleContext()); | ||
|
|
||
| // Check that the calling function or lambda: | ||
| // - Does not have any attributes preventing TSan checking | ||
| // - Is not in the ignore list | ||
| auto IsNotSanitized = [&](NamedDecl *Decl) { | ||
| const auto SpecificAttrs = Decl->specific_attrs<NoSanitizeAttr>(); | ||
| const auto IsNoSanitizeThreadAttr = [](NoSanitizeAttr *Attr) { | ||
| return static_cast<bool>(Attr->getMask() & SanitizerKind::Thread); | ||
| }; | ||
|
|
||
| // Get mangled name for ignorelist lookup | ||
| std::string MangledName; | ||
| if (MC->shouldMangleDeclName(Decl)) { | ||
| llvm::raw_string_ostream S = llvm::raw_string_ostream(MangledName); | ||
| MC->mangleName(Decl, S); | ||
| } else { | ||
| MangledName = Decl->getName(); | ||
| } | ||
|
|
||
| return Decl && | ||
| (Decl->hasAttr<DisableSanitizerInstrumentationAttr>() || | ||
| std::any_of(SpecificAttrs.begin(), SpecificAttrs.end(), | ||
| IsNoSanitizeThreadAttr) || | ||
| NoSanitizeList.containsFunction(EnabledTSanMask, MangledName)); | ||
| }; | ||
| if (IsNotSanitized(getCurFunctionOrMethodDecl())) | ||
| return; | ||
| if (IsNotSanitized(getCurFunctionDecl(/*AllowLambdas*/ true))) | ||
| return; | ||
|
Comment on lines
+9879
to
+9882
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than this, you should just update and - NamedDecl *Sema::getCurFunctionOrMethodDecl() const {
- DeclContext *DC = getFunctionLevelDeclContext();
+ NamedDecl *Sema::getCurFunctionOrMethodDecl(bool AllowsLambdas) const {
+ DeclContext *DC = getFunctionLevelDeclContext(AllowsLambdas);Then this simply turns into a single if (!IsNotSanitized(getCurFunctionOrMethodDecl(/*AllowsLambdas=*/true))
return;Which also saves duplicate walks of the context
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like a good improvement, I'll do it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ojhunt I think we actually do need to check with and without lambdas separately, due to the case of a non-ignored lambda in an ignored function. I think one would expect the warning to be ignored as the enclosing function is ignored, but if we only check once and include lambdas, it won't pick up the function attribute. We can only do the "AllowLambda = false" check if the first check returns a lambda |
||
|
|
||
| Diag(Call->getExprLoc(), diag::warn_atomic_thread_fence_with_tsan); | ||
| } | ||
|
|
||
| //===--- CHECK: Standard memory functions ---------------------------------===// | ||
|
|
||
| /// Takes the expression passed to the size_t parameter of functions | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| // No warnings in regular compile | ||
| // RUN: %clang_cc1 -verify=no-tsan %s | ||
|
|
||
| // Emits warning with `-fsanitize=thread` | ||
| // RUN: %clang_cc1 -verify=with-tsan -fsanitize=thread %s | ||
|
|
||
| // No warnings if `-Wno-tsan` is passed | ||
| // RUN: %clang_cc1 -verify=no-tsan -fsanitize=thread -Wno-tsan %s | ||
|
|
||
| // Ignoring func1 | ||
| // RUN: echo "fun:*func1*" > %t | ||
| // RUN: %clang_cc1 -verify=no-tsan -fsanitize=thread -fsanitize-ignorelist=%t %s | ||
|
|
||
| // Ignoring source file | ||
| // RUN: echo "src:%s" > %t | ||
| // RUN: %clang_cc1 -verify=no-tsan -fsanitize=thread -fsanitize-ignorelist=%t %s | ||
|
|
||
| // no-tsan-no-diagnostics | ||
|
|
||
| namespace std { | ||
| enum memory_order { | ||
| memory_order_relaxed, | ||
| memory_order_consume, | ||
| memory_order_acquire, | ||
| memory_order_release, | ||
| memory_order_acq_rel, | ||
| memory_order_seq_cst, | ||
| }; | ||
| void atomic_thread_fence(memory_order) {} | ||
| }; | ||
|
|
||
| void func1() { // extern "C" to stop name mangling | ||
| std::atomic_thread_fence(std::memory_order_relaxed); // with-tsan-warning {{'std::atomic_thread_fence' is not supported with '-fsanitize=thread'}} | ||
|
|
||
| auto lam = []() __attribute__((no_sanitize("thread"))) { | ||
| std::atomic_thread_fence(std::memory_order_relaxed); | ||
| }; | ||
| } | ||
|
|
||
| __attribute__((no_sanitize("thread"))) | ||
| void func2() { | ||
| std::atomic_thread_fence(std::memory_order_relaxed); | ||
|
|
||
| auto lam = []() { | ||
| std::atomic_thread_fence(std::memory_order_relaxed); | ||
| }; | ||
| } | ||
|
|
||
| __attribute__((no_sanitize_thread)) | ||
| void func3() { | ||
| std::atomic_thread_fence(std::memory_order_relaxed); | ||
| } | ||
|
|
||
| int main() {} |
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.
Sorry this is a pedant/nit - rather than
autoyou should use the actual type. As I understand it this is the preferred model unless the RHS includes the target type explicitly (e.gX.getAsSomeType(),cast<SomeTYpe>, etcThere 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.
Thanks, I'm quite new to LLVM so code style tips like this are very appreciated, I will change it