[PATCH] fix for PR16393: miscompile with struct byval

[Jakob Stoklund Olesen]

On Jul 10, 2013, at 11:06 PM, Manman Ren <mren at apple.com> wrote:

> 
> 3 patches attached:

Thanks, Manman!

> 1> modify machine verifier to verify the 3 variants

+  const TargetInstrInfo &TII = *MF->getTarget().getInstrInfo();

The MachineVerifier already has a TII member.

+  // FrameSetup and FrameDestroy can have zero adjustment, so we can't tell
+  // whether it is a FrameSetup or FrameDestroy from an integer value.
+  // We use a bool plus an integer to capture the stack state.
+  typedef DenseMap<const MachineBasicBlock*,
+    std::pair<std::pair<bool, int>, std::pair<bool, int> > > StackMap;

Please use structs instead of nested pairs.

You could use -1 to indicate a !IsSetup state, but the bool also works. (It would be invalid to have a negative stack adjustment, right?).

+      int Size = I->getOperand(0).getImm();

getImm() returns an int64_t. Is it safe to assume it fits in an int?

Should we verify that the adjustment is non-negative?

+      // If we see a FrameSetup, now we are expecting a FrameDestroy with the
+      // same value; otherwise we are expecting a FrameSetup.
+      if ((IsSetupAfter && (I->getOpcode() != FrameDestroyOpcode ||
+                            SPAdjAfter != Size)) ||
+          (!IsSetupAfter && I->getOpcode() != FrameSetupOpcode))
+        report("FrameSetup <n> is always followed by a FrameDestroy <n>", I);
+      SPAdjAfter = Size - SPAdjAfter;
+      IsSetupAfter = I->getOpcode() == FrameSetupOpcode;
+    }

This logic is pretty convoluted. Can it be broken into different cases? That is usually required to give better error messages anyway.

That SPAdjAfter calculation looks wrong.

Are you trying to handle nested FrameSetups? I don’t think we should allow that unless it’s actually being used by targets.

+        report("The exit stack state of a predecessor is inconsistent.", MBB);

Please provide more information in the verification reports. Imagine getting the above error message, it doesn’t say what it is talking about. It would also be useful to print out the actual and expected states.

Thanks,
/jakob