[instcombine] Exploit UB implied by nofree attributes

[reames]

Summary

This patch simply implements the documented UB of the current nofree attributes as specified. It doesn't try to be fancy about inference (yet), it just implements the cases already specified and inferred.

Note: When this lands, it may expose miscompiles. If so, please revert and provide a test case. It's likely the bug is in the existing inference code and without a relatively complete test case, it will be hard to debug.

[jdoerfert]

LGTM. We should probably add the following negative test case:

; Freeing in a nonfree function is fine if the effect is invisible to the outside
define void @test16() nofree {
  i8* %foo = call i8* @malloc(i32 1)
  call void @free(i8* %foo)
  ret void
}

[reames]

In D96349#2551984, @jdoerfert wrote:

LGTM. We should probably add the following negative test case:

; Freeing in a nonfree function is fine if the effect is invisible to the outside
define void @test16() nofree {
  i8* %foo = call i8* @malloc(i32 1)
  call void @free(i8* %foo)
  ret void
}

Er, no. That's not okay. Either by the implementation in this patch, or by my reading of the LangRef. The langref says "This function attribute indicates that the function does not, directly or indirectly, call a memory-deallocation function (free, for example). " There's no exception there for memory allocated in scope, nor should there be.

[jdoerfert]

In D96349#2552491, @reames wrote:

In D96349#2551984, @jdoerfert wrote:

LGTM. We should probably add the following negative test case:

; Freeing in a nonfree function is fine if the effect is invisible to the outside
define void @test16() nofree {
  i8* %foo = call i8* @malloc(i32 1)
  call void @free(i8* %foo)
  ret void
}

Er, no. That's not okay. Either by the implementation in this patch, or by my reading of the LangRef. The langref says "This function attribute indicates that the function does not, directly or indirectly, call a memory-deallocation function (free, for example). " There's no exception there for memory allocated in scope, nor should there be.

Right, we want to tweak the lang ref. Generally, I believe this is is one of those cases where it is "as if" it doesn't free. I mean, you can't tell it does from the outside. Imagine a backend that realizes the stack allocation is too big and puts it on the heap with a proper free. If the function was nofree before it would not be anymore even though we might have "thought so" during the compilation. Similarly, a writenone function can write to some alloca without it being a problem (IMHO).

[reames]

In D96349#2552559, @jdoerfert wrote:

In D96349#2552491, @reames wrote:

In D96349#2551984, @jdoerfert wrote:

LGTM. We should probably add the following negative test case:

; Freeing in a nonfree function is fine if the effect is invisible to the outside
define void @test16() nofree {
  i8* %foo = call i8* @malloc(i32 1)
  call void @free(i8* %foo)
  ret void
}

Er, no. That's not okay. Either by the implementation in this patch, or by my reading of the LangRef. The langref says "This function attribute indicates that the function does not, directly or indirectly, call a memory-deallocation function (free, for example). " There's no exception there for memory allocated in scope, nor should there be.

Right, we want to tweak the lang ref.

I'm not going to enter a debate on whether the LangRef is right here. We can do that on llvm-dev or private conversation. This patch is solely implementing the semantics as currently specified.

Does your LGTM still stand with that understanding?

[jdoerfert]

In D96349#2552672, @reames wrote:

In D96349#2552559, @jdoerfert wrote:

In D96349#2552491, @reames wrote:

In D96349#2551984, @jdoerfert wrote:

LGTM. We should probably add the following negative test case:

; Freeing in a nonfree function is fine if the effect is invisible to the outside
define void @test16() nofree {
  i8* %foo = call i8* @malloc(i32 1)
  call void @free(i8* %foo)
  ret void
}

Er, no. That's not okay. Either by the implementation in this patch, or by my reading of the LangRef. The langref says "This function attribute indicates that the function does not, directly or indirectly, call a memory-deallocation function (free, for example). " There's no exception there for memory allocated in scope, nor should there be.

Right, we want to tweak the lang ref.

I'm not going to enter a debate on whether the LangRef is right here. We can do that on llvm-dev or private conversation. This patch is solely implementing the semantics as currently specified.

Does your LGTM still stand with that understanding?

Yes, this is restrictive enough to be correct either way.

[reames]

FYI, there was a nasty bug found in this patch. A fix is on review in https://reviews.llvm.org/D100779.

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:
+  // 1) freeing a pointer which is explicitly nofree
+  // 2) calling free from a call site marked nofree
+  // 3) calling free in a function scope marked nofree
+  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);
+      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
   // free(foo);
   //
   // Note that we can only do this for 'free' and not for any flavor of

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

 ; CHECK-NEXT:    ret void
 ;
   call void @_ZdlPv(i8* null)
   ret void
 }
 
 define void @test11() {
 ; CHECK-LABEL: @test11(
-; CHECK-NEXT:    [[CALL:%.*]] = call dereferenceable(8) i8* @_Znwm(i64 8) #7
+; CHECK-NEXT:    [[CALL:%.*]] = call dereferenceable(8) i8* @_Znwm(i64 8) #8
 ; CHECK-NEXT:    call void @_ZdlPv(i8* nonnull [[CALL]])
 ; CHECK-NEXT:    ret void
 ;
   %call = call i8* @_Znwm(i64 8) builtin
   call void @_ZdlPv(i8* %call)
   ret void
 }
 
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
+define void @test13(i8* nofree %foo) {
+; CHECK-LABEL: @test13(
+; CHECK-NEXT:    store i1 true, i1* undef, align 1
+; CHECK-NEXT:    ret void
+;
+  call void @free(i8* %foo)
+  ret void
+}
+
+; Freeing a no-free pointer -> full UB
+define void @test14(i8* %foo) nofree {
+; CHECK-LABEL: @test14(
+; CHECK-NEXT:    store i1 true, i1* undef, align 1
+; CHECK-NEXT:    ret void
+;
+  call void @free(i8* %foo)
+  ret void
+}
+
+; free call marked no-free -> full UB
+define void @test15(i8* %foo) {
+; CHECK-LABEL: @test15(
+; CHECK-NEXT:    store i1 true, i1* undef, align 1
+; CHECK-NEXT:    ret void
+;
+  call void @free(i8* %foo) nofree
+  ret void
+}