Skip to content
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

[Sanitizers] the access size (8 bytes) exceeds the max lock-free size (4 bytes) for 32-bit #125388

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
+28 −1
Open

[Sanitizers] the access size (8 bytes) exceeds the max lock-free size (4 bytes) for 32-bit #125388

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f9d8e7f
Test Cases for adding -latomic (the access size (8 bytes) exceeds the…
Feb 2, 2025
1b7439e
Added -latomic when sanitizer flag is enabled and processor is 32-bit
Feb 2, 2025
84d973a
Suppress -Watomic-alignment warnings by not treating them as errors
Feb 2, 2025
3addb86
Update clang/test/Driver/aix-ld.c
honeygoyal Feb 3, 2025
b0b9c5a
Update clang/test/Driver/aix-ld.c
honeygoyal Feb 3, 2025
317f5ca
Added Comments Required for 64-bit atomic operations used in sanitize…
Feb 3, 2025
2bae081
Added Comments in aix-id.c test
Feb 3, 2025
0b737c9
Added Check specific to AIX and 32 bit processor in CMakeLists.txt
Feb 3, 2025
e130b14
- Use MATCHES for flexible OS detection
Feb 3, 2025
9715d9e
Update clang/test/Driver/aix-ld.c
honeygoyal Feb 4, 2025
721a57c
Update aix-ld.c
honeygoyal Feb 5, 2025
395131e
Update aix-ld.c
honeygoyal Feb 5, 2025
04857d6
Update the comments and mention the specific issue on non-AIX host
Feb 5, 2025
2269b9c
Merge branch 'main' into issue-17812
honeygoyal Feb 5, 2025
a45304a
Update clang/test/Driver/aix-ld.c
honeygoyal Feb 5, 2025
195445e
Merge branch 'main' into issue-17812
honeygoyal Feb 6, 2025
22749c3
Merge branch 'main' into issue-17812
honeygoyal Feb 6, 2025
cfd96ad
Update test cases
Feb 6, 2025
6388207
Dummy commit
Feb 6, 2025
c3f8734
Dummy commit
Feb 6, 2025
de739d8
Update clang/test/Driver/aix-ld.c
honeygoyal Feb 6, 2025
d3857ba
Update clang/test/Driver/aix-ld.c
honeygoyal Feb 6, 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
9 changes: 8 additions & 1 deletion clang/lib/Driver/ToolChains/AIX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -338,6 +338,13 @@ void aix::Linker::ConstructJob(Compilation &C, const JobAction &JA,
CmdArgs.push_back("-lpthread");
}
const char *Exec = Args.MakeArgString(ToolChain.GetLinkerPath());

// Required for 64-bit atomic operations used in sanitizer runtimes
// (e.g., sanitizer_common's atomic utilities). On 32-bit AIX, these
// are not natively supported, necessitating linkage with -latomic.
if (Sanitize.hasAnySanitizer() && IsArch32Bit) {
honeygoyal marked this conversation as resolved.
Show resolved Hide resolved
CmdArgs.push_back("-latomic");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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:

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Line 541 in f729477

bool NeedsSanitizerDeps = addSanitizerRuntimes(ToolChain, Args, CmdArgs);

llvm-project/clang/lib/Driver/ToolChains/Gnu.cpp

Lines 585 to 586 in f729477

if (NeedsSanitizerDeps)
linkSanitizerRuntimeDeps(ToolChain, Args, CmdArgs);

llvm-project/clang/lib/Driver/ToolChains/CommonArgs.cpp

Lines 1607 to 1608 in f729477

// C runtime, etc). Returns true if sanitizer system deps need to be linked in.
bool tools::addSanitizerRuntimes(const ToolChain &TC, const ArgList &Args,

llvm-project/clang/lib/Driver/ToolChains/CommonArgs.cpp

Line 1678 in f729477

return !StaticRuntimes.empty() || !NonWholeStaticRuntimes.empty();

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@hubert-reinterpretcast hubert-reinterpretcast Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (TC.getTriple().isOSFreeBSD() || TC.getTriple().isOSNetBSD() ||
TC.getTriple().isOSOpenBSD() || TC.getTriple().isOSDragonFly())
CmdArgs.push_back("-lexecinfo");

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.

Copy link
Collaborator

@hubert-reinterpretcast hubert-reinterpretcast Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
C.addCommand(std::make_unique<Command>(JA, *this, ResponseFileSupport::None(),
Exec, CmdArgs, Inputs, Output));
}
Expand Down
16 changes: 16 additions & 0 deletions clang/test/Driver/aix-ld.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1120,3 +1120,19 @@
// 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: %clang -target powerpc-ibm-aix -m32 %s -### 2>&1 \
Copy link
Member

Choose a reason for hiding this comment

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

-target has been deprecated for a long time though without a warning. Use --target= for new tests

// RUN: | FileCheck -check-prefix=CHECK-LD32-NO-SANITIZER %s
// CHECK-LD32-NO-SANITIZER-NOT: "-latomic"

// This test verifies that the linker doesn't include '-latomic' when no sanitizers are enabled
honeygoyal marked this conversation as resolved.
Show resolved Hide resolved
// FIXME: Running this test on non-AIX host will result in the following error:
// LLVM ERROR: Sanitizer interface functions must be exported by export files on AIX

// Check enable AddressSanitizer on 32-bit AIX
honeygoyal marked this conversation as resolved.
Show resolved Hide resolved
// RUN: %if target={{.*aix.*}} %{ \
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this diagnostic Sanitizer interface functions must be exported by export files on AIX defined 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.

// RUN: %clang -target powerpc-ibm-aix -m32 -fsanitize=address %s -### 2>&1 \
// RUN: | FileCheck -check-prefix=CHECK-LD32-ASAN %s \
// RUN: %}
// CHECK-LD32-ASAN: "-latomic"
hubert-reinterpretcast marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 5 additions & 0 deletions compiler-rt/lib/sanitizer_common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,11 @@ 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
honeygoyal marked this conversation as resolved.
Show resolved Hide resolved
if (CMAKE_SIZEOF_VOID_P EQUAL 4 AND CMAKE_SYSTEM_NAME MATCHES "AIX")
list(APPEND SANITIZER_CFLAGS "-Wno-error=atomic-alignment")
endif()

if(APPLE)
set(OS_OPTION OS ${SANITIZER_COMMON_SUPPORTED_OS})
endif()
Expand Down