[Sanitizers] the access size (8 bytes) exceeds the max lock-free size (4 bytes) for 32-bit #125388
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-compiler-rt-sanitizer @llvm/pr-subscribers-clang Author: None (honeygoyal) ChangesDescription[Atomic] the access size (8 bytes) exceeds the max lock-free size (4 bytes) for 32-bit.
Driver Test Case (AIX.cpp) - Full diff: https://github.com/llvm/llvm-project/pull/125388.diff 3 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/AIX.cpp b/clang/lib/Driver/ToolChains/AIX.cpp
index 09a8dc2f4fa5dd..493983468b0bcf 100644
--- a/clang/lib/Driver/ToolChains/AIX.cpp
+++ b/clang/lib/Driver/ToolChains/AIX.cpp
@@ -232,7 +232,7 @@ void aix::Linker::ConstructJob(Compilation &C, const JobAction &JA,
// Specify linker input file(s).
AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs, JA);
-
+ const SanitizerArgs &Sanitize = ToolChain.getSanitizerArgs(Args);
if (D.isUsingLTO()) {
assert(!Inputs.empty() && "Must have at least one input.");
// Find the first filename InputInfo object.
@@ -338,6 +338,10 @@ void aix::Linker::ConstructJob(Compilation &C, const JobAction &JA,
CmdArgs.push_back("-lpthread");
}
const char *Exec = Args.MakeArgString(ToolChain.GetLinkerPath());
+
+ if (Sanitize.hasAnySanitizer() && IsArch32Bit) {
+ CmdArgs.push_back("-latomic");
+ }
C.addCommand(std::make_unique<Command>(JA, *this, ResponseFileSupport::None(),
Exec, CmdArgs, Inputs, Output));
}
diff --git a/clang/test/Driver/aix-ld.c b/clang/test/Driver/aix-ld.c
index 7e0f2bf91e06ee..de65597bcf5d91 100644
--- a/clang/test/Driver/aix-ld.c
+++ b/clang/test/Driver/aix-ld.c
@@ -1120,3 +1120,21 @@
// RUN: -c \
// RUN: | FileCheck --check-prefixes=CHECK-K-UNUSED %s
// CHECK-K-UNUSED: clang: warning: -K: 'linker' input unused [-Wunused-command-line-argument]
+
+// Check No Sanitizer on 32-bit AIX
+// RUN: %if target={{.*aix.*}} %{ \
+// RUN: %clang -target powerpc-ibm-aix -m32 %s -### 2>&1 \
+// RUN: | FileCheck -check-prefix=CHECK-LD32-NO-SANITIZER %s \
+// RUN: %}
+// %if target={{.*aix.*}} %{
+// CHECK-LD32-NO-SANITIZER-NOT: "-latomic"
+// %}
+
+// Check enable AddressSanitizer on 32-bit AIX
+// RUN: %if target={{.*aix.*}} %{ \
+// RUN: %clang -target powerpc-ibm-aix -m32 -fsanitize=address %s -### 2>&1 \
+// RUN: | FileCheck -check-prefix=CHECK-LD32-ASAN %s \
+// RUN: %}
+// %if target={{.*aix.*}} %{
+// CHECK-LD32-ASAN: "-latomic"
+// %}
diff --git a/compiler-rt/lib/sanitizer_common/CMakeLists.txt b/compiler-rt/lib/sanitizer_common/CMakeLists.txt
index 09391e4f5f3704..79eeb31c035a28 100644
--- a/compiler-rt/lib/sanitizer_common/CMakeLists.txt
+++ b/compiler-rt/lib/sanitizer_common/CMakeLists.txt
@@ -239,6 +239,9 @@ append_list_if(SANITIZER_LIMIT_FRAME_SIZE -Wframe-larger-than=570
append_list_if(COMPILER_RT_HAS_WGLOBAL_CONSTRUCTORS_FLAG -Wglobal-constructors
SANITIZER_CFLAGS)
+# Suppress -Watomic-alignment warnings by not treating them as errors
+list(APPEND SANITIZER_CFLAGS "-Wno-error=atomic-alignment")
+
if(APPLE)
set(OS_OPTION OS ${SANITIZER_COMMON_SUPPORTED_OS})
endif()
|
|
@llvm/pr-subscribers-backend-powerpc Author: None (honeygoyal) ChangesDescription[Atomic] the access size (8 bytes) exceeds the max lock-free size (4 bytes) for 32-bit.
Driver Test Case (AIX.cpp) - Full diff: https://github.com/llvm/llvm-project/pull/125388.diff 3 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/AIX.cpp b/clang/lib/Driver/ToolChains/AIX.cpp
index 09a8dc2f4fa5dd8..493983468b0bcf9 100644
--- a/clang/lib/Driver/ToolChains/AIX.cpp
+++ b/clang/lib/Driver/ToolChains/AIX.cpp
@@ -232,7 +232,7 @@ void aix::Linker::ConstructJob(Compilation &C, const JobAction &JA,
// Specify linker input file(s).
AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs, JA);
-
+ const SanitizerArgs &Sanitize = ToolChain.getSanitizerArgs(Args);
if (D.isUsingLTO()) {
assert(!Inputs.empty() && "Must have at least one input.");
// Find the first filename InputInfo object.
@@ -338,6 +338,10 @@ void aix::Linker::ConstructJob(Compilation &C, const JobAction &JA,
CmdArgs.push_back("-lpthread");
}
const char *Exec = Args.MakeArgString(ToolChain.GetLinkerPath());
+
+ if (Sanitize.hasAnySanitizer() && IsArch32Bit) {
+ CmdArgs.push_back("-latomic");
+ }
C.addCommand(std::make_unique<Command>(JA, *this, ResponseFileSupport::None(),
Exec, CmdArgs, Inputs, Output));
}
diff --git a/clang/test/Driver/aix-ld.c b/clang/test/Driver/aix-ld.c
index 7e0f2bf91e06ee5..de65597bcf5d914 100644
--- a/clang/test/Driver/aix-ld.c
+++ b/clang/test/Driver/aix-ld.c
@@ -1120,3 +1120,21 @@
// RUN: -c \
// RUN: | FileCheck --check-prefixes=CHECK-K-UNUSED %s
// CHECK-K-UNUSED: clang: warning: -K: 'linker' input unused [-Wunused-command-line-argument]
+
+// Check No Sanitizer on 32-bit AIX
+// RUN: %if target={{.*aix.*}} %{ \
+// RUN: %clang -target powerpc-ibm-aix -m32 %s -### 2>&1 \
+// RUN: | FileCheck -check-prefix=CHECK-LD32-NO-SANITIZER %s \
+// RUN: %}
+// %if target={{.*aix.*}} %{
+// CHECK-LD32-NO-SANITIZER-NOT: "-latomic"
+// %}
+
+// Check enable AddressSanitizer on 32-bit AIX
+// RUN: %if target={{.*aix.*}} %{ \
+// RUN: %clang -target powerpc-ibm-aix -m32 -fsanitize=address %s -### 2>&1 \
+// RUN: | FileCheck -check-prefix=CHECK-LD32-ASAN %s \
+// RUN: %}
+// %if target={{.*aix.*}} %{
+// CHECK-LD32-ASAN: "-latomic"
+// %}
diff --git a/compiler-rt/lib/sanitizer_common/CMakeLists.txt b/compiler-rt/lib/sanitizer_common/CMakeLists.txt
index 09391e4f5f3704b..79eeb31c035a282 100644
--- a/compiler-rt/lib/sanitizer_common/CMakeLists.txt
+++ b/compiler-rt/lib/sanitizer_common/CMakeLists.txt
@@ -239,6 +239,9 @@ append_list_if(SANITIZER_LIMIT_FRAME_SIZE -Wframe-larger-than=570
append_list_if(COMPILER_RT_HAS_WGLOBAL_CONSTRUCTORS_FLAG -Wglobal-constructors
SANITIZER_CFLAGS)
+# Suppress -Watomic-alignment warnings by not treating them as errors
+list(APPEND SANITIZER_CFLAGS "-Wno-error=atomic-alignment")
+
if(APPLE)
set(OS_OPTION OS ${SANITIZER_COMMON_SUPPORTED_OS})
endif()
|
w2yehia
changed the title
Co-authored-by: Hubert Tong <hubert.reinterpretcast@gmail.com>
Co-authored-by: Hubert Tong <hubert.reinterpretcast@gmail.com>
clang/test/Driver/aix-ld.c
Outdated
|
|
||
| // This check is only applicable to AIX targets. | ||
| // AIX-specific link behavior requires `-latomic` for 32-bit sanitizer libraries, | ||
| // Running this test on non-AIX targets will result in an unrelated error |
There was a problem hiding this comment.
I don't think that having this test (32-bit with no sanitizer) fail when using non-AIX environments/configurations is acceptable. This is basically the same as the first test in the file (CHECK-LD32). If this test is indeed failing, then please identify what the key difference is between this and the CHECK-LD32 test.
Additionally, "on non-AIX targets" is not correct. Is it "on non-AIX hosts" or "with non-AIX target configurations"?
Co-authored-by: Hubert Tong <hubert.reinterpretcast@gmail.com>
Co-authored-by: Wael Yehia <wmyehia2001@yahoo.com>
| // (e.g., sanitizer_common's atomic utilities). On 32-bit AIX, these | ||
| // are not natively supported, necessitating linkage with -latomic. | ||
| if (Sanitize.hasAnySanitizer() && IsArch32Bit) { | ||
| CmdArgs.push_back("-latomic"); |
There was a problem hiding this comment.
Please consider adding the code to clang::tools::driver::linkSanitizerRuntimeDeps (as was done for AIX sanitizer enablement in IBM's downstream) and adding the tests to clang/test/Driver/sanitizer-ld.c.
There was a problem hiding this comment.
Thank you for the suggestion, @hubert-reinterpretcast. After careful consideration, I believe that keeping the -latomic handling directly in AIX.cpp is the more appropriate approach for the following reasons:
- Target-Specific Clarity:
The logic for appending -latomic is very specific to the 32-bit AIX environment. By keeping it in AIX.cpp, we maintain a clear separation between platform-specific behavior and the more general sanitizer runtime dependency logic. This makes it immediately obvious to developers reviewing AIX-specific code that this flag is required solely for this target.
There was a problem hiding this comment.
2. Consistency with Community LLVM Behavior:
Our implementation in AIX.cpp follows the established behavior in the LLVM community codebase. While IBM’s downstream has opted to centralize similar logic in clang::tools::driver::linkSanitizerRuntimeDeps, our approach isolates the AIX-specific behavior. This minimizes the risk of inadvertently impacting sanitizer handling on non-AIX targets.
There was a problem hiding this comment.
3. Testing and Maintainability: The tests in clang/test/Driver/aix-ld.c are designed around this implementation. Moving the logic would complicate our test setup and risk unintended side effects.
There was a problem hiding this comment.
while I appreciate the suggestion and understand the merits of aligning with IBM’s downstream practices, I believe that maintaining the target-specific logic in AIX.cpp is the best course for now. This decision is based on ensuring clarity, minimizing risk, and preserving consistency with the community’s expected behavior.
Please let me know if there are additional points to consider.
There was a problem hiding this comment.
Please let me know if there are additional points to consider.
The addition of -latomic is not actually required even on AIX if the only sanitizer runtimes that are linked in are purely shared libraries.
The addition of -latomic for 32-bit AIX belongs in clang::tools::driver::linkSanitizerRuntimeDeps because the wider logic is already present to call that function only when sanitizer runtimes that contain static components are linked in.
See
llvm-project/clang/lib/Driver/ToolChains/Gnu.cpp
Lines 585 to 586 in f729477
llvm-project/clang/lib/Driver/ToolChains/CommonArgs.cpp
Lines 1607 to 1608 in f729477
There was a problem hiding this comment.
re: https://github.com/llvm/llvm-project/pull/125388/files#r1945365070:
By keeping it in AIX.cpp, we maintain a clear separation between platform-specific behavior and the more general sanitizer runtime dependency logic.
In this case, the actual result would have been unintentional platform-specific behaviour.
There was a problem hiding this comment.
re: https://github.com/llvm/llvm-project/pull/125388/files#r1945365836:
Our implementation in AIX.cpp follows the established behavior in the LLVM community codebase.
Ignoring the lack of elaboration on how exactly placing this logic in AIX.cpp "follows the established behavior in the LLVM community codebase", the statement ignores the fact that the majority of linkSanitizerRuntimeDeps is code that is conditional based on platforms. For example:
llvm-project/clang/lib/Driver/ToolChains/CommonArgs.cpp
Lines 1452 to 1454 in f729477
While IBM’s downstream has opted to centralize similar logic in clang::tools::driver::linkSanitizerRuntimeDeps
The characterization that it is "IBM's downstream" that opted to centralize the logic in said function is misleading. The LLVM community had created said function for logic associated with linking in dependencies for static sanitizer components; IBM's downstream followed with that.
There was a problem hiding this comment.
This minimizes the risk of inadvertently impacting sanitizer handling on non-AIX targets.
Generally, placing the code in a more widely used path will lead to more likely detection of problems. In other words, minimizing the risk of inadvertently impacting sanitizer handling on non-AIX targets increases the risk of having broken sanitizer handling on AIX. Considering that the risk of breaking non-AIX should be low (especially in an undetected manner) and that the risk of incorrect sanitizer handling on AIX is high during active development, I think deliberate attempts to isolate the AIX code (as opposed to using common code paths) is a mistake.
Co-authored-by: Hubert Tong <hubert.reinterpretcast@gmail.com>
Co-authored-by: Hubert Tong <hubert.reinterpretcast@gmail.com>
There was a problem hiding this comment.
@honeygoyal, I do not believe that the prerequisite changes from IBM's downstream are present to support the Clang driver test being added in this PR.
| // (e.g., sanitizer_common's atomic utilities). On 32-bit AIX, these | ||
| // are not natively supported, necessitating linkage with -latomic. | ||
| if (Sanitize.hasAnySanitizer() && IsArch32Bit) { | ||
| CmdArgs.push_back("-latomic"); |
There was a problem hiding this comment.
Please let me know if there are additional points to consider.
The addition of -latomic is not actually required even on AIX if the only sanitizer runtimes that are linked in are purely shared libraries.
The addition of -latomic for 32-bit AIX belongs in clang::tools::driver::linkSanitizerRuntimeDeps because the wider logic is already present to call that function only when sanitizer runtimes that contain static components are linked in.
See
llvm-project/clang/lib/Driver/ToolChains/Gnu.cpp
Lines 585 to 586 in f729477
llvm-project/clang/lib/Driver/ToolChains/CommonArgs.cpp
Lines 1607 to 1608 in f729477
| // (e.g., sanitizer_common's atomic utilities). On 32-bit AIX, these | ||
| // are not natively supported, necessitating linkage with -latomic. | ||
| if (Sanitize.hasAnySanitizer() && IsArch32Bit) { | ||
| CmdArgs.push_back("-latomic"); |
There was a problem hiding this comment.
re: https://github.com/llvm/llvm-project/pull/125388/files#r1945365070:
By keeping it in AIX.cpp, we maintain a clear separation between platform-specific behavior and the more general sanitizer runtime dependency logic.
In this case, the actual result would have been unintentional platform-specific behaviour.
| // (e.g., sanitizer_common's atomic utilities). On 32-bit AIX, these | ||
| // are not natively supported, necessitating linkage with -latomic. | ||
| if (Sanitize.hasAnySanitizer() && IsArch32Bit) { | ||
| CmdArgs.push_back("-latomic"); |
There was a problem hiding this comment.
re: https://github.com/llvm/llvm-project/pull/125388/files#r1945365836:
Our implementation in AIX.cpp follows the established behavior in the LLVM community codebase.
Ignoring the lack of elaboration on how exactly placing this logic in AIX.cpp "follows the established behavior in the LLVM community codebase", the statement ignores the fact that the majority of linkSanitizerRuntimeDeps is code that is conditional based on platforms. For example:
llvm-project/clang/lib/Driver/ToolChains/CommonArgs.cpp
Lines 1452 to 1454 in f729477
While IBM’s downstream has opted to centralize similar logic in clang::tools::driver::linkSanitizerRuntimeDeps
The characterization that it is "IBM's downstream" that opted to centralize the logic in said function is misleading. The LLVM community had created said function for logic associated with linking in dependencies for static sanitizer components; IBM's downstream followed with that.
| // (e.g., sanitizer_common's atomic utilities). On 32-bit AIX, these | ||
| // are not natively supported, necessitating linkage with -latomic. | ||
| if (Sanitize.hasAnySanitizer() && IsArch32Bit) { | ||
| CmdArgs.push_back("-latomic"); |
There was a problem hiding this comment.
This minimizes the risk of inadvertently impacting sanitizer handling on non-AIX targets.
Generally, placing the code in a more widely used path will lead to more likely detection of problems. In other words, minimizing the risk of inadvertently impacting sanitizer handling on non-AIX targets increases the risk of having broken sanitizer handling on AIX. Considering that the risk of breaking non-AIX should be low (especially in an undetected manner) and that the risk of incorrect sanitizer handling on AIX is high during active development, I think deliberate attempts to isolate the AIX code (as opposed to using common code paths) is a mistake.
|
|
||
| // Check No Sanitizer on 32-bit AIX | ||
| // This test verifies that the linker doesn't include '-latomic' when no sanitizers are enabled | ||
| // RUN: %clang -target powerpc-ibm-aix -m32 %s -### 2>&1 \ |
There was a problem hiding this comment.
-target has been deprecated for a long time though without a warning. Use --target= for new tests
| // Check enable AddressSanitizer on 32-bit AIX | ||
| // FIXME: Running this test on non-AIX hosts will result in the following error: | ||
| // LLVM ERROR: Sanitizer interface functions must be exported by export files on AIX | ||
| // RUN: %if target={{.*aix.*}} %{ \ |
There was a problem hiding this comment.
Where is this diagnostic Sanitizer interface functions must be exported by export files on AIX defined and why is there a host difference? I am concerned this would cause friction for people improve the generic interface to inadvertently cause regression on AIX, since most contributors don't have access to AIX.
There was a problem hiding this comment.
Where is this diagnostic
Sanitizer interface functions must be exported by export files on AIXdefined and why is there a host difference?
It is strictly in IBM's downstream at this time. It will likely need revisiting before upstreaming. I have requested the removal of the driver part from this PR.
Description
[Atomic] the access size (8 bytes) exceeds the max lock-free size (4 bytes) for 32-bit.
Driver Test Case (AIX.cpp) -
Test Case 1: Sanitizer enabled on 32-bit AIX – -latomic should be added with diagnostic.
Test Case 2: Sanitizer disabled on 32-bit AIX – -latomic should not be added.