Skip to content

[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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+151 −4
Open

[Clang] Warn when std::atomic_thread_fence is used with fsanitize=thread #166542

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8b4ff4f
[Clang] Warn when `std::atomic_thread_fence` is used with `fsanitize=…
BStott6 Nov 5, 2025
2558549
Implement suggestions, fix handling of ignored functions, handle nosa…
BStott6 Nov 6, 2025
30835da
Fix formatting mistake
BStott6 Nov 6, 2025
8a1b662
Improve test, account for lambda attributes, use mangled name to look…
BStott6 Nov 7, 2025
10e30f0
Merge branch 'main' into warn-atomic-thread-fence-with-tsan
BStott6 Nov 26, 2025
df5a1ab
Apply suggestions
BStott6 Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1769,3 +1769,5 @@ def ExplicitSpecializationStorageClass : DiagGroup<"explicit-specialization-stor

// A warning for options that enable a feature that is not yet complete
def ExperimentalOption : DiagGroup<"experimental-option">;

def TSan : DiagGroup<"tsan">;
Copy link
Contributor

@gbMattN gbMattN Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this could be renamed to be more descriptive?
Address sanitizer on line 1599 has

// AddressSanitizer frontend instrumentation remarks.
def SanitizeAddressRemarks : DiagGroup<"sanitize-address">;

Copy link
Contributor Author

@BStott6 BStott6 Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string is "tsan" to match GCC, I agree about renaming the record though
How about ThreadSanitizerWarnings?

Copy link
Contributor

@gbMattN gbMattN Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, unless you can think of any reason not to, I think copying the ASan style exactly, and maybe moving your change to be next to it to logically group them together would be better. Keeping consistency with llvm over gcc seems like it makes sense.

Copy link
Collaborator

@AaronBallman AaronBallman Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is only needed for a single diagnostic, I would skip making a group entirely and just use an inline group in DiagnosticSemaKinds.td. As for the string we expose for the group name, I think consistency with GCC is more useful; the sanitize-address group is for controlling remarks rather than warnings anyway.

3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ def warn_max_unsigned_zero : Warning<
"%select{a value and unsigned zero|unsigned zero and a value}0 "
"is always equal to the other value">,
InGroup<MaxUnsignedZero>;
def warn_atomic_thread_fence_with_tsan : Warning<
"`std::atomic_thread_fence` is not supported with `-fsanitize=thread`">,
Copy link
Collaborator

@AaronBallman AaronBallman Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"`std::atomic_thread_fence` is not supported with `-fsanitize=thread`">,
"'std::atomic_thread_fence' is not supported with '-fsanitize=thread'">,

Using straight quote instead of backtick

InGroup<TSan>;
def note_remove_max_call : Note<
"remove call to max function and unsigned zero argument">;

Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3033,6 +3033,8 @@ class Sema final : public SemaBase {

void CheckMaxUnsignedZero(const CallExpr *Call, const FunctionDecl *FDecl);

void CheckUseOfAtomicThreadFenceWithTSan(const CallExpr *Call, const FunctionDecl *FDecl);

/// Check for dangerous or invalid arguments to memset().
///
/// This issues warnings on known problematic, dangerous or unspecified
Expand Down
34 changes: 34 additions & 0 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,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"
Expand Down Expand Up @@ -4100,6 +4101,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);
Expand Down Expand Up @@ -9822,6 +9824,38 @@ 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 positive. Example issue:
// https://github.com/llvm/llvm-project/issues/52942

if (!Call || !FDecl)
return;

if (!IsStdFunction(FDecl, "atomic_thread_fence"))
return;

// See if TSan is enabled in this function
const auto EnabledTSanMask =
Copy link
Contributor

@ojhunt ojhunt Nov 13, 2025

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 auto you should use the actual type. As I understand it this is the preferred model unless the RHS includes the target type explicitly (e.g X.getAsSomeType(), cast<SomeTYpe>, etc

Copy link
Contributor Author

@BStott6 BStott6 Nov 14, 2025

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

Context.getLangOpts().Sanitize.Mask & (SanitizerKind::Thread);
if (!EnabledTSanMask)
return;

const auto &NoSanitizeList = Context.getNoSanitizeList();
if (NoSanitizeList.containsLocation(EnabledTSanMask,
Call->getSourceRange().getBegin()))
// File is excluded
return;
if (NoSanitizeList.containsFunction(EnabledTSanMask,
FDecl->getQualifiedNameAsString()))
// Function is excluded
return;

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
Expand Down
11 changes: 11 additions & 0 deletions clang/test/SemaCXX/warn-tsan-atomic-fence.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// RUN: %clang -std=c++17 %s 2>&1 | FileCheck %s --check-prefix=NO-TSAN --allow-empty
// RUN: %clang -std=c++17 -fsanitize=thread %s 2>&1 | FileCheck %s --check-prefix=WITH-TSAN

// WITH-TSAN: `std::atomic_thread_fence` is not supported with `-fsanitize=thread`
// NO-TSAN-NOT: `std::atomic_thread_fence` is not supported with `-fsanitize=thread`
Copy link
Collaborator

@AaronBallman AaronBallman Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// RUN: %clang -std=c++17 %s 2>&1 | FileCheck %s --check-prefix=NO-TSAN --allow-empty
// RUN: %clang -std=c++17 -fsanitize=thread %s 2>&1 | FileCheck %s --check-prefix=WITH-TSAN
// WITH-TSAN: `std::atomic_thread_fence` is not supported with `-fsanitize=thread`
// NO-TSAN-NOT: `std::atomic_thread_fence` is not supported with `-fsanitize=thread`
// RUN: %clang -std=c++17 %s 2>&1 | FileCheck %s --check-prefix=NO-TSAN --allow-empty
// RUN: %clang -std=c++17 -fsanitize=thread %s 2>&1 | FileCheck %s --check-prefix=WITH-TSAN
// WITH-TSAN: `std::atomic_thread_fence` is not supported with `-fsanitize=thread`
// NO-TSAN-NOT: `std::atomic_thread_fence` is not supported with `-fsanitize=thread`

These should be running %clang_cc1 so we're not executing the driver and the frontend. Also, please use -verify instead of | FileCheck %s to verify the diagnostics appear on the expected line. You can use -verify= to give different RUN lines a different prefix so you can show that the diagnostic is only emitted for some RUN lines.


#include <atomic>
Copy link
Collaborator

@AaronBallman AaronBallman Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not include system headers; that makes the test non-hermetic. Instead, manually add just the declarations needed for the test case.

Some other test cases to add: use of no_sanitize("thread") and no_sanitize_thread attributes on the function using atomic_thread_fence.


int main() {
std::atomic_thread_fence(std::memory_order::memory_order_relaxed);
}
Copy link
Collaborator

@AaronBallman AaronBallman Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think other tests that would be interesting to try out would be:

__attribute__((no_sanitize("thread")))
void func() {
  auto lam = [](){
    std::atomic_thread_fence(std::memory_order_relaxed); // This should still diagnose, right?
  };
}

void other() {
  auto lam = []() __attribute__((no_sanitize("thread"))) {
    std::atomic_thread_fence(std::memory_order_relaxed); // This should not diagnose, right?
  };
}

and we probably should document that there can be false positives, e.g.,

inline void inline_func() {
  std::atomic_thread_fence(std::memory_order_relaxed); // Still diagnosed even though it's an inline function
}

__attribute__((no_sanitize("thread"))) void caller() {
  inline_func();
  if (0) {
    std::atomic_thread_fence(std::memory_order_relaxed); // Still diagnosed even though it's unreachable
  }
}

Copy link
Contributor Author

@BStott6 BStott6 Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lambda cases exposed another oversight in my code (it does not take into account the attributes applied to the lambda directly, fixed by also checking the attributes of getCurFunctionDecl(/*AllowLambdas*/ true)
Interestingly, GCC does not emit the warning in the if (0) case; it seems to do some reachability check - should I try to match this behaviour?

Copy link
Collaborator

@AaronBallman AaronBallman Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lambda cases exposed another oversight in my code (it does not take into account the attributes applied to the lambda directly, fixed by also checking the attributes of getCurFunctionDecl(/*AllowLambdas*/ true) Interestingly, GCC does not emit the warning in the if (0) case; it seems to do some reachability check - should I try to match this behaviour?

I'd say let's try to land this without any control flow analysis and see whether the false positive rate is acceptable or not. We do have CFG-based diagnostics, but they're off-by-default due to the compile time overhead.