[PATCH] D115167: [DSE] Use precise loc for memset_chk writing to local objects.

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 392782.
fhahn added a comment.



In D115167#3177439 <https://reviews.llvm.org/D115167#3177439>, @efriedma wrote:

> In D115167#3176652 <https://reviews.llvm.org/D115167#3176652>, @fhahn wrote:
>
>> It might have been overly conservative on my side. I was thinking about cases where a failing `memset_chk` aborts the current thread/process but not other threads or  processes that may access the same (shared) memory. But if that's not an issue the restriction can be removed.
>
> memset_chk aborts the whole process.  Throwing an exception on an out-of-bounds access is a popular model in other contexts, but memset_chk is not defined that way.  It's meant purely as a hardening measure.
>
> memset_chk should not be used on memory shared with other processes.  Accesses to such memory should be volatile or atomic; otherwise, weird stuff can happen for a variety of reasons...

Sounds good, thanks! I remove the local-object restriction and added a comment explaining why using the precise bound is fine here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115167/new/

https://reviews.llvm.org/D115167

Files:
  llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
  llvm/test/Transforms/DeadStoreElimination/libcalls.ll


Index: llvm/test/Transforms/DeadStoreElimination/libcalls.ll
===================================================================
--- llvm/test/Transforms/DeadStoreElimination/libcalls.ll
+++ llvm/test/Transforms/DeadStoreElimination/libcalls.ll
@@ -468,8 +468,7 @@
 ; strncpy -> __memset_chk, full overwrite
 define void @dse_strncpy_memset_chk_test1(i8* noalias %out, i8* noalias %in, i64 %n) {
 ; CHECK-LABEL: @dse_strncpy_memset_chk_test1(
-; CHECK-NEXT:    [[CALL:%.*]] = tail call i8* @strncpy(i8* [[OUT:%.*]], i8* [[IN:%.*]], i64 100)
-; CHECK-NEXT:    [[CALL_2:%.*]] = tail call i8* @__memset_chk(i8* [[OUT]], i32 42, i64 100, i64 [[N:%.*]])
+; CHECK-NEXT:    [[CALL_2:%.*]] = tail call i8* @__memset_chk(i8* [[OUT:%.*]], i32 42, i64 100, i64 [[N:%.*]])
 ; CHECK-NEXT:    ret void
 ;
   %call = tail call i8* @strncpy(i8* %out, i8* %in, i64 100)
@@ -481,8 +480,7 @@
 
 define void @dse_memset_chk_cannot_eliminates_store(i8* %out, i64 %n) {
 ; CHECK-LABEL: @dse_memset_chk_cannot_eliminates_store(
-; CHECK-NEXT:    store i8 10, i8* [[OUT:%.*]], align 1
-; CHECK-NEXT:    [[CALL_2:%.*]] = tail call i8* @__memset_chk(i8* [[OUT]], i32 42, i64 100, i64 [[N:%.*]])
+; CHECK-NEXT:    [[CALL_2:%.*]] = tail call i8* @__memset_chk(i8* [[OUT:%.*]], i32 42, i64 100, i64 [[N:%.*]])
 ; CHECK-NEXT:    ret void
 ;
   store i8 10, i8* %out
@@ -494,7 +492,6 @@
 ; CHECK-LABEL: @dse_memset_chk_eliminates_store_local_object_escapes_after(
 ; CHECK-NEXT:    [[A:%.*]] = alloca [200 x i8], align 1
 ; CHECK-NEXT:    [[OUT:%.*]] = bitcast [200 x i8]* [[A]] to i8*
-; CHECK-NEXT:    store i8 10, i8* [[OUT]], align 1
 ; CHECK-NEXT:    [[OUT_100:%.*]] = getelementptr i8, i8* [[OUT]], i64 100
 ; CHECK-NEXT:    store i8 10, i8* [[OUT_100]], align 1
 ; CHECK-NEXT:    [[CALL_2:%.*]] = tail call i8* @__memset_chk(i8* [[OUT]], i32 42, i64 100, i64 [[N:%.*]])
@@ -516,7 +513,6 @@
 ; CHECK-NEXT:    [[A:%.*]] = alloca [200 x i8], align 1
 ; CHECK-NEXT:    [[OUT:%.*]] = bitcast [200 x i8]* [[A]] to i8*
 ; CHECK-NEXT:    call void @use(i8* [[OUT]])
-; CHECK-NEXT:    store i8 10, i8* [[OUT]], align 1
 ; CHECK-NEXT:    [[OUT_100:%.*]] = getelementptr i8, i8* [[OUT]], i64 100
 ; CHECK-NEXT:    store i8 0, i8* [[OUT_100]], align 1
 ; CHECK-NEXT:    [[CALL_2:%.*]] = tail call i8* @__memset_chk(i8* [[OUT]], i32 42, i64 100, i64 [[N:%.*]])
Index: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -1022,7 +1022,7 @@
     return I.first->second;
   }
 
-  Optional<MemoryLocation> getLocForWriteEx(Instruction *I) const {
+  Optional<MemoryLocation> getLocForWriteEx(Instruction *I) {
     if (!I->mayWriteToMemory())
       return None;
 
@@ -1032,6 +1032,19 @@
           !CB->onlyAccessesInaccessibleMemOrArgMem())
         return None;
 
+      LibFunc F;
+      if (TLI.getLibFunc(*CB, F) && TLI.has(F) && F == LibFunc_memset_chk) {
+        // For the uses in DSE, it is safe to use the precise location instead
+        // of an upper bound. DSE uses the location information to remove
+        // earlier or later stores with the assumption that the memset_chk
+        // executes. So it either writes exactly the number of specified bytes
+        // or the program gets aborted.
+        if (const auto *Len = dyn_cast<ConstantInt>(CB->getArgOperand(2))) {
+          Value *Ptr = CB->getArgOperand(0);
+          return MemoryLocation(Ptr, LocationSize::precise(Len->getZExtValue()),
+                                CB->getAAMetadata());
+        }
+      }
       return MemoryLocation::getForDest(CB, TLI);
     }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D115167.392782.patch
Type: text/x-patch
Size: 3699 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211208/1b0ff7e6/attachment.bin>