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

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

rsmith added a comment.

In D37933#872815 <https://reviews.llvm.org/D37933#872815>, @davide wrote:

> So, from what I can see no transformation made for free() is valid in case of `operator delete`.
>  What do you think instead of introducing a predicate `isCallToOperatorDelete()` and bail out at the beginning of `visitFree()` ?


Sounds fine (but we should also check whether the call is a `nobuiltin` call to `free`, which should also not be optimized).



================
Comment at: lib/Analysis/MemoryBuiltins.cpp:351
 
+/// hasSideEffectsFreeCall - Returns true if the call has side effects.
+bool llvm::hasSideEffectsFreeCall(const CallInst *CI,
----------------
davide wrote:
> rsmith wrote:
> > 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.
> Not really happy about the name either, have a better proposal?
Maybe `freeCallHasSideEffects`? (Or, if we revert the sense of this as suggested below, `freeCallHasNoSideEffects`.)


================
Comment at: lib/Analysis/MemoryBuiltins.cpp:352-353
+/// hasSideEffectsFreeCall - Returns true if the call has side effects.
+bool llvm::hasSideEffectsFreeCall(const CallInst *CI,
+                                  const TargetLibraryInfo &TLI) {
+  Function *Callee = CI->getCalledFunction();
----------------
This should presumably also be checking whether the call is `nobuiltin`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D37933/new/

https://reviews.llvm.org/D37933