[PR 31727] Recognize -2 as a tombstone value in .debug_line

[PR 31727] Recognize -2 as a tombstone value in .debug_line

[Dmitry Neverov]

Commit a8caed5d7faa639a1e6769eba551d15d8ddd9510 handled the tombstone
value -1 used by lld (https://reviews.llvm.org/D81784).  The
referenced lld commit also uses the tombstone value -2 for
pre-DWARF-v5
(https://github.com/llvm/llvm-project/commit/e618ccbf431f6730edb6d1467a127c3a52fd57f7).

If not handled, -2 breaks the pc step range calculation and triggers
the assertion:

  gdb/infrun.c:2794: internal-error: resume_1: Assertion
  `pc_in_thread_step_range (pc, tp)' failed.

This commit adds -2 tombstone value and handles it in the same way as -1.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31727
---
 gdb/dwarf2/read.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 6a19064409c..d720f58c8b4 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -17897,8 +17897,8 @@ class lnp_state_machine
      we're processing the end of a sequence.  */
   void record_line (bool end_sequence);
 
-  /* Check ADDRESS is -1, or zero and less than UNRELOCATED_LOWPC, and if true
-     nop-out rest of the lines in this sequence.  */
+  /* Check ADDRESS is -1, -2, or zero and less than UNRELOCATED_LOWPC, and if
+     true nop-out rest of the lines in this sequence.  */
   void check_line_address (struct dwarf2_cu *cu,
 			   const gdb_byte *line_ptr,
 			   unrelocated_addr unrelocated_lowpc,
@@ -18308,13 +18308,16 @@ lnp_state_machine::check_line_address (struct dwarf2_cu *cu,
 				       unrelocated_addr unrelocated_lowpc,
 				       unrelocated_addr address)
 {
-  /* Linkers resolve a symbolic relocation referencing a GC'd function to 0 or
-     -1.  If ADDRESS is 0, ignoring the opcode will err if the text section is
+  /* Linkers resolve a symbolic relocation referencing a GC'd function to 0,
+     -1 or -2 (-2 is used by certain lld versions, see
+     https://github.com/llvm/llvm-project/commit/e618ccbf431f6730edb6d1467a127c3a52fd57f7).
+     If ADDRESS is 0, ignoring the opcode will err if the text section is
      located at 0x0.  In this case, additionally check that if
      ADDRESS < UNRELOCATED_LOWPC.  */
 
   if ((address == (unrelocated_addr) 0 && address < unrelocated_lowpc)
-      || address == (unrelocated_addr) -1)
+      || address == (unrelocated_addr) -1
+      || address == (unrelocated_addr) -2)
     {
       /* This line table is for a function which has been
 	 GCd by the linker.  Ignore it.  PR gdb/12528 */
-- 
2.45.1

[PR 31727] Recognize -2 as a tombstone value in .debug_line

[Dmitry.Neverov]

I ran tests on linux/x64, got the following failures:

FAIL: gdb.base/branch-to-self.exp: single-step: si
FAIL: gdb.base/empty-host-env-vars.exp: env_var_name=HOME: show index-cache directory
FAIL: gdb.python/tui-window.exp: Window was updated
FAIL: gdb.threads/clone-attach-detach.exp: bg attach 1: attach (timeout)
FAIL: gdb.threads/step-over-thread-exit.exp: step_over_mode=displaced: non-stop=on: target-non-stop=on: schedlock=off: cmd=continue: ns_stop_all=0: iter 38: continue (timeout)
FAIL: gdb.tui/empty.exp: src: 90x40: box 1 (ll corner is  , not +)
FAIL: gdb.tui/empty.exp: src-regs: 90x40: box 1 (ll corner is |, not +)
FAIL: gdb.tui/empty.exp: src-regs: 90x40: box 2 (ul corner is |, not +)
FAIL: gdb.tui/empty.exp: asm: 90x40: box 1 (ll corner is  , not +)
FAIL: gdb.tui/empty.exp: asm-regs: 90x40: box 1 (ll corner is |, not +)
FAIL: gdb.tui/empty.exp: asm-regs: 90x40: box 2 (ul corner is |, not +)
FAIL: gdb.tui/empty.exp: split: 90x40: box 1 (ll corner is |, not +)
FAIL: gdb.tui/empty.exp: split: 90x40: box 2 (ul corner is |, not +)
FAIL: gdb.tui/empty.exp: split-regs: 90x40: box 1 (ll corner is |, not +)
FAIL: gdb.tui/empty.exp: split-regs: 90x40: box 2 (ul corner is |, not +)
FAIL: gdb.tui/resize-2.exp: curses width 90
FAIL: gdb.tui/resize-2.exp: gdb width 90
FAIL: gdb.tui/resize-2.exp: prompt redisplays after first resize (timeout)
FAIL: gdb.tui/resize-2.exp: prompt redisplays after second resize (timeout)
FAIL: gdb.tui/resize-2.exp: curses width after resize with TUI disabled
FAIL: gdb.tui/resize-one-line.exp: after resizing: src window shows main
FAIL: gdb.tui/resize.exp: source box after resize (ll corner is  , not +)
FAIL: gdb.tui/resize.exp: lines=2: has prompt
FAIL: gdb.tui/resize.exp: lines=1: has prompt

They seem to be unrelated to the change.

I also checked manually that the patch fixes the assertion in the
project where it was triggered 
(https://sourceware.org/bugzilla/show_bug.cgi?id=31727).

--
Dmitry

Re: [PR 31727] Recognize -2 as a tombstone value in .debug_line

[Tom Tromey]

>>>>> "Dmitry" == Dmitry Neverov <dmitry.neverov@jetbrains.com> writes:

Dmitry> Commit a8caed5d7faa639a1e6769eba551d15d8ddd9510 handled the tombstone
Dmitry> value -1 used by lld (https://reviews.llvm.org/D81784).  The
Dmitry> referenced lld commit also uses the tombstone value -2 for
Dmitry> pre-DWARF-v5
Dmitry> (https://github.com/llvm/llvm-project/commit/e618ccbf431f6730edb6d1467a127c3a52fd57f7).

Dmitry> If not handled, -2 breaks the pc step range calculation and triggers
Dmitry> the assertion:

Dmitry>   gdb/infrun.c:2794: internal-error: resume_1: Assertion
Dmitry>   `pc_in_thread_step_range (pc, tp)' failed.

Dmitry> This commit adds -2 tombstone value and handles it in the same way as -1.

Dmitry> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31727

I wonder if it's possible to write a test case for this using the DWARF
assembler.

If that looks too hard, though, I wouldn't bother, considering that the
-2 approach is gone from lld now.  In that case the patch is OK:
Approved-By: Tom Tromey <tom@tromey.com>

Tom

[PING][PR 31727] Recognize -2 as a tombstone value in .debug_line

[Dmitry.Neverov]

Dmitry.Neverov <dmitry.neverov@jetbrains.com> writes:

> Tom Tromey <tom@tromey.com> writes:
>
>> I wonder if it's possible to write a test case for this using the DWARF
>> assembler.
>
> I suspect it will take me quite some time to understand how to use dwarf
> assembler. If possible I'd commit it without test, at least for now.
>
>> If that looks too hard, though, I wouldn't bother, considering that the
>> -2 approach is gone from lld now.  In that case the patch is OK:
>> Approved-By: Tom Tromey <tom@tromey.com>
>
> Does it mean I can push the patch to master, or someone else will merge
> it?
>
> --
> Dmitry

Sorry, forgot to CC the mailing list. Ping.

Thanks,
Dmitry

Re: [PING][PR 31727] Recognize -2 as a tombstone value in .debug_line

[Tom Tromey]

>>>>> "Dmitry" == Dmitry Neverov <dmitry.neverov@jetbrains.com> writes:

>>> If that looks too hard, though, I wouldn't bother, considering that the
>>> -2 approach is gone from lld now.  In that case the patch is OK:
>>> Approved-By: Tom Tromey <tom@tromey.com>
>> 
>> Does it mean I can push the patch to master, or someone else will merge
>> it?

Dmitry> Sorry, forgot to CC the mailing list. Ping.

Sorry about the delay.

If you get an Approved-By, it means you can apply the tag and then push.

thanks,
Tom

Re: [PR 31727] Recognize -2 as a tombstone value in .debug_line

[Joel Brobecker]

Hi everyone,

I think this patch would be safe to apply to the gdb-15-branch,
wouldn't it?

I don't think the patch itself is critical per se, but having it
in the GDB 15.2 release could be helpful to avoid what looked like
a GDB 15.1 regression which turned out to be caused by a faulty
stub. This is documented in...

   https://sourceware.org/bugzilla/show_bug.cgi?id=31801 

... and that PR has been on our list of PRs to fix for GDB 15
due to the apparent regression aspect. However, the current
conclusion for 31801 is that there is no fix to do in GDB.

The reason why this seemingly unrelated patch below would be helpful
in the context above is that, apparently, the patch below indirectly
hides the issue by making GDB's code going through a different
code path. Two birds in one stone.

OK for Dmitry to backport this patch to gdb-15-branch?

Thank you,


On Sat, Jun 08, 2024 at 10:45:29AM +0200, Dmitry Neverov wrote:
> Commit a8caed5d7faa639a1e6769eba551d15d8ddd9510 handled the tombstone
> value -1 used by lld (https://reviews.llvm.org/D81784).  The
> referenced lld commit also uses the tombstone value -2 for
> pre-DWARF-v5
> (https://github.com/llvm/llvm-project/commit/e618ccbf431f6730edb6d1467a127c3a52fd57f7).
> 
> If not handled, -2 breaks the pc step range calculation and triggers
> the assertion:
> 
>   gdb/infrun.c:2794: internal-error: resume_1: Assertion
>   `pc_in_thread_step_range (pc, tp)' failed.
> 
> This commit adds -2 tombstone value and handles it in the same way as -1.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31727
> ---
>  gdb/dwarf2/read.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 6a19064409c..d720f58c8b4 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -17897,8 +17897,8 @@ class lnp_state_machine
>       we're processing the end of a sequence.  */
>    void record_line (bool end_sequence);
>  
> -  /* Check ADDRESS is -1, or zero and less than UNRELOCATED_LOWPC, and if true
> -     nop-out rest of the lines in this sequence.  */
> +  /* Check ADDRESS is -1, -2, or zero and less than UNRELOCATED_LOWPC, and if
> +     true nop-out rest of the lines in this sequence.  */
>    void check_line_address (struct dwarf2_cu *cu,
>  			   const gdb_byte *line_ptr,
>  			   unrelocated_addr unrelocated_lowpc,
> @@ -18308,13 +18308,16 @@ lnp_state_machine::check_line_address (struct dwarf2_cu *cu,
>  				       unrelocated_addr unrelocated_lowpc,
>  				       unrelocated_addr address)
>  {
> -  /* Linkers resolve a symbolic relocation referencing a GC'd function to 0 or
> -     -1.  If ADDRESS is 0, ignoring the opcode will err if the text section is
> +  /* Linkers resolve a symbolic relocation referencing a GC'd function to 0,
> +     -1 or -2 (-2 is used by certain lld versions, see
> +     https://github.com/llvm/llvm-project/commit/e618ccbf431f6730edb6d1467a127c3a52fd57f7).
> +     If ADDRESS is 0, ignoring the opcode will err if the text section is
>       located at 0x0.  In this case, additionally check that if
>       ADDRESS < UNRELOCATED_LOWPC.  */
>  
>    if ((address == (unrelocated_addr) 0 && address < unrelocated_lowpc)
> -      || address == (unrelocated_addr) -1)
> +      || address == (unrelocated_addr) -1
> +      || address == (unrelocated_addr) -2)
>      {
>        /* This line table is for a function which has been
>  	 GCd by the linker.  Ignore it.  PR gdb/12528 */
> -- 
> 2.45.1
> 

-- 
Joel

Re: [PR 31727] Recognize -2 as a tombstone value in .debug_line

[Dmitry.Neverov]

Joel Brobecker <brobecker@adacore.com> writes:

> OK for Dmitry to backport this patch to gdb-15-branch?

Sure. I should send the patch for gdb-15-branch and get a separate
approval, right?

--
Dmitry

Re: [PR 31727] Recognize -2 as a tombstone value in .debug_line

[Joel Brobecker]

> > OK for Dmitry to backport this patch to gdb-15-branch?
> 
> Sure. I should send the patch for gdb-15-branch and get a separate
> approval, right?

I haven't checked, but I'm hoping the same patch applies cleanly
to the gdb-15-branch, and that the testing doesn't show any regression
there. If that's the case, I don't think it's necessary to resend
the patch (admitedly it wouldn't hurt either).

You are right that separate approval for gdb-15-branch is necessary.
Hoping that Tom sees this message and can confirm it's OK for the
branch. But otherwise, procedurally speaking, any global maintainer
can approve for backport. As release manager, I could do this too,
but I've been away from the day to day and so I want to be careful
with that.

-- 
Joel

Re: [PR 31727] Recognize -2 as a tombstone value in .debug_line

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> You are right that separate approval for gdb-15-branch is necessary.
Joel> Hoping that Tom sees this message and can confirm it's OK for the
Joel> branch.

Sorry about the delay on this.  I agree it's ok for the gdb-15 branch.

Tom