[PATCH] D37933: Prevent InstCombine from miscompiling `operator delete`

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added inline comments.


================
Comment at: lib/Analysis/MemoryBuiltins.cpp:351
 
+/// hasSideEffectsFreeCall - Returns true if the call has side effects.
+bool llvm::hasSideEffectsFreeCall(const CallInst *CI,
----------------
This is a really confusing name. It sounds like it's detecting whether a call is free of side-effects, whereas it's *actually* detecting whether the call is to a `free`-like function that has side-effects.


================
Comment at: lib/Analysis/MemoryBuiltins.cpp:363-368
+  return TLIFn == LibFunc_ZdlPv ||                   // operator delete(void*)
+         TLIFn == LibFunc_ZdaPv ||                   // operator delete[](void*)
+         TLIFn == LibFunc_msvc_delete_ptr32 ||       // operator delete(void*)
+         TLIFn == LibFunc_msvc_delete_ptr64 ||       // operator delete(void*)
+         TLIFn == LibFunc_msvc_delete_array_ptr32 || // operator delete[](void*)
+         TLIFn == LibFunc_msvc_delete_array_ptr64;   // operator delete[](void*)
----------------
I really don't like duplicating this list here, in a way that means we would miscompile again if the list in `isFreeCall` is extended but this list is not. (Notably, this list is already missing cases from `isFreeCall`, and as it happens, the one in `isFreeCall` is also missing cases.)

Perhaps instead changing this function to determine whether a call is known to be side-effect free when given a null pointer, and special-casing the very small number of functions for which that's true, would be a better and more maintainable approach?


================
Comment at: lib/Analysis/MemoryBuiltins.cpp:393-394
   // Check free prototype.
   // FIXME: workaround for PR5130, this will be obsolete when a nobuiltin
   // attribute will exist.
   FunctionType *FTy = Callee->getFunctionType();
----------------
Note that this function returns the wrong value for a `nobuiltin` call, and thus may still miscompile calls to `free` when building with `-fno-builtin=free`.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2231
 
   // free undef -> unreachable.
   if (isa<UndefValue>(Op)) {
----------------
This is wrong too. Either passing `undef` to *any* pointer parameter in a function call results in UB, or you cannot assume that passing `undef` to `::operator delete` results in UB. The user-provided `::operator delete` might ignore its argument, for example.

I don't know if this is the reason, but LLVM miscompiles calls to `operator delete` in cases where it ignores its argument too: https://godbolt.org/g/dvvirQ


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2242
   if (isa<ConstantPointerNull>(Op))
     return eraseInstFromFunction(FI);
 
----------------
Wrong for `operator delete`.


https://reviews.llvm.org/D37933