Re: [PATCH] [RFC] PR target/52813 and target/11807

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Bernd Edlinger]

On 12/17/18 7:46 PM, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> On Mon, Dec 17, 2018 at 11:47:42AM +0000, Richard Sandiford wrote:
>>> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>>>> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>>>>> Hi,
>>>>>
>>>>> if I understood that right, then clobbering sp is and has always been
>>>>> ignored.
>>>
>>> PR77904 was about the clobber not being ignored, so the behaviour
>>> hasn't been consistent.
>>>
>>> I'm also not sure it was always ignored in recent sources.  The clobber
>>> does get added to the associated rtl insn, and it'd be surprising if
>>> that never had an effect.
>>
>> Yes, you will usually get a frame pointer.  My point was that the epilogue
>> will restore your stack pointer both with and without the asm clobber.
> 
> I'm not confident that's the only effect though.
> 
> Also, we didn't use a frame in PR77904, and using a frame would have
> been the wrong thing to do.
> 
>>> I don't think there's a good reason to treat this differently from the
>>> preexisting PIC register error.  If the argument for making it a warning
>>> rather than an error is that the asm might happen to work by accident,
>>> then the same is true for the PIC register.
>>
>> Yes.  As well as quite a few more registers, many of those specific to
>> the target.  And there are many more things you can do terribly wrong in
>> inline assembler, of course, most of which we can never detect.
> 
> Right.  And I don't think anyone's suggesting GCC can detect everything.
> It can only police the things it knows about, which include the input,
> output and clobber clauses.
> 
> What makes the PIC register and sp worth special attention is that
> changing their values would in general invalidate other code that GCC
> generates itself.  It's not just about whether the asm has the effect
> the author wanted (whatever that was).
> 
> FWIW, I don't think we should go on a proactive hunt for other registers
> to complain about.
> 

out of curiosity I looked at the clobber statement in gdb/nat/linux-ptrace.c:

           asm volatile ("pushq %0;"
                         ".globl linux_ptrace_test_ret_to_nx_instr;"
                         "linux_ptrace_test_ret_to_nx_instr:"
                         "ret"
                         : : "r" ((uint64_t) (uintptr_t) return_address)
                         : "%rsp", "memory");

it turns out to be a far jump, instruction.  And I wanted to find out what
removing the %rsp clobber actually does.  First there is a technical problem
with that, because gcc does not print an error, it is possbile to compile the
code without the sp clobber, but not to compare the code that would have been
generated if the error would only be a warning.  So I had to undo the patch
in order to see, what the sp clobber actually does, and I think Segher
mentioned that this might have an influence on the frame pointer, that turns
out to be true:

diff  linux-ptrace-spclobber.dis  linux-ptrace-noclobber.dis

449,590c449,593
<  5c0: 55                      push   %rbp
<  5c1: 45 31 c9                xor    %r9d,%r9d
<  5c4: 41 b8 ff ff ff ff       mov    $0xffffffff,%r8d
<  5ca: b9 22 00 00 00          mov    $0x22,%ecx
<  5cf: ba 03 00 00 00          mov    $0x3,%edx
<  5d4: be 02 00 00 00          mov    $0x2,%esi
<  5d9: 31 ff                   xor    %edi,%edi
<  5db: 48 89 e5                mov    %rsp,%rbp
<  5de: 41 57                   push   %r15
<  5e0: 41 56                   push   %r14
<  5e2: 41 55                   push   %r13
<  5e4: 41 54                   push   %r12
<  5e6: 53                      push   %rbx
<  5e7: 48 81 ec f8 00 00 00    sub    $0xf8,%rsp
[snip]
---
>  5c0: 41 56                   push   %r14
>  5c2: 45 31 c9                xor    %r9d,%r9d
>  5c5: 41 b8 ff ff ff ff       mov    $0xffffffff,%r8d
>  5cb: b9 22 00 00 00          mov    $0x22,%ecx
>  5d0: 41 55                   push   %r13
>  5d2: ba 03 00 00 00          mov    $0x3,%edx
>  5d7: be 02 00 00 00          mov    $0x2,%esi
>  5dc: 31 ff                   xor    %edi,%edi
>  5de: 41 54                   push   %r12
>  5e0: 55                      push   %rbp
>  5e1: 53                      push   %rbx
>  5e2: 48 81 ec f0 00 00 00    sub    $0xf0,%rsp


So for me this looks not at all trivial to see if this
would work without the sp clobber, since the stack frame
might be completely different without that sp clobber.

I wonder what gdb developers think about the sp clobber
here, if it is easy to fix or if it gives trouble to them.


Thanks
Bernd.

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Dimitar Dimitrov]

On Mon, Dec 17 2018 20:15:02 EET Bernd Edlinger wrote:
> out of curiosity I looked at the clobber statement in
> gdb/nat/linux-ptrace.c:
> 
>            asm volatile ("pushq %0;"
>                          ".globl linux_ptrace_test_ret_to_nx_instr;"
>                          "linux_ptrace_test_ret_to_nx_instr:"
>                          "ret"
>                          : : "r" ((uint64_t) (uintptr_t) return_address)
>                          : "%rsp", "memory");
> 
> it turns out to be a far jump, instruction.

GDB functionality should not be affected if SP clobber is removed, even if the 
generated code is slightly different. Please see this comment:
http://sourceware.org/ml/gdb-patches/2018-12/msg00204.html

As I understand it, this particular code is never meant to return. It should 
either stop due to the NX mapping of return_address/%0, or hit the breakpoint 
placed at return_address/%0.

Regards,
Dimitar

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

On Wed, Dec 19, 2018 at 08:40:13AM +0200, Dimitar Dimitrov wrote:
> On Mon, Dec 17 2018 20:15:02 EET Bernd Edlinger wrote:
> > out of curiosity I looked at the clobber statement in
> > gdb/nat/linux-ptrace.c:
> > 
> >            asm volatile ("pushq %0;"
> >                          ".globl linux_ptrace_test_ret_to_nx_instr;"
> >                          "linux_ptrace_test_ret_to_nx_instr:"
> >                          "ret"
> >                          : : "r" ((uint64_t) (uintptr_t) return_address)
> >                          : "%rsp", "memory");
> > 
> > it turns out to be a far jump, instruction.
> 
> GDB functionality should not be affected if SP clobber is removed, even if the 
> generated code is slightly different. Please see this comment:
> http://sourceware.org/ml/gdb-patches/2018-12/msg00204.html
> 
> As I understand it, this particular code is never meant to return. It should 
> either stop due to the NX mapping of return_address/%0, or hit the breakpoint 
> placed at return_address/%0.

If it doesn't return it is undefined behaviour, so anything might happen
and that is perfectly alright.

Defining labels is an asm is undefined, too.

Maybe real assembler code is wanted here?  I.e. a .s file.


Segher