[PATCH] [gdb/symtab] Workaround PR gas/31115

[PATCH] [gdb/symtab] Workaround PR gas/31115

[Tom de Vries]

On arm-linux, with gas 2.40, I run into:
...
(gdb) x /i main+8^M
   0x4e1 <main+7>:      vrhadd.u16      d14, d14, d31^M
(gdb) FAIL: gdb.arch/pr25124.exp: disassemble thumb instruction (1st try)
...

This is a regression due to PR gas/31115, which makes gas produce a low_pc
with the thumb bit set (0x4d8 & 0x1):
...
 <1><24>: Abbrev Number: 2 (DW_TAG_subprogram)
    <25>   DW_AT_name        : main
    <29>   DW_AT_external    : 1
    <29>   DW_AT_type        : <0x2f>
    <2a>   DW_AT_low_pc      : 0x4d9
    <2e>   DW_AT_high_pc     : 12
...

The regression was introduced in 2.39, and is also present in 2.40 and 2.41,
and hasn't been fixed yet.

Work around this in read_func_scope, by using gdbarch_addr_bits_remove on
low_pc and high_pc.

Tested on arm-linux and x86_64-linux.

PR tdep/31453
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31453
---
 gdb/dwarf2/cu.c   |  1 +
 gdb/dwarf2/cu.h   |  1 +
 gdb/dwarf2/read.c | 22 ++++++++++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index ae46dd2a9b2..62c0a8a3ede 100644
--- a/gdb/dwarf2/cu.c
+++ b/gdb/dwarf2/cu.c
@@ -42,6 +42,7 @@ dwarf2_cu::dwarf2_cu (dwarf2_per_cu_data *per_cu,
     producer_is_clang (false),
     producer_is_gas_lt_2_38 (false),
     producer_is_gas_2_39 (false),
+    producer_is_gas_ge_2_40 (false),
     processing_has_namespace_info (false)
 {
 }
diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
index ad89228ef8d..58e89960aad 100644
--- a/gdb/dwarf2/cu.h
+++ b/gdb/dwarf2/cu.h
@@ -263,6 +263,7 @@ struct dwarf2_cu
   bool producer_is_clang : 1;
   bool producer_is_gas_lt_2_38 : 1;
   bool producer_is_gas_2_39 : 1;
+  bool producer_is_gas_ge_2_40 : 1;
 
   /* When true, the file that we're processing is known to have
      debugging info for C++ namespaces.  GCC 3.3.x did not produce
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 486be7e4921..34985898b6a 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -148,6 +148,7 @@ static int dwarf2_loclist_block_index;
 static int ada_block_index;
 
 static bool producer_is_gas_lt_2_38 (struct dwarf2_cu *cu);
+static bool producer_is_gas_ge_2_39 (struct dwarf2_cu *cu);
 
 /* Size of .debug_loclists section header for 32-bit DWARF format.  */
 #define LOCLIST_HEADER_SIZE32 12
@@ -9974,6 +9975,15 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
   lowpc = per_objfile->relocate (unrel_low);
   highpc = per_objfile->relocate (unrel_high);
 
+  if (gdbarch_bfd_arch_info (gdbarch)->arch == bfd_arch_arm
+      && producer_is_gas_ge_2_39 (cu))
+    {
+      /* Gas version 2.39 produces DWARF for a Thumb subprogram with a low_pc
+	 attribute with the thumb bit set (PR gas/31115).  Work around this.  */
+      lowpc = gdbarch_addr_bits_remove (gdbarch, lowpc);
+      highpc = gdbarch_addr_bits_remove (gdbarch, highpc);
+    }
+
   /* If we have any template arguments, then we must allocate a
      different sort of symbol.  */
   for (child_die = die->child; child_die; child_die = child_die->sibling)
@@ -11326,6 +11336,7 @@ check_producer (struct dwarf2_cu *cu)
     {
       cu->producer_is_gas_lt_2_38 = major < 2 || (major == 2 && minor < 38);
       cu->producer_is_gas_2_39 = major == 2 && minor == 39;
+      cu->producer_is_gas_ge_2_40 = major > 2 || (major == 2 && minor >= 40);
     }
   else
     {
@@ -11380,6 +11391,17 @@ producer_is_gas_2_39 (struct dwarf2_cu *cu)
   return cu->producer_is_gas_2_39;
 }
 
+/* Return true if CU is produced by GAS 2.39 or later.  */
+
+static bool
+producer_is_gas_ge_2_39 (struct dwarf2_cu *cu)
+{
+  if (!cu->checked_producer)
+    check_producer (cu);
+
+  return cu->producer_is_gas_2_39 || cu->producer_is_gas_ge_2_40;
+}
+
 /* Return the accessibility of DIE, as given by DW_AT_accessibility.
    If that attribute is not available, return the appropriate
    default.  */

base-commit: 164cc86b81de7ab25682a234b978134d08fb3009
-- 
2.35.3

Re: [PATCH] [gdb/symtab] Workaround PR gas/31115

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> This is a regression due to PR gas/31115, which makes gas produce a low_pc
Tom> with the thumb bit set (0x4d8 & 0x1):
Tom> ...
Tom>  <1><24>: Abbrev Number: 2 (DW_TAG_subprogram)
Tom>     <25>   DW_AT_name        : main
Tom>     <29>   DW_AT_external    : 1
Tom>     <29>   DW_AT_type        : <0x2f>
Tom>     <2a>   DW_AT_low_pc      : 0x4d9
Tom>     <2e>   DW_AT_high_pc     : 12
Tom> ...

Tom> The regression was introduced in 2.39, and is also present in 2.40 and 2.41,
Tom> and hasn't been fixed yet.

Tom> Work around this in read_func_scope, by using gdbarch_addr_bits_remove on
Tom> low_pc and high_pc.

Are there ever cases where this bit ought to be set here?

Tom>    lowpc = per_objfile->relocate (unrel_low);
Tom>    highpc = per_objfile->relocate (unrel_high);
 
Tom> +  if (gdbarch_bfd_arch_info (gdbarch)->arch == bfd_arch_arm
Tom> +      && producer_is_gas_ge_2_39 (cu))
Tom> +    {
Tom> +      /* Gas version 2.39 produces DWARF for a Thumb subprogram with a low_pc
Tom> +	 attribute with the thumb bit set (PR gas/31115).  Work around this.  */
Tom> +      lowpc = gdbarch_addr_bits_remove (gdbarch, lowpc);
Tom> +      highpc = gdbarch_addr_bits_remove (gdbarch, highpc);

'relocate' calls gdbarch_adjust_dwarf2_addr.  I wonder if that's a
better approach.

Right now there are a few gdbarch methods here and it's never been clear
to me why they are different or when one should be used or not.  That
is, there's also adjust_dwarf2_line, which is used in several other
spots -- except dwarf_record_line_1 which for some reason calls
gdbarch_addr_bits_remove.

Tom

Re: [PATCH] [gdb/symtab] Workaround PR gas/31115

[Tom de Vries]

On 3/7/24 20:07, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> This is a regression due to PR gas/31115, which makes gas produce a low_pc
> Tom> with the thumb bit set (0x4d8 & 0x1):
> Tom> ...
> Tom>  <1><24>: Abbrev Number: 2 (DW_TAG_subprogram)
> Tom>     <25>   DW_AT_name        : main
> Tom>     <29>   DW_AT_external    : 1
> Tom>     <29>   DW_AT_type        : <0x2f>
> Tom>     <2a>   DW_AT_low_pc      : 0x4d9
> Tom>     <2e>   DW_AT_high_pc     : 12
> Tom> ...
> 
> Tom> The regression was introduced in 2.39, and is also present in 2.40 and 2.41,
> Tom> and hasn't been fixed yet.
> 
> Tom> Work around this in read_func_scope, by using gdbarch_addr_bits_remove on
> Tom> low_pc and high_pc.
> 
> Are there ever cases where this bit ought to be set here?
> 

I don't know.  The only information I have is that it never should be 
the case for arm, which is info I got out of the PR.

The target hook is present for other archs though (hppa, mips, rl78, 
s390, sparc64), and I don't know what the requirements for those are.

Which is why I limited the scope of the fix to the arm architecture.

I also limited the scope of the fix to the known wrong producer, that 
might not be necessary, it's just defensive programming.

> Tom>    lowpc = per_objfile->relocate (unrel_low);
> Tom>    highpc = per_objfile->relocate (unrel_high);
>   
> Tom> +  if (gdbarch_bfd_arch_info (gdbarch)->arch == bfd_arch_arm
> Tom> +      && producer_is_gas_ge_2_39 (cu))
> Tom> +    {
> Tom> +      /* Gas version 2.39 produces DWARF for a Thumb subprogram with a low_pc
> Tom> +	 attribute with the thumb bit set (PR gas/31115).  Work around this.  */
> Tom> +      lowpc = gdbarch_addr_bits_remove (gdbarch, lowpc);
> Tom> +      highpc = gdbarch_addr_bits_remove (gdbarch, highpc);
> 
> 'relocate' calls gdbarch_adjust_dwarf2_addr.  I wonder if that's a
> better approach.
> 

The only arch implementing that is mips, and looking at the 
implementation, it filters out the ISA bit (bit 0) and then sets back it 
if appropriate.

Which is not the thing we want to be doing here for arm.

> Right now there are a few gdbarch methods here and it's never been clear
> to me why they are different or when one should be used or not.  That
> is, there's also adjust_dwarf2_line, which is used in several other
> spots -- except dwarf_record_line_1 which for some reason calls
> gdbarch_addr_bits_remove.

As for adjust_dwarf2_line, well, the mips implementation (its only 
implementor) is a thin wrapper around adjust_dwarf2_addr that is 
stateful.  To me this looks like a candidate for removal.

Thanks,
- Tom

Re: [PATCH] [gdb/symtab] Workaround PR gas/31115

[Tom de Vries]

On 3/7/24 22:09, Tom de Vries wrote:
> On 3/7/24 20:07, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> This is a regression due to PR gas/31115, which makes gas produce 
>> a low_pc
>> Tom> with the thumb bit set (0x4d8 & 0x1):
>> Tom> ...
>> Tom>  <1><24>: Abbrev Number: 2 (DW_TAG_subprogram)
>> Tom>     <25>   DW_AT_name        : main
>> Tom>     <29>   DW_AT_external    : 1
>> Tom>     <29>   DW_AT_type        : <0x2f>
>> Tom>     <2a>   DW_AT_low_pc      : 0x4d9
>> Tom>     <2e>   DW_AT_high_pc     : 12
>> Tom> ...
>>
>> Tom> The regression was introduced in 2.39, and is also present in 
>> 2.40 and 2.41,
>> Tom> and hasn't been fixed yet.
>>
>> Tom> Work around this in read_func_scope, by using 
>> gdbarch_addr_bits_remove on
>> Tom> low_pc and high_pc.
>>
>> Are there ever cases where this bit ought to be set here?
>>
> 
> I don't know.  The only information I have is that it never should be 
> the case for arm, which is info I got out of the PR.
> 
> The target hook is present for other archs though (hppa, mips, rl78, 
> s390, sparc64), and I don't know what the requirements for those are.
> 
> Which is why I limited the scope of the fix to the arm architecture.
> 
> I also limited the scope of the fix to the known wrong producer, that 
> might not be necessary, it's just defensive programming.
> 
>> Tom>    lowpc = per_objfile->relocate (unrel_low);
>> Tom>    highpc = per_objfile->relocate (unrel_high);
>> Tom> +  if (gdbarch_bfd_arch_info (gdbarch)->arch == bfd_arch_arm
>> Tom> +      && producer_is_gas_ge_2_39 (cu))
>> Tom> +    {
>> Tom> +      /* Gas version 2.39 produces DWARF for a Thumb subprogram 
>> with a low_pc
>> Tom> +     attribute with the thumb bit set (PR gas/31115).  Work 
>> around this.  */
>> Tom> +      lowpc = gdbarch_addr_bits_remove (gdbarch, lowpc);
>> Tom> +      highpc = gdbarch_addr_bits_remove (gdbarch, highpc);
>>
>> 'relocate' calls gdbarch_adjust_dwarf2_addr.  I wonder if that's a
>> better approach.
>>
> 
> The only arch implementing that is mips, and looking at the 
> implementation, it filters out the ISA bit (bit 0) and then sets back it 
> if appropriate.
> 
> Which is not the thing we want to be doing here for arm.
> 
>> Right now there are a few gdbarch methods here and it's never been clear
>> to me why they are different or when one should be used or not.  That
>> is, there's also adjust_dwarf2_line, which is used in several other
>> spots -- except dwarf_record_line_1 which for some reason calls
>> gdbarch_addr_bits_remove.
> 
> As for adjust_dwarf2_line, well, the mips implementation (its only 
> implementor) is a thin wrapper around adjust_dwarf2_addr that is 
> stateful.  To me this looks like a candidate for removal.
> 

I'm planning to commit this tomorrow, any objects?

Thanks,
- Tom

Re: [PATCH] [gdb/symtab] Workaround PR gas/31115

[Luis Machado]

On 3/19/24 09:39, Tom de Vries wrote:
> On 3/7/24 22:09, Tom de Vries wrote:
>> On 3/7/24 20:07, Tom Tromey wrote:
>>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>>
>>> Tom> This is a regression due to PR gas/31115, which makes gas produce a low_pc
>>> Tom> with the thumb bit set (0x4d8 & 0x1):
>>> Tom> ...
>>> Tom>  <1><24>: Abbrev Number: 2 (DW_TAG_subprogram)
>>> Tom>     <25>   DW_AT_name        : main
>>> Tom>     <29>   DW_AT_external    : 1
>>> Tom>     <29>   DW_AT_type        : <0x2f>
>>> Tom>     <2a>   DW_AT_low_pc      : 0x4d9
>>> Tom>     <2e>   DW_AT_high_pc     : 12
>>> Tom> ...
>>>
>>> Tom> The regression was introduced in 2.39, and is also present in 2.40 and 2.41,
>>> Tom> and hasn't been fixed yet.
>>>
>>> Tom> Work around this in read_func_scope, by using gdbarch_addr_bits_remove on
>>> Tom> low_pc and high_pc.
>>>
>>> Are there ever cases where this bit ought to be set here?
>>>
>>
>> I don't know.  The only information I have is that it never should be the case for arm, which is info I got out of the PR.
>>
>> The target hook is present for other archs though (hppa, mips, rl78, s390, sparc64), and I don't know what the requirements for those are.
>>
>> Which is why I limited the scope of the fix to the arm architecture.
>>
>> I also limited the scope of the fix to the known wrong producer, that might not be necessary, it's just defensive programming.
>>
>>> Tom>    lowpc = per_objfile->relocate (unrel_low);
>>> Tom>    highpc = per_objfile->relocate (unrel_high);
>>> Tom> +  if (gdbarch_bfd_arch_info (gdbarch)->arch == bfd_arch_arm
>>> Tom> +      && producer_is_gas_ge_2_39 (cu))
>>> Tom> +    {
>>> Tom> +      /* Gas version 2.39 produces DWARF for a Thumb subprogram with a low_pc
>>> Tom> +     attribute with the thumb bit set (PR gas/31115).  Work around this.  */
>>> Tom> +      lowpc = gdbarch_addr_bits_remove (gdbarch, lowpc);
>>> Tom> +      highpc = gdbarch_addr_bits_remove (gdbarch, highpc);
>>>
>>> 'relocate' calls gdbarch_adjust_dwarf2_addr.  I wonder if that's a
>>> better approach.
>>>
>>
>> The only arch implementing that is mips, and looking at the implementation, it filters out the ISA bit (bit 0) and then sets back it if appropriate.
>>
>> Which is not the thing we want to be doing here for arm.
>>
>>> Right now there are a few gdbarch methods here and it's never been clear
>>> to me why they are different or when one should be used or not.  That
>>> is, there's also adjust_dwarf2_line, which is used in several other
>>> spots -- except dwarf_record_line_1 which for some reason calls
>>> gdbarch_addr_bits_remove.
>>
>> As for adjust_dwarf2_line, well, the mips implementation (its only implementor) is a thin wrapper around adjust_dwarf2_addr that is stateful.  To me this looks like a candidate for removal.
>>
> 
> I'm planning to commit this tomorrow, any objects?

None from me.