Skip to content

[UBSan][Clang][CodeGen] Improve memory effect modeling of ubsan handlers #130093

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

Closed
dtcxzyw opened this issue Mar 6, 2025 · 1 comment · Fixed by #130990
Closed

[UBSan][Clang][CodeGen] Improve memory effect modeling of ubsan handlers #130093

dtcxzyw opened this issue Mar 6, 2025 · 1 comment · Fixed by #130990
Assignees
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. compiler-rt:ubsan Undefined behavior sanitizer missed-optimization

Comments

@dtcxzyw
Copy link
Member

dtcxzyw commented Mar 6, 2025

Consider the following case: https://godbolt.org/z/zxxn3oG1z

; bin/clang -O3 -fsanitize=undefined -S -emit-llvm -o -
int test(int &a, short &c) {
    a += c;
    return c;
}
@.src = private unnamed_addr constant [17 x i8] c"/app/example.cpp\00", align 1
@0 = private unnamed_addr constant { i16, i16, [6 x i8] } { i16 0, i16 11, [6 x i8] c"'int'\00" }
@1 = private unnamed_addr global { { ptr, i32, i32 }, ptr } { { ptr, i32, i32 } { ptr @.src, i32 2, i32 7 }, ptr @0 }

define dso_local noundef range(i32 -32768, 32768) i32 @test(int&, short&)(ptr noundef nonnull align 4 captures(none) dereferenceable(4) %a, ptr noundef nonnull readonly align 2 captures(none) dereferenceable(2) %c) local_unnamed_addr #0 {
entry:
  %0 = load i16, ptr %c, align 2
  %conv = sext i16 %0 to i32
  %1 = load i32, ptr %a, align 4
  %2 = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 %1, i32 %conv)
  %3 = extractvalue { i32, i1 } %2, 1
  br i1 %3, label %handler.add_overflow, label %cont

handler.add_overflow:
  %4 = zext i32 %1 to i64
  %5 = zext i32 %conv to i64
  tail call void @__ubsan_handle_add_overflow(ptr nonnull @1, i64 %4, i64 %5) #3
  %.pre = load i16, ptr %c, align 2
  %.pre3 = sext i16 %.pre to i32
  br label %cont

cont:
  %conv1.pre-phi = phi i32 [ %.pre3, %handler.add_overflow ], [ %conv, %entry ]
  %6 = extractvalue { i32, i1 } %2, 0
  store i32 %6, ptr %a, align 4
  ret i32 %conv1.pre-phi
}

declare { i32, i1 } @llvm.sadd.with.overflow.i32(i32, i32) #1

declare void @__ubsan_handle_add_overflow(ptr, i64, i64) local_unnamed_addr #2

attributes #0 = { mustprogress nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #1 = { mustprogress nocallback nofree nosync nounwind speculatable willreturn memory(none) }
attributes #2 = { uwtable }
attributes #3 = { nounwind }

If we mark __ubsan_handle_add_overflow(_abort) as memory(argmem: read, inaccessiblemem: readwrite), we can avoid reloading c after the call to __ubsan_handle_add_overflow.

TBH this example doesn't demonstrate a performance issue since we only optimize code in the cold path. However, it would allow more aggressive LICM in the hot path, which improves the performance of fuzzers like AFL.

@dtcxzyw dtcxzyw added clang:codegen IR generation bugs: mangling, exceptions, etc. missed-optimization labels Mar 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 6, 2025

@llvm/issue-subscribers-clang-codegen

Author: Yingwei Zheng (dtcxzyw)

Consider the following case: https://godbolt.org/z/zxxn3oG1z ``` ; bin/clang -O3 -fsanitize=undefined -S -emit-llvm -o - int test(int &a, short &c) { a += c; return c; } ``` ``` @.src = private unnamed_addr constant [17 x i8] c"/app/example.cpp\00", align 1 @0 = private unnamed_addr constant { i16, i16, [6 x i8] } { i16 0, i16 11, [6 x i8] c"'int'\00" } @1 = private unnamed_addr global { { ptr, i32, i32 }, ptr } { { ptr, i32, i32 } { ptr @.src, i32 2, i32 7 }, ptr @0 }

define dso_local noundef range(i32 -32768, 32768) i32 @test(int&, short&)(ptr noundef nonnull align 4 captures(none) dereferenceable(4) %a, ptr noundef nonnull readonly align 2 captures(none) dereferenceable(2) %c) local_unnamed_addr #0 {
entry:
%0 = load i16, ptr %c, align 2
%conv = sext i16 %0 to i32
%1 = load i32, ptr %a, align 4
%2 = tail call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 %1, i32 %conv)
%3 = extractvalue { i32, i1 } %2, 1
br i1 %3, label %handler.add_overflow, label %cont

handler.add_overflow:
%4 = zext i32 %1 to i64
%5 = zext i32 %conv to i64
tail call void @__ubsan_handle_add_overflow(ptr nonnull @1, i64 %4, i64 %5) #3
%.pre = load i16, ptr %c, align 2
%.pre3 = sext i16 %.pre to i32
br label %cont

cont:
%conv1.pre-phi = phi i32 [ %.pre3, %handler.add_overflow ], [ %conv, %entry ]
%6 = extractvalue { i32, i1 } %2, 0
store i32 %6, ptr %a, align 4
ret i32 %conv1.pre-phi
}

declare { i32, i1 } @llvm.sadd.with.overflow.i32(i32, i32) #1

declare void @__ubsan_handle_add_overflow(ptr, i64, i64) local_unnamed_addr #2

attributes #0 = { mustprogress nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #1 = { mustprogress nocallback nofree nosync nounwind speculatable willreturn memory(none) }
attributes #2 = { uwtable }
attributes #3 = { nounwind }

If we mark `__ubsan_handle_add_overflow(_abort)` as `memory(argmem: read, inaccessiblemem: readwrite)`, we can avoid reloading `c` after the call to `__ubsan_handle_add_overflow`.

TBH this example doesn't demonstrate a performance issue since we only optimize code in the cold path. However, it would allow more aggressive LICM in the hot path, which improves the performance of fuzzers like AFL.


</details>

@dtcxzyw dtcxzyw added the compiler-rt:ubsan Undefined behavior sanitizer label Mar 6, 2025
@dtcxzyw dtcxzyw self-assigned this Mar 6, 2025
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this issue Apr 10, 2025
…overable ubsan handlers (#130990)

This patch adds `memory(argmem: read, inaccessiblemem: readwrite)
mustprogress` to **recoverable** ubsan handlers in order to unblock some
memory/loop optimizations. It provides an average of 3% performance
improvement on llvm-test-suite (except for 49 test failures due to ubsan
diagnostics).

Closes llvm/llvm-project#130093.
AllinLeeYL pushed a commit to AllinLeeYL/llvm-project that referenced this issue Apr 10, 2025
…san handlers (llvm#130990)

This patch adds `memory(argmem: read, inaccessiblemem: readwrite)
mustprogress` to **recoverable** ubsan handlers in order to unblock some
memory/loop optimizations. It provides an average of 3% performance
improvement on llvm-test-suite (except for 49 test failures due to ubsan
diagnostics).

Closes llvm#130093.
dtcxzyw added a commit to dtcxzyw/llvm-project that referenced this issue Apr 10, 2025
…san handlers (llvm#130990)

This patch adds `memory(argmem: read, inaccessiblemem: readwrite)
mustprogress` to **recoverable** ubsan handlers in order to unblock some
memory/loop optimizations. It provides an average of 3% performance
improvement on llvm-test-suite (except for 49 test failures due to ubsan
diagnostics).

Closes llvm#130093.
dtcxzyw added a commit to dtcxzyw/llvm-project that referenced this issue Apr 12, 2025
…san handlers (llvm#130990)

This patch adds `memory(argmem: read, inaccessiblemem: readwrite)
mustprogress` to **recoverable** ubsan handlers in order to unblock some
memory/loop optimizations. It provides an average of 3% performance
improvement on llvm-test-suite (except for 49 test failures due to ubsan
diagnostics).

Closes llvm#130093.
var-const pushed a commit to ldionne/llvm-project that referenced this issue Apr 17, 2025
…san handlers (llvm#130990)

This patch adds `memory(argmem: read, inaccessiblemem: readwrite)
mustprogress` to **recoverable** ubsan handlers in order to unblock some
memory/loop optimizations. It provides an average of 3% performance
improvement on llvm-test-suite (except for 49 test failures due to ubsan
diagnostics).

Closes llvm#130093.
dtcxzyw added a commit to dtcxzyw/llvm-project that referenced this issue Apr 17, 2025
…san handlers (llvm#130990)

This patch adds `memory(argmem: read, inaccessiblemem: readwrite)
mustprogress` to **recoverable** ubsan handlers in order to unblock some
memory/loop optimizations. It provides an average of 3% performance
improvement on llvm-test-suite (except for 49 test failures due to ubsan
diagnostics).

Closes llvm#130093.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. compiler-rt:ubsan Undefined behavior sanitizer missed-optimization
Projects
None yet
2 participants