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

Message ID 20200701173905.7znyf7jzhyatkpqx@google.com
State New
Headers show
Series
  • [v2] gdb: Recognize -1 as a tombstone value in .debug_line
Related show

Commit Message

Jose E. Marchesi via Gdb-patches July 1, 2020, 5:39 p.m.
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

>>

Comments

Tom Tromey July 1, 2020, 7:17 p.m. | #1
>>>>> ">" == 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

Patch

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