Re: [PATCH] gdb/testsuite: change hardcoded assembly in gdb.arch/disp-step-insn-reloc.exp

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> When testing gdb.arch.disp-step-insn-reloc.exp with clang in an x86_64
>> machine, the compiled test case would segfault when returning from
>> the function can_relocate_call, with a suggestion of a broken stack.
>> The example assembly in the commment was the following:
>>
>>    f:
>>      MOV $1, %[ok]
>>      JMP end
>>    set_point0:
>>      CALL f ; tracepoint here.
>>    end:
>
> Honestly, I found the following paragraph really hard to grok.  I had to
> go look at the failure myself to see what was going on.  Don't spend too
> long on it, but it might be possible to make it a little clearer...
>
>>
>> And the segmentation fault happening at the final "ret" instruction of
>> the original function.
>
> s/the original function/the function cal_relocate_call/.
>
>>                         This suggests that gcc's compilation process would
>> realize that no ret instruction ever happened after that call and doesn't
>
> s/after that call and/after the inner call and so/
>
>> save the return address, while clang's process wouldn't.  Looking at the
>
> Maybe:
>
> save the return address, while clang doesn't spot the missing 'ret' and
> so does push the return address onto the stack.  Looking at the
>
>> generated instructions, we can indeed see a difference:
>
>>
>> clang's version: e8 f1 ff ff ff          call   11a4 <can_relocate_call+0x14>
>> gcc's version:   e8 f4 ff ff ff          call   401125 <can_relocate_call+0x11>
>>
>> Notice the difference on the second byte.
>
> Yes, but please, don't expect me to go read the x86-64 ISA manual --
> what does this difference mean?
>
>>
>> Changing the assembly to use "ret" instead of "JMP end" does not change
>> the behavior of the program and guarantees a compiler independent
>> behavior.  This commit does just that.
>
> The change itself is great, and I agree that this shouldn't change the
> test.
>
> Feel free to tweak the commit message and push this.

Actually, I retract this.  Having thought about this a bit more I now
worry that the explanation doesn't really explain what's going on.  Is
there really a call variant that doesn't push the return address?

Even if that was the case, it is gas, not gcc that decides the encoding
of the instruction, and gas isn't analysing the destination to decide if
there's a ret or not.

I suspect you fix is actually the right one, but I think this
explanation is going in the wrong direction.  Maybe if you reword things
it might be clearer what's going on.

Thanks,
Andrew


>
> Approved-By: Andrew Burgess <aburgess@redhat.com>
>
> Thanks,
> Andrew
>
>> ---
>>  gdb/testsuite/gdb.arch/insn-reloc.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.arch/insn-reloc.c b/gdb/testsuite/gdb.arch/insn-reloc.c
>> index f687c2c5631..365e6180057 100644
>> --- a/gdb/testsuite/gdb.arch/insn-reloc.c
>> +++ b/gdb/testsuite/gdb.arch/insn-reloc.c
>> @@ -49,10 +49,9 @@ fail (void)
>>       JMP set_point0
>>     f:
>>       MOV $1, %[ok]
>> -     JMP end
>> +     RET
>>     set_point0:
>>       CALL f ; tracepoint here.
>> -   end:
>>  
>>     */
>>  
>> @@ -65,10 +64,9 @@ can_relocate_call (void)
>>         "  jmp " SYMBOL (set_point0) "\n"
>>         "0:\n"
>>         "  mov $1, %[ok]\n"
>> -       "  jmp 1f\n"
>> +       "  ret\n"
>>         SYMBOL (set_point0) ":\n"
>>         "  call 0b\n"
>> -       "1:\n"
>>         : [ok] "=r" (ok));
>>  
>>    if (ok == 1)
>> -- 
>> 2.39.2