[PATCH] fix for PR16393: miscompile with struct byval

[Manman Ren]

On Jul 11, 2013, at 9:46 AM, Jakob Stoklund Olesen wrote:

> 
> 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.
Will do.
> 
> 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?).
Stack adjustment should be non-negative.
> 
> +      int Size = I->getOperand(0).getImm();
> 
> getImm() returns an int64_t. Is it safe to assume it fits in an int?
This is what we do in PEI, so I assume it should fit in an int.
Of course, we can add an assert.

> 
> Should we verify that the adjustment is non-negative?
Will do.
> 
> +      // 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.
Will do.
> 
> 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.

I am assuming SPAdj and SPAdjAfter are nonnegative, it saves the absolute amount of adjustment.

If we are working on a destroy, the previous SPAdjAfter should be on a setup.
The new SPAdjAfter should be Size - the previous SPAdjAfter, which should be zero.
If we are working on a setup, the previous SPAdjAfter should be zero and the new SPAdjAfter should be Size.

The above is quite confusing. I am going to switch the definition of SPAdj to be an int, similar to what I do in PEI.

No, we are not allowing nested frame setups. As long as each setup is followed with a destroy with the same amount,
and each destroy is followed with a setup, we don't have nested frame setups.
> 
> +        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.
Will do.

Manman
> 
> Thanks,
> /jakob
>