[PATCH] gdb: Recognize -1 as a tombstone value in .debug_line

[PATCH] gdb: Recognize -1 as a tombstone value in .debug_line

[Fangrui Song]

LLD from 11 onwards (https://reviews.llvm.org/D81784) uses -1 to
represent a relocation in .debug_line referencing a discarded symbol.
Recognize -1 to fix gdb.base/break-on-linker-gcd-function.exp when the
linker is a newer LLD.

gdb/ChangeLog:

	* dwarf2/read.c (lnp_state_machine::check_line_address): Test -1.
---
 gdb/dwarf2/read.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index b097f624b6..7cf2691ae9 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -20380,9 +20380,13 @@ lnp_state_machine::check_line_address (struct dwarf2_cu *cu,
   /* If ADDRESS < UNRELOCATED_LOWPC then it's not a usable value, it's outside
      the pc range of the CU.  However, we restrict the test to only ADDRESS
      values of zero to preserve GDB's previous behaviour which is to handle
-     the specific case of a function being GC'd by the linker.  */
+     the specific case of a function being GC'd by the linker.
 
-  if (address == 0 && address < unrelocated_lowpc)
+     LLD from 11 onwards (https://reviews.llvm.org/D81784) uses -1 to represent
+     the tombstone value.
+     */
+
+  if ((address == 0 && address < unrelocated_lowpc) || address == (CORE_ADDR)-1)
     {
       /* This line table is for a function which has been
 	 GCd by the linker.  Ignore it.  PR gdb/12528 */
-- 
2.27.0.212.ge8ba1cc988-goog

Re: [PATCH] gdb: Recognize -1 as a tombstone value in .debug_line

[Andrew Burgess]

* Fangrui Song via Gdb-patches <gdb-patches@sourceware.org> [2020-06-30 16:18:42 -0700]:

> LLD from 11 onwards (https://reviews.llvm.org/D81784) uses -1 to
> represent a relocation in .debug_line referencing a discarded symbol.
> Recognize -1 to fix gdb.base/break-on-linker-gcd-function.exp when the
> linker is a newer LLD.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2/read.c (lnp_state_machine::check_line_address): Test -1.
> ---
>  gdb/dwarf2/read.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index b097f624b6..7cf2691ae9 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -20380,9 +20380,13 @@ lnp_state_machine::check_line_address (struct dwarf2_cu *cu,
>    /* If ADDRESS < UNRELOCATED_LOWPC then it's not a usable value, it's outside
>       the pc range of the CU.  However, we restrict the test to only ADDRESS
>       values of zero to preserve GDB's previous behaviour which is to handle
> -     the specific case of a function being GC'd by the linker.  */
> +     the specific case of a function being GC'd by the linker.
>  
> -  if (address == 0 && address < unrelocated_lowpc)
> +     LLD from 11 onwards (https://reviews.llvm.org/D81784) uses -1 to represent
> +     the tombstone value.
> +     */

If I understand this correctly then "tombstone value" here is the same
as "a function being GC'd by the linker".  If that's correct then
could you just use that phrase please as it is clearer, and matches
the terminology already used in the comment.

The trailing '*/' should be on the same line as the end of the
comment, with two whitespace before it, so '...value.  */'.

> +
> +  if ((address == 0 && address < unrelocated_lowpc) || address == (CORE_ADDR)-1)

There should be a whitespace after the cast, so '(CORE_ADDR) -1'.

Thanks,
Andrewx

>      {
>        /* This line table is for a function which has been
>  	 GCd by the linker.  Ignore it.  PR gdb/12528 */
> -- 
> 2.27.0.212.ge8ba1cc988-goog
> 

[PATCH v2] gdb: Recognize -1 as a tombstone value in .debug_line

[Fangrui Song]

[-- Attachment #1: Type: text/plain, Size: 2400 bytes --]

Thanks for review.  Attached PATCH v2 with comment adjustment.

On 2020-07-01, Andrew Burgess wrote:
>* Fangrui Song via Gdb-patches <gdb-patches@sourceware.org> [2020-06-30 16:18:42 -0700]:
>
>> LLD from 11 onwards (https://reviews.llvm.org/D81784) uses -1 to
>> represent a relocation in .debug_line referencing a discarded symbol.
>> Recognize -1 to fix gdb.base/break-on-linker-gcd-function.exp when the
>> linker is a newer LLD.
>>
>> gdb/ChangeLog:
>>
>> 	* dwarf2/read.c (lnp_state_machine::check_line_address): Test -1.
>> ---
>>  gdb/dwarf2/read.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
>> index b097f624b6..7cf2691ae9 100644
>> --- a/gdb/dwarf2/read.c
>> +++ b/gdb/dwarf2/read.c
>> @@ -20380,9 +20380,13 @@ lnp_state_machine::check_line_address (struct dwarf2_cu *cu,
>>    /* If ADDRESS < UNRELOCATED_LOWPC then it's not a usable value, it's outside
>>       the pc range of the CU.  However, we restrict the test to only ADDRESS
>>       values of zero to preserve GDB's previous behaviour which is to handle
>> -     the specific case of a function being GC'd by the linker.  */
>> +     the specific case of a function being GC'd by the linker.
>>
>> -  if (address == 0 && address < unrelocated_lowpc)
>> +     LLD from 11 onwards (https://reviews.llvm.org/D81784) uses -1 to represent
>> +     the tombstone value.
>> +     */
>
>If I understand this correctly then "tombstone value" here is the same
>as "a function being GC'd by the linker".  If that's correct then
>could you just use that phrase please as it is clearer, and matches
>the terminology already used in the comment.

Thanks. Reworded the comment added by
c3b7b696c231416ac90fd9cb7d5ce735b3683356 a bit.

>The trailing '*/' should be on the same line as the end of the
>comment, with two whitespace before it, so '...value.  */'.
>> +
>> +  if ((address == 0 && address < unrelocated_lowpc) || address == (CORE_ADDR)-1)
>
>There should be a whitespace after the cast, so '(CORE_ADDR) -1'.
>
>Thanks,
>Andrewx

clang-format deletes space in `(CORE_ADDR) -1`. I think I need to adjust
some options for my minimal .clang-format ...

% cat .clang-format
BasedOnStyle: GNU

>>      {
>>        /* This line table is for a function which has been
>>  	 GCd by the linker.  Ignore it.  PR gdb/12528 */
>> --
>> 2.27.0.212.ge8ba1cc988-goog
>>

[-- Attachment #2: 0001-gdb-Recognize-1-as-a-tombstone-value-in-.debug_line.patch --]
[-- Type: text/x-diff, Size: 2234 bytes --]

From 709d115624db44d6f1a60488f1038e0552b9d534 Mon Sep 17 00:00:00 2001
From: Fangrui Song <maskray@google.com>
Date: Wed, 1 Jul 2020 10:32:36 -0700
Subject: [PATCH] gdb: Recognize -1 as a tombstone value in .debug_line

LLD from 11 onwards (https://reviews.llvm.org/D81784) uses -1 to
represent a relocation in .debug_line referencing a discarded symbol.
Recognize -1 to fix gdb.base/break-on-linker-gcd-function.exp when the
linker is a newer LLD.

gdb/ChangeLog:

	* dwarf2/read.c (lnp_state_machine::check_line_address): Test -1.
---
 gdb/dwarf2/read.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4622d14a05..a83e225cb6 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -19983,7 +19983,7 @@ class lnp_state_machine
      we're processing the end of a sequence.  */
   void record_line (bool end_sequence);
 
-  /* Check ADDRESS is zero and less than UNRELOCATED_LOWPC and if true
+  /* Check ADDRESS is -1, 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,
@@ -20377,12 +20377,13 @@ lnp_state_machine::check_line_address (struct dwarf2_cu *cu,
 				       const gdb_byte *line_ptr,
 				       CORE_ADDR unrelocated_lowpc, CORE_ADDR address)
 {
-  /* If ADDRESS < UNRELOCATED_LOWPC then it's not a usable value, it's outside
-     the pc range of the CU.  However, we restrict the test to only ADDRESS
-     values of zero to preserve GDB's previous behaviour which is to handle
-     the specific case of a function being GC'd by the linker.  */
+  /* 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
+     located at 0x0. In this case, additionaly check that
+     if ADDRESS < UNRELOCATED_LOWPC.  */
 
-  if (address == 0 && address < unrelocated_lowpc)
+  if ((address == 0 && address < unrelocated_lowpc)
+      || address == (CORE_ADDR) -1)
     {
       /* This line table is for a function which has been
 	 GCd by the linker.  Ignore it.  PR gdb/12528 */
-- 
2.27.0.212.ge8ba1cc988-goog

Re: [PATCH v2] gdb: Recognize -1 as a tombstone value in .debug_line

[Tom Tromey]

>>>>> ">" == Fangrui Song via Gdb-patches <gdb-patches@sourceware.org> writes:

>> clang-format deletes space in `(CORE_ADDR) -1`. I think I need to adjust
>> some options for my minimal .clang-format ...

>> % cat .clang-format
>> BasedOnStyle: GNU

gdb doesn't use clang-format.  I wrote a more comprehensive
.clang-format once, as a test, but in the end I couldn't make it produce
reasonable-looking output.  In particular, long function calls were
reformatted illegibly.

>> +  /* 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

Two spaces after the period.

>> +     located at 0x0. In this case, additionaly check that

Same here.  Also a typo, should be "additionally".

This is ok with these fixed.  Thank you for the patch.

Tom