free(nullptr) does not violate the nofree specification

[reames]

Summary

This fixes a subtle and nasty bug in my 86664638. The problem is that free(nullptr) is well defined (and common).

The specification for the nofree attributes talks about memory objects, and doesn't explicitly address null, but I think it's reasonable to assume that nofree doesn't disallow a call to free(nullptr). If it did, we'd have to prove nonnull on an argument to ever infer nofree which doesn't seem to be the intent.

This was found by Nuno and Alive2 over in https://reviews.llvm.org/D100141#2697374. Once I saw the test case (and explanation Nuno provided), I also suspect this is the root cause of https://github.com/emscripten-core/emscripten/issues/9443 as well. (Though that had at least two root issues, so there might be more as well.)

[jdoerfert]

LGTM

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

if (isa<UndefValue>(Op)) {
     return eraseInstFromFunction(FI);
   }
 
   // If we have 'free null' delete the instruction.  This can happen in stl code
   // when lots of inlining happens.
   if (isa<ConstantPointerNull>(Op))
     return eraseInstFromFunction(FI);
 
-  // If we free a pointer we've been explicitly told won't be freed, this
-  // would be full UB and thus we can conclude this is unreachable. Cases:
+  
+  // 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)) {
-      // Leave a marker since we can't modify the CFG here.
-      CreateNonTerminatorUnreachable(&FI);
+      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:
   // if (foo) free(foo);
   // into

llvm/test/Transforms/InstCombine/malloc-free-delete.ll

if.then:                                          ; preds = %entry
   %bitcast = bitcast i32* %foo to i8*
   tail call void @free(i8* %bitcast)
   br label %if.end
 
 if.end:                                           ; preds = %entry, %if.then
   ret void
 }
 
-; Freeing a no-free pointer -> full UB
+; Freeing a no-free pointer -> %foo must be null
 define void @test13(i8* nofree %foo) {
 ; CHECK-LABEL: @test13(
-; CHECK-NEXT:    store i1 true, i1* undef, align 1
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i8* [[FOO:%.*]], null
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP1]])
 ; CHECK-NEXT:    ret void
 ;
   call void @free(i8* %foo)
   ret void
 }
 
-; Freeing a no-free pointer -> full UB
+; Freeing a no-free pointer -> %foo must be null
 define void @test14(i8* %foo) nofree {
 ; CHECK-LABEL: @test14(
-; CHECK-NEXT:    store i1 true, i1* undef, align 1
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i8* [[FOO:%.*]], null
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP1]])
 ; CHECK-NEXT:    ret void
 ;
   call void @free(i8* %foo)
   ret void
 }
 
-; free call marked no-free -> full UB
+; free call marked no-free ->  %foo must be null
 define void @test15(i8* %foo) {
 ; CHECK-LABEL: @test15(
-; CHECK-NEXT:    store i1 true, i1* undef, align 1
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i8* [[FOO:%.*]], null
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP1]])
 ; CHECK-NEXT:    ret void
 ;
   call void @free(i8* %foo) nofree
   ret void
 }
+
+; 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:    ret void
+;
+  call void @free(i8* %foo)
+  ret void
+}