Re: [PATCH] Fix inline frame unwinding breakage

[Tom de Vries <tdevries@suse.de>]

On 24-04-2020 13:37, Luis Machado wrote:
> On 4/24/20 8:08 AM, Tom de Vries wrote:
>> On 24-04-2020 12:58, Luis Machado wrote:
>>> On 4/24/20 7:02 AM, Luis Machado wrote:
>>>> On 4/24/20 6:17 AM, Tom de Vries wrote:
>>>>> On 23-04-2020 19:51, Luis Machado via Gdb-patches wrote:
>>>>>> On 4/22/20 8:22 AM, Luis Machado wrote:
>>>>>>> Hi Andrew,
>>>>>>>
>>>>>>> On 4/22/20 6:37 AM, Andrew Burgess wrote:
>>>>>>>> * Luis Machado via Gdb-patches <gdb-patches@sourceware.org>
>>>>>>>> [2020-04-14 18:38:36 -0300]:
>>>>>>>>
>>>>>>>>> *** re-sending due to the poor choice of characters for the
>>>>>>>>> backtrace
>>>>>>>>> annotations. GIT swallowed parts of it.
>>>>>>>>>
>>>>>>>>> There has been some breakage for aarch64-linux, arm-linux and
>>>>>>>>> s390-linux in
>>>>>>>>> terms of inline frame unwinding. There may be other targets, but
>>>>>>>>> these are
>>>>>>>>> the ones i'm aware of.
>>>>>>>>>
>>>>>>>>> The following testcases started to show numerous failures and
>>>>>>>>> trigger internal
>>>>>>>>> errors in GDB after commit
>>>>>>>>> 1009d92fc621bc4d017029b90a5bfab16e17fde5,
>>>>>>>>> "Find tailcall frames before inline frames".
>>>>>>>>>
>>>>>>>>> gdb.opt/inline-break.exp
>>>>>>>>> gdb.opt/inline-cmds.exp
>>>>>>>>> gdb.python/py-frame-inline.exp
>>>>>>>>> gdb.reverse/insn-reverse.exp
>>>>>>>>>
>>>>>>>>> The internal errors were of this kind:
>>>>>>>>>
>>>>>>>>> binutils-gdb/gdb/frame.c:579: internal-error: frame_id
>>>>>>>>> get_frame_id(frame_info*): Assertion `fi->level == 0' failed.
>>>>>>>>
>>>>>>>> I have also started seeing this assert on RISC-V, and your patch
>>>>>>>> resolves this issue for me, so I'm keen to see this merged.
>>>>>>>
>>>>>>> Great.
>>>>>>>
>>>>>>>>
>>>>>>>> I took a look through and it all looks good to me - is there
>>>>>>>> anything
>>>>>>>> holding this back from being merged?
>>>>>>>
>>>>>>> Not really. I was waiting for an OK before pushing it.
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Andrew
>>>>>>
>>>>>> I've pushed this now. Tromey and Andrew OK-ed it on IRC.
>>>>>
>>>>> This causes at least:
>>>>> ...
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: tailcall: bt
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: tailcall: p i
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: tailcall: p i@entry
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: tailcall: p j
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: tailcall: p j@entry
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: p $sp0 == $sp
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: frame 3
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: down
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: disassemble
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: ambiguous: bt
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: self: bt
>>>>> FAIL: gdb.arch/amd64-entry-value.exp: self: bt debug entry-values
>>>>> FAIL: gdb.arch/amd64-tailcall-cxx.exp: bt
>>>>> FAIL: gdb.arch/amd64-tailcall-noret.exp: bt
>>>>> FAIL: gdb.arch/amd64-tailcall-self.exp: bt
>>>>> ...
>>>>>
>>>>> Looking at the first FAIL, before this commit we have:
>>>>> ...
>>>>> (gdb) PASS: gdb.arch/amd64-entry-value.exp: continue to breakpoint:
>>>>> tailcall: breakhere
>>>>> bt^M
>>>>> #0  d (i=71, i@entry=70, j=73.5, j@entry=72.5) at
>>>>> gdb.arch/amd64-entry-value.cc:34^M
>>>>> #1  0x00000000004006af in c (i=i@entry=7, j=j@entry=7.25) at
>>>>> gdb.arch/amd64-entry-value.cc:47^M
>>>>> #2  0x00000000004006cd in b (i=i@entry=5, j=j@entry=5.25) at
>>>>> gdb.arch/amd64-entry-value.cc:59^M
>>>>> #3  0x0000000000400524 in main () at
>>>>> gdb.arch/amd64-entry-value.cc:229^M
>>>>> (gdb) PASS: gdb.arch/amd64-entry-value.exp: tailcall: bt
>>>>> ...
>>>>> which has now degraded into:
>>>>> ...
>>>>> (gdb) PASS: gdb.arch/amd64-entry-value.exp: continue to breakpoint:
>>>>> tailcall: breakhere
>>>>> bt^M
>>>>> #0  d (i=<optimized out>, j=<optimized out>) at
>>>>> gdb.arch/amd64-entry-value.cc:34^M
>>>>> #1  0x0000000000400524 in main () at
>>>>> gdb.arch/amd64-entry-value.cc:229^M
>>>>> (gdb) FAIL: gdb.arch/amd64-entry-value.exp: tailcall: bt
>>>>> ...
>>>>>
>>>>> Thanks,
>>>>> - Tom
>>>>>
>>>>
>>>> I'll take a look at it.
>>>
>>> Just a quick update... I did a before/after run and the only regression
>>> seems to be from gdb.arch/amd64-entry-value.exp.
>>>
>>> The other failures are still there even after reverting the inline frame
>>> unwinding fix.
>>>
>>> I'll check what's up with the regressed test.
>>>
>>> Could you please confirm this when you have some cycles?
>>
>> Hi,
>>
>> I cannot confirm this.  All these FAILs fail with the patch, and pass
>> with the patch reverted.
>>
>> Looking at amd64-tailcall-cxx.exp, we have normally:
>> ...
>> (gdb) bt^M
>> #0  g (x=x@entry=2) at gdb.arch/amd64-tailcall-cxx1.cc:23^M
>> #1  0x00000000004004e8 in f (x=x@entry=1) at
>> gdb.arch/amd64-tailcall-cxx2.cc:23^M
>> #2  0x00000000004003de in main () at gdb.arch/amd64-tailcall-cxx1.cc:31^M
>> (gdb) PASS: gdb.arch/amd64-tailcall-cxx.exp: bt
>> ...
>> and with the patch:
>> ...
>> (gdb) bt^M
>> #0  g (x=2) at gdb.arch/amd64-tailcall-cxx1.cc:23^M
>> #1  0x00000000004003de in main () at gdb.arch/amd64-tailcall-cxx1.cc:31^M
>> (gdb) FAIL: gdb.arch/amd64-tailcall-cxx.exp: bt
>> ...
>>
>> So, I'd say it looks very similar to the issue in
>> gdb.arch/amd64-entry-value.exp.
>>
>> Thanks,
>> - Tom
>>
> 
> Ok. I double-checked this and I'm still seeing failures for those that i
> mentioned, even with the patch reverted. It may be the case that these
> tests are not supposed to pass (or the testcase has issues) on non-amd64
> targets (running Intel here).
> 

Also Intel here (FWIW: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz).

> I'll work with the testcase that does show the issue. Hopefully a fix
> for that will address all the others, but i may need further confirmation.

Understood.

Can you file a PR for the amd64-tailcall-cxx.exp FAIL that you're seeing
before the patch, and attach the exec?

Thanks,
- Tom