[llvm] 15e19a2 - Revert "[instcombine] Exploit UB implied by nofree attributes"

[Philip Reames via llvm-commits]

Author: Philip Reames
Date: 2021-04-22T10:53:17-07:00
New Revision: 15e19a259986ebb993a719b960b6bd3bb28d8329

URL: https://github.com/llvm/llvm-project/commit/15e19a259986ebb993a719b960b6bd3bb28d8329
DIFF: https://github.com/llvm/llvm-project/commit/15e19a259986ebb993a719b960b6bd3bb28d8329.diff

LOG: Revert "[instcombine] Exploit UB implied by nofree attributes"

This change effectively reverts 86664638, but since there have been some changes on top and I wanted to leave the tests in, it's not a mechanical revert.

Why revert this now?  Two main reasons:
1) There are continuing discussion around what the semantics of nofree.  I am getting increasing uncomfortable with the seeming possibility we might redefine nofree in a way incompatible with these changes.
2) There was a reported miscompile triggered by this change (https://github.com/emscripten-core/emscripten/issues/9443).  At first, I was making good progress on tracking down the issues exposed and those issues appeared to be unrelated latent bugs.  Now that we've found at least one bug in the original change, and the investigation has stalled, I'm no longer comfortable leaving this in tree.  In retrospect, I probably should have reverted this earlier and investigated the issues once the triggering change was out of tree.

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
    llvm/test/Transforms/InstCombine/malloc-free-delete.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 724e2122ff600..fc10757dc1ed4 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2799,25 +2799,6 @@ Instruction *InstCombinerImpl::visitFree(CallInst &FI) {
   if (isa<ConstantPointerNull>(Op))
     return eraseInstFromFunction(FI);
 
-  
-  // If we free a non-null pointer we've been explicitly told won't be freed,
-  // this would be full UB and thus we can conclude that the argument must
-  // be null at the point of the free. Cases:
-  // 1) freeing a pointer which is explicitly nofree
-  // 2) calling free from a call site marked nofree (TODO: can generalize
-  //    for non-arguments)
-  // 3) calling free in a function scope marked nofree (when we can prove
-  //    the allocation existed before the start of the function scope)
-  if (auto *A = dyn_cast<Argument>(Op->stripPointerCasts()))
-    if (A->hasAttribute(Attribute::NoFree) ||
-        FI.hasFnAttr(Attribute::NoFree) ||
-        FI.getFunction()->hasFnAttribute(Attribute::NoFree)) {
-      Value *Null = ConstantPointerNull::get(cast<PointerType>(A->getType()));
-      Value *Cond = Builder.CreateICmpEQ(A, Null);
-      Builder.CreateAssumption(Cond);
-      return eraseInstFromFunction(FI);
-    }
-
   // If we optimize for code size, try to move the call to free before the null
   // test so that simplify cfg can remove the empty block and dead code
   // elimination the branch. I.e., helps to turn something like:

diff  --git a/llvm/test/Transforms/InstCombine/malloc-free-delete.ll b/llvm/test/Transforms/InstCombine/malloc-free-delete.ll
index 876d5a14108da..ee7341424607f 100644
--- a/llvm/test/Transforms/InstCombine/malloc-free-delete.ll
+++ b/llvm/test/Transforms/InstCombine/malloc-free-delete.ll
@@ -391,43 +391,44 @@ if.end:                                           ; preds = %entry, %if.then
   ret void
 }
 
-; Freeing a no-free pointer -> %foo must be null
+; The next four tests cover the semantics of the nofree attributes.  These
+; are thought to be legal transforms, but an implementation thereof has
+; been reverted once due to 
diff icult to isolate fallout.
+
+; TODO: Freeing a no-free pointer -> %foo must be null
 define void @test13(i8* nofree %foo) {
 ; CHECK-LABEL: @test13(
-; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i8* [[FOO:%.*]], null
-; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP1]])
+; CHECK-NEXT:    call void @free(i8* [[FOO:%.*]])
 ; CHECK-NEXT:    ret void
 ;
   call void @free(i8* %foo)
   ret void
 }
 
-; Freeing a no-free pointer -> %foo must be null
+; TODO: Freeing a no-free pointer -> %foo must be null
 define void @test14(i8* %foo) nofree {
 ; CHECK-LABEL: @test14(
-; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i8* [[FOO:%.*]], null
-; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP1]])
+; CHECK-NEXT:    call void @free(i8* [[FOO:%.*]])
 ; CHECK-NEXT:    ret void
 ;
   call void @free(i8* %foo)
   ret void
 }
 
-; free call marked no-free ->  %foo must be null
+; TODO: free call marked no-free ->  %foo must be null
 define void @test15(i8* %foo) {
 ; CHECK-LABEL: @test15(
-; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i8* [[FOO:%.*]], null
-; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP1]])
+; CHECK-NEXT:    call void @free(i8* [[FOO:%.*]]) #[[ATTR6:[0-9]+]]
 ; CHECK-NEXT:    ret void
 ;
   call void @free(i8* %foo) nofree
   ret void
 }
 
-; freeing a nonnull nofree pointer -> full UB
+; TODO: freeing a nonnull nofree pointer -> full UB
 define void @test16(i8* nonnull nofree %foo) {
 ; CHECK-LABEL: @test16(
-; CHECK-NEXT:    call void @llvm.assume(i1 false)
+; CHECK-NEXT:    call void @free(i8* [[FOO:%.*]])
 ; CHECK-NEXT:    ret void
 ;
   call void @free(i8* %foo)