Skip to content

Commit

Permalink
Revert "[instcombine] Exploit UB implied by nofree attributes"
Browse files Browse the repository at this point in the history
This change effectively reverts 8666463, 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 (emscripten-core/emscripten#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.
  • Loading branch information
preames committed Apr 22, 2021
1 parent e01c419 commit 15e19a2
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 30 deletions.
19 changes: 0 additions & 19 deletions llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
23 changes: 12 additions & 11 deletions llvm/test/Transforms/InstCombine/malloc-free-delete.ll
Original file line number Diff line number Diff line change
Expand Up @@ -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 difficult 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)
Expand Down

0 comments on commit 15e19a2

Please sign in to comment.