[llvm] r228748 - X86: Make @llvm.frameaddress work correctly with Windows unwind codes

[Justin Bogner]

David Majnemer <david.majnemer at gmail.com> writes:
> On Thu, May 14, 2015 at 9:29 PM, Justin Bogner <mail at justinbogner.com> wrote:
>
>     David Majnemer <david.majnemer at gmail.com> writes:
>     > On Tue, Apr 28, 2015 at 7:56 PM, Justin Bogner <mail at justinbogner.com>
>     wrote:
>     >>>> Sorry to dig up an old thread, but I was playing around with ubsan
>     and
>     >>>> it looks like this causes some undefined behaviour. I guess we set
>     the
>     >>>> Offset to INT64_MIN here as a dummy value, and it isn't intended to
>     be
>     >>>> used, but later, in PrologEpilogInserter.cpp, we hit this logic in
>     >>>> PEI::calculateFrameObjectOffsets:
>     >>>>
>     >>>>   if (StackGrowsDown) {
>     >>>>     // The maximum distance from the stack pointer is at lower
>     address of
>     >>>>     // the object -- which is given by offset. For down growing stack
>     >>>>     // the offset is negative, so we negate the offset to get the
>     distance.
>     >>>>     FixedOff = -MFI->getObjectOffset(i);
>     >>>>
>     >>>> Here, when MFI->getObjectOffset returns INT64_MIN and we negate it,
>     we
>     >>>> can't represent the result and thus hit undefined behaviour. It looks
>     >>>> like the effect is mostly harmless in practice, but...
>     >>>>
>     >>>> I guess that 0 is a better dummy value here - all tests continue to
>     pass
>     >>>> if I change INT64_MIN to 0, anyway.
>     >>>>
>     >>>> WDYT?
>     >>>
>     >>> Could we do INT64_MIN+1?
>     >>
>     >> I don't see why not. Is there any particular reason that's better than
>     >> 0? I guess just to make it obvious that it's special?
>     >
>     > Yep, pretty much.
>    
>     Interestingly, setting this to 0 passes all of the tests, but setting it
>     to INT64_MIN+1 does not. That's pretty scary - I guess we're using this
>     value somewhere, somehow...
>
> I'm pretty sure that's because INT64_MIN+1 isn't a multiple of SlotSize.  I
> ended up committing r237474 which just uses 0 for the offset.

Cool. Thanks!