Page MenuHomePhabricator

Prevent InstCombine from miscompiling `operator delete`
Needs ReviewPublic

Authored by davide on Sep 15 2017, 2:42 PM.

Details

Summary

It's not legal inventing calls to operator delete.

Diff Detail

Event Timeline

davide created this revision.Sep 15 2017, 2:42 PM

Side note: instcombine speculates these calls to free using pattern matching.
In my opinion, it shouldn't. Hoisting shouldn't be done in instcombine, and hoisting shouldn't be done on a per-name basis. Presumably, the frontend should attach an attribute to this call to make sure they can be hoisted.
If there's not enough information in the frontend, then the middle-end should compute this information (the attribute) and then some pass should be responsible for the sinking/hoisting.

rsmith added inline comments.Sep 15 2017, 3:06 PM
lib/Analysis/MemoryBuiltins.cpp
351

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.

363–368

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?

393–394

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.

lib/Transforms/InstCombine/InstructionCombining.cpp
2231

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

2242

Wrong for operator delete.

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() ?
Alternatively, we could add a boolean argument to isFreeCall() which doesn't include operator delete (still trying to pick a name for the argument).

I'm leaning towards 1), what do you think?

lib/Analysis/MemoryBuiltins.cpp
351

Not really happy about the name either, have a better proposal?

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).

lib/Analysis/MemoryBuiltins.cpp
351

Maybe freeCallHasSideEffects? (Or, if we revert the sense of this as suggested below, freeCallHasNoSideEffects.)

352–353

This should presumably also be checking whether the call is nobuiltin.