[PATCH] D144057: [GVN] permit GVN for ASAN unless undef is produced

Tue Feb 14 16:20:45 PST 2023

nickdesaulniers created this revision.
Herald added a subscriber: hiraditya.
Herald added a project: All.
nickdesaulniers requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

While compiling the Linux kernel with CONFIG_FORTIFY_SOURCE=y and
CONFIG_KASAN=y, we discovered a curious case where a repeated condition
check that should have been optimized out was not.

It looks like GVN's ability to process "non local loads" was simply
disabled outright in pr25924 due to an external report.

That fix was too broad IMO. While we do want ASAN (and HWASAN) to help
us spot loads that produce undef, since those are likely OOB reads, it
still would be nice when we don't have undef to allow GVN to proceed. I
think this is a better balance. It should allow us to better optimize
KASAN binaries without regressions in the sanitizers ability to help
spot OOB access.

Tested with the original reproducer from the mailing list, as well as
the above linux kernel configs.

Link: https://github.com/ClangBuiltLinux/linux/issues/1687
Link: https://github.com/llvm/llvm-project/issues/25924
Link: https://lists.llvm.org/pipermail/llvm-dev/2015-November/092427.html
Link: http://lists.llvm.org/pipermail/llvm-dev/attachments/20151114/1c7d8dbe/attachment.c


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144057

Files:
  llvm/lib/Transforms/Scalar/GVN.cpp
  llvm/test/Transforms/GVN/no_speculative_loads_with_asan.ll


Index: llvm/test/Transforms/GVN/no_speculative_loads_with_asan.ll
===================================================================

--- llvm/test/Transforms/GVN/no_speculative_loads_with_asan.ll
+++ llvm/test/Transforms/GVN/no_speculative_loads_with_asan.ll
@@ -3,7 +3,7 @@
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 declare noalias ptr @_Znam(i64) #1
 
-; Load of %i8 is an out of bounds load, which is folded to poison, which allows
+; Load of %i8 is an out of bounds load, which is folded to undef, which allows
 ; us to elide the phi.
 define i32 @TestNoAsan() {
 ; CHECK-LABEL: @TestNoAsan(
@@ -158,27 +158,25 @@
 ; CHECK-NEXT:    [[TOBOOL_I:%.*]] = icmp ne i32 [[TMP1]], 0
 ; CHECK-NEXT:    br i1 [[TOBOOL_I]], label [[WHILE_BODY_I:%.*]], label [[NODE_FROM_NDEV_IP_EXIT:%.*]]
 ; CHECK:       while.body.i:
-; CHECK-NEXT:    [[TMP2:%.*]] = load ptr, ptr [[SA_ADDR_I]], align 8
-; CHECK-NEXT:    store ptr [[TMP2]], ptr [[SA_ADDR_I_I]], align 8
-; CHECK-NEXT:    [[TMP3:%.*]] = load i16, ptr [[TMP2]], align 2
-; CHECK-NEXT:    [[TOBOOL_I_I:%.*]] = icmp ne i16 [[TMP3]], 0
+; CHECK-NEXT:    store ptr [[NEIGH_SOCK_4]], ptr [[SA_ADDR_I_I]], align 8
+; CHECK-NEXT:    [[TMP2:%.*]] = load i16, ptr [[NEIGH_SOCK_4]], align 2
+; CHECK-NEXT:    [[TOBOOL_I_I:%.*]] = icmp ne i16 [[TMP2]], 0
 ; CHECK-NEXT:    br i1 [[TOBOOL_I_I]], label [[IF_THEN_I_I:%.*]], label [[IF_END_I_I:%.*]]
 ; CHECK:       if.then.i.i:
-; CHECK-NEXT:    [[TMP4:%.*]] = load ptr, ptr [[SA_ADDR_I_I]], align 8
-; CHECK-NEXT:    [[TMP5:%.*]] = load i32, ptr [[TMP4]], align 4
-; CHECK-NEXT:    [[CONV_I_I:%.*]] = sext i32 [[TMP5]] to i64
-; CHECK-NEXT:    [[CALL_I_I:%.*]] = call i32 @_Z6memcmpPvS_m(ptr noundef [[TMP4]], ptr noundef @compare_netdev_and_ip_sb_0, i64 noundef [[CONV_I_I]])
+; CHECK-NEXT:    [[TMP3:%.*]] = load i32, ptr [[NEIGH_SOCK_4]], align 4
+; CHECK-NEXT:    [[CONV_I_I:%.*]] = sext i32 [[TMP3]] to i64
+; CHECK-NEXT:    [[CALL_I_I:%.*]] = call i32 @_Z6memcmpPvS_m(ptr noundef [[NEIGH_SOCK_4]], ptr noundef @compare_netdev_and_ip_sb_0, i64 noundef [[CONV_I_I]])
 ; CHECK-NEXT:    store i32 [[CALL_I_I]], ptr [[RETVAL_I_I]], align 4
 ; CHECK-NEXT:    br label [[COMPARE_NETDEV_AND_IP_EXIT_I:%.*]]
 ; CHECK:       if.end.i.i:
-; CHECK-NEXT:    [[TMP6:%.*]] = load ptr, ptr @ipv6_addr_cmp_a1, align 8
-; CHECK-NEXT:    [[CALL2_I_I:%.*]] = call i32 @_Z6memcmpPvS_m(ptr noundef [[TMP6]], ptr noundef @ipv6_addr_cmp_a2, i64 noundef 4)
+; CHECK-NEXT:    [[TMP4:%.*]] = load ptr, ptr @ipv6_addr_cmp_a1, align 8
+; CHECK-NEXT:    [[CALL2_I_I:%.*]] = call i32 @_Z6memcmpPvS_m(ptr noundef [[TMP4]], ptr noundef @ipv6_addr_cmp_a2, i64 noundef 4)
 ; CHECK-NEXT:    store i32 [[CALL2_I_I]], ptr @compare_netdev_and_ip___trans_tmp_1, align 4
 ; CHECK-NEXT:    store i32 [[CALL2_I_I]], ptr [[RETVAL_I_I]], align 4
 ; CHECK-NEXT:    br label [[COMPARE_NETDEV_AND_IP_EXIT_I]]
 ; CHECK:       compare_netdev_and_ip.exit.i:
-; CHECK-NEXT:    [[TMP7:%.*]] = load i32, ptr [[RETVAL_I_I]], align 4
-; CHECK-NEXT:    store i32 [[TMP7]], ptr @node_from_ndev_ip_data, align 4
+; CHECK-NEXT:    [[TMP5:%.*]] = phi i32 [ [[CALL2_I_I]], [[IF_END_I_I]] ], [ [[CALL_I_I]], [[IF_THEN_I_I]] ]
+; CHECK-NEXT:    store i32 [[TMP5]], ptr @node_from_ndev_ip_data, align 4
 ; CHECK-NEXT:    br label [[WHILE_COND_I]]
 ; CHECK:       node_from_ndev_ip.exit:
 ; CHECK-NEXT:    br label [[IF_END]]
Index: llvm/lib/Transforms/Scalar/GVN.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/GVN.cpp
+++ llvm/lib/Transforms/Scalar/GVN.cpp
@@ -1717,13 +1717,6 @@
 /// Attempt to eliminate a load whose dependencies are
 /// non-local by performing PHI construction.
 bool GVNPass::processNonLocalLoad(LoadInst *Load) {
-  // non-local speculations are not allowed under asan.
-  if (Load->getParent()->getParent()->hasFnAttribute(
-          Attribute::SanitizeAddress) ||
-      Load->getParent()->getParent()->hasFnAttribute(
-          Attribute::SanitizeHWAddress))
-    return false;
-
   // Step 1: Find the non-local dependencies of the load.
   LoadDepVect Deps;
   MD->getNonLocalPointerDependency(Load, Deps);
@@ -1773,6 +1766,16 @@
 
     // Perform PHI construction.
     Value *V = ConstructSSAForLoadSet(Load, ValuesPerBlock, *this);
+
+    // If a non-local load would result in producing undef, don't fold away
+    // control flow. We want the sanitizers to help report this case.
+    if (isa<UndefValue>(V)) {
+      Function *F = Load->getParent()->getParent();
+      if (F->hasFnAttribute(Attribute::SanitizeAddress) ||
+          F->hasFnAttribute(Attribute::SanitizeHWAddress))
+        return false;
+    }
+
     Load->replaceAllUsesWith(V);
 
     if (isa<PHINode>(V))


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144057.497473.patch
Type: text/x-patch
Size: 4713 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230215/8836abe5/attachment.bin>
