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

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

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