Re: [PATCH] [PR target/83641] Fix incorrect CFI for stack clash protected noreturn function on x86/x86_64

[Jeff Law <law at redhat dot com>]

On 01/02/2018 04:22 PM, Jeff Law wrote:
> On 01/02/2018 03:05 PM, Florian Weimer wrote:
>> On 01/02/2018 09:02 PM, Jeff Law wrote:
>>>     * config/i386/i386.c (ix86_adjut_stack_and_probe_stack_clash): Do not
>>
>> Typo: “adjut”.
>>
>>>     explicitly probe *sp in a noreturn function if there were any callee
>>>     register saves.
>>
>> I recompiled glibc with this patch, and I can confirm it fixes the new
>> glibc test:
>>
>>   https://sourceware.org/ml/libc-alpha/2017-12/msg00987.html
>>
>> However, I would appreciate if it were possible to avoid emitting the
>> .cfi_offset/.cfi_register annotations and only record the change of
>> frame address.  The other CFI notes aren't needed, and it would avoid
>> reintroducing this bug if the way the prologue is constructed changes
>> and the condition for emitting the probe is not completely correct anymore.
> I'm not aware of a way to do that.  I'm not even sure having the ability
> to tell the CFI machinery to avoid that stuff is a good idea from a
> design/implementation standpoint.
So Jakub pointed out in IRC we can use a REG_FRAME_RELATED_EXPR note to
control more precisely what cfis are emitted.  But that introduces its
own set of issues.   I suspect we're more likely to muck up generating
the note explicitly than we are to muck up the condition controlling
whether or not we need the explicit *sp probe.

The way we run into problems is if the CFI notes do not reflect reality
-- for example, indicating a register is saved at a location where it
isn't actually saved, indicating a register was restored to its entry
value when that's not true throughout the function, or mucking up the
CFA offset.

The way to get into those first two states is if we have a redundant
push/pop pair and its associated notes for the *sp explicit probe,
particularly for %esi on x86 or %rax on x86_64.  An additional comment
seems warranted though.  And testing that we don't have a cfi restore in
the regression test also seems warranted.  Consider those added :-)
I'll come up with the exact comment text in the AM.  While avoiding the
cfi notes seems useful, I doubt it's really going to improve things in
practice.

In theory if someone completely rewrote the prologue code they could
potentially introduce all kinds of problems, but I don't see how your
suggested change would really help there either.  Comments in the code
as well as tests verifying the output seem to be the way to go here as well.


Jeff