This is an archive of the discontinued LLVM Phabricator instance.

[ImplicitNullChecks] Uphold an invariant in areMemoryOpsAliased
ClosedPublic

Authored by skatkov on Jun 19 2017, 10:53 PM.

Details

Summary

Right now areMemoryOpsAliased has an assertion justified as:

MMO1 should have a value due it comes from operation we'd like to use
as implicit null check.
assert(MMO1->getValue() && "MMO1 should have a Value!");
However, it is possible for that invariant to not be upheld in the
following situation (conceptually):

Null check %RAX

NotNullSucc:

%RAX = LEA %RSP, 16            // I0
%RDX = MOV64rm %RAX            // I1

With the current code, we will have an early exit from
ImplicitNullChecks::isSuitableMemoryOp on I0 with SR_Unsuitable.
However, I1 will look plausible (since it loads from %RAX) and
will go ahead and call areMemoryOpsAliased(I1, I0). This will cause
us to fail the assert mentioned above since I1 does not load from an
IR level value and thus is allowed to have a non-Value base address.

The fix is to return SR_Impossible whenever we see an unsuitable
instruction overwrite PointerReg. This would guarantee that when we
call areMemoryOpsAliased, we're guaranteed to be looking at an
instruction that loads from or stores to an IR level value.

Diff Detail

Event Timeline

skatkov created this revision.Jun 19 2017, 10:53 PM

This is a duplicate of https://reviews.llvm.org/D33300 with slightly different fix.

sanjoy accepted this revision.Jun 20 2017, 10:35 PM

lgtm

lib/CodeGen/ImplicitNullChecks.cpp
362

s/Check/check/

test/CodeGen/X86/non-value-mem-operand.mir
154

Do we need to keep the CFI_INSTRUCTION s?

This revision is now accepted and ready to land.Jun 20 2017, 10:35 PM
This revision was automatically updated to reflect the committed changes.