[1/5] Remove call to dwarf2_per_objfile::adjust from ranges readers

  dwarf2_per_objfile::adjust applies gdbarch_adjust_dwarf2_addr to an
address, leaving the result unrelocated.  However, this adjustment is
only needed for text-section symbols -- it isn't needed for any sort
of address mapping.  Therefore, these calls can be removed from
read_addrmap_from_aranges and create_addrmap_from_gdb_index.
---
 gdb/dwarf2/aranges.c        | 2 --
 gdb/dwarf2/read-gdb-index.c | 2 --
 2 files changed, 4 deletions(-)
  

Hi Tom,

So, I don't know the details of the DWARF reader too well, so my attempt
to review this patch might be just wasting your time.  But I didn't
really understand what was going on here, so...

Tom Tromey <tom@tromey.com> writes:

> dwarf2_per_objfile::adjust applies gdbarch_adjust_dwarf2_addr to an
> address, leaving the result unrelocated.  However, this adjustment is
> only needed for text-section symbols -- it isn't needed for any sort

I didn't know if the use of the word 'symbols' here was significant.
gdbarch_adjust_dwarf2_addr operates on addresses, and I guess symbol
could be a synonym for address in some contexts.  But the addresses here
do seem to be .text addresses, so clearly there's some important
distinction that I'm not understanding.

> of address mapping.  Therefore, these calls can be removed from

gdbarch_adjust_dwarf2_addr seems to be relevant for .text addresses,
which is what you're handling here, but you are creating a map, but it's
not clear _why_ those addresses wouldn't need to be updated.

Sorry to ask what are probably obvious questions...

Thanks,
Andrew

> read_addrmap_from_aranges and create_addrmap_from_gdb_index.
> ---
>  gdb/dwarf2/aranges.c        | 2 --
>  gdb/dwarf2/read-gdb-index.c | 2 --
>  2 files changed, 4 deletions(-)
>
> diff --git a/gdb/dwarf2/aranges.c b/gdb/dwarf2/aranges.c
> index d577db62726..0d1dc11e27a 100644
> --- a/gdb/dwarf2/aranges.c
> +++ b/gdb/dwarf2/aranges.c
> @@ -190,8 +190,6 @@ read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
>  	      continue;
>  	    }
>  	  ULONGEST end = start + length;
> -	  start = (ULONGEST) per_objfile->adjust ((unrelocated_addr) start);
> -	  end = (ULONGEST) per_objfile->adjust ((unrelocated_addr) end);
>  	  mutable_map->set_empty (start, end - 1, per_cu);
>  	}
>  
> diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
> index 8c0895b9639..cc6361674e8 100644
> --- a/gdb/dwarf2/read-gdb-index.c
> +++ b/gdb/dwarf2/read-gdb-index.c
> @@ -566,8 +566,6 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
>  	  continue;
>  	}
>  
> -      lo = (ULONGEST) per_objfile->adjust ((unrelocated_addr) lo);
> -      hi = (ULONGEST) per_objfile->adjust ((unrelocated_addr) hi);
>        mutable_map.set_empty (lo, hi - 1, per_bfd->get_cu (cu_index));
>      }
>  
>
> -- 
> 2.43.0
  

Andrew Burgess <aburgess@redhat.com> writes:

> Hi Tom,
>
> So, I don't know the details of the DWARF reader too well, so my attempt
> to review this patch might be just wasting your time.  But I didn't
> really understand what was going on here, so...
>
> Tom Tromey <tom@tromey.com> writes:
>
>> dwarf2_per_objfile::adjust applies gdbarch_adjust_dwarf2_addr to an
>> address, leaving the result unrelocated.  However, this adjustment is
>> only needed for text-section symbols -- it isn't needed for any sort
>
> I didn't know if the use of the word 'symbols' here was significant.
> gdbarch_adjust_dwarf2_addr operates on addresses, and I guess symbol
> could be a synonym for address in some contexts.  But the addresses here
> do seem to be .text addresses, so clearly there's some important
> distinction that I'm not understanding.
>
>> of address mapping.  Therefore, these calls can be removed from
>
> gdbarch_adjust_dwarf2_addr seems to be relevant for .text addresses,
> which is what you're handling here, but you are creating a map, but it's
> not clear _why_ those addresses wouldn't need to be updated.

A follow on question.  Looking through gdb/dwarf/ there seem to be
several other places where the addrmap_mutable::set_empty is called, and
in at least some of those places the addresses are still being
adjusted.  E.g.:

  dwarf2_ranges_read
  cooked_indexer::check_bounds
  cooked_indexer::scan_attributes

Why do these not need changing?

Thanks,
Andrew



>
> Sorry to ask what are probably obvious questions...
>
> Thanks,
> Andrew
>
>> read_addrmap_from_aranges and create_addrmap_from_gdb_index.
>> ---
>>  gdb/dwarf2/aranges.c        | 2 --
>>  gdb/dwarf2/read-gdb-index.c | 2 --
>>  2 files changed, 4 deletions(-)
>>
>> diff --git a/gdb/dwarf2/aranges.c b/gdb/dwarf2/aranges.c
>> index d577db62726..0d1dc11e27a 100644
>> --- a/gdb/dwarf2/aranges.c
>> +++ b/gdb/dwarf2/aranges.c
>> @@ -190,8 +190,6 @@ read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
>>  	      continue;
>>  	    }
>>  	  ULONGEST end = start + length;
>> -	  start = (ULONGEST) per_objfile->adjust ((unrelocated_addr) start);
>> -	  end = (ULONGEST) per_objfile->adjust ((unrelocated_addr) end);
>>  	  mutable_map->set_empty (start, end - 1, per_cu);
>>  	}
>>  
>> diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
>> index 8c0895b9639..cc6361674e8 100644
>> --- a/gdb/dwarf2/read-gdb-index.c
>> +++ b/gdb/dwarf2/read-gdb-index.c
>> @@ -566,8 +566,6 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
>>  	  continue;
>>  	}
>>  
>> -      lo = (ULONGEST) per_objfile->adjust ((unrelocated_addr) lo);
>> -      hi = (ULONGEST) per_objfile->adjust ((unrelocated_addr) hi);
>>        mutable_map.set_empty (lo, hi - 1, per_bfd->get_cu (cu_index));
>>      }
>>  
>>
>> -- 
>> 2.43.0
  

Andrew Burgess <aburgess@redhat.com> writes:

> Andrew Burgess <aburgess@redhat.com> writes:
>
>> Hi Tom,
>>
>> So, I don't know the details of the DWARF reader too well, so my attempt
>> to review this patch might be just wasting your time.  But I didn't
>> really understand what was going on here, so...
>>
>> Tom Tromey <tom@tromey.com> writes:
>>
>>> dwarf2_per_objfile::adjust applies gdbarch_adjust_dwarf2_addr to an
>>> address, leaving the result unrelocated.  However, this adjustment is
>>> only needed for text-section symbols -- it isn't needed for any sort
>>
>> I didn't know if the use of the word 'symbols' here was significant.
>> gdbarch_adjust_dwarf2_addr operates on addresses, and I guess symbol
>> could be a synonym for address in some contexts.  But the addresses here
>> do seem to be .text addresses, so clearly there's some important
>> distinction that I'm not understanding.
>>
>>> of address mapping.  Therefore, these calls can be removed from
>>
>> gdbarch_adjust_dwarf2_addr seems to be relevant for .text addresses,
>> which is what you're handling here, but you are creating a map, but it's
>> not clear _why_ those addresses wouldn't need to be updated.
>
> A follow on question.  Looking through gdb/dwarf/ there seem to be
> several other places where the addrmap_mutable::set_empty is called, and
> in at least some of those places the addresses are still being
> adjusted.  E.g.:
>
>   dwarf2_ranges_read
>   cooked_indexer::check_bounds
>   cooked_indexer::scan_attributes
>
> Why do these not need changing?

And an additional question.  Are lookups in these maps not done via the
two ::find_per_cu functions?  Which are passed, and are documented to
expected an un-adjusted (i.e. have text_section_offset removed) address.

If we don't add text_section_offset in to begin with doesn't that cause
problems?   Or maybe the bit I'm missing is that the two paths you've
changed already are adjusted?

Thanks,
Andrew


>
> Thanks,
> Andrew
>
>
>
>>
>> Sorry to ask what are probably obvious questions...
>>
>> Thanks,
>> Andrew
>>
>>> read_addrmap_from_aranges and create_addrmap_from_gdb_index.
>>> ---
>>>  gdb/dwarf2/aranges.c        | 2 --
>>>  gdb/dwarf2/read-gdb-index.c | 2 --
>>>  2 files changed, 4 deletions(-)
>>>
>>> diff --git a/gdb/dwarf2/aranges.c b/gdb/dwarf2/aranges.c
>>> index d577db62726..0d1dc11e27a 100644
>>> --- a/gdb/dwarf2/aranges.c
>>> +++ b/gdb/dwarf2/aranges.c
>>> @@ -190,8 +190,6 @@ read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
>>>  	      continue;
>>>  	    }
>>>  	  ULONGEST end = start + length;
>>> -	  start = (ULONGEST) per_objfile->adjust ((unrelocated_addr) start);
>>> -	  end = (ULONGEST) per_objfile->adjust ((unrelocated_addr) end);
>>>  	  mutable_map->set_empty (start, end - 1, per_cu);
>>>  	}
>>>  
>>> diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
>>> index 8c0895b9639..cc6361674e8 100644
>>> --- a/gdb/dwarf2/read-gdb-index.c
>>> +++ b/gdb/dwarf2/read-gdb-index.c
>>> @@ -566,8 +566,6 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
>>>  	  continue;
>>>  	}
>>>  
>>> -      lo = (ULONGEST) per_objfile->adjust ((unrelocated_addr) lo);
>>> -      hi = (ULONGEST) per_objfile->adjust ((unrelocated_addr) hi);
>>>        mutable_map.set_empty (lo, hi - 1, per_bfd->get_cu (cu_index));
>>>      }
>>>  
>>>
>>> -- 
>>> 2.43.0
  

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> So, I don't know the details of the DWARF reader too well, so my
Andrew> attempt to review this patch might be just wasting your time.

Not at all.  Thank you for looking at it.

>> dwarf2_per_objfile::adjust applies gdbarch_adjust_dwarf2_addr to an
>> address, leaving the result unrelocated.  However, this adjustment is
>> only needed for text-section symbols -- it isn't needed for any sort

Andrew> I didn't know if the use of the word 'symbols' here was significant.
Andrew> gdbarch_adjust_dwarf2_addr operates on addresses, and I guess symbol
Andrew> could be a synonym for address in some contexts.  But the addresses here
Andrew> do seem to be .text addresses, so clearly there's some important
Andrew> distinction that I'm not understanding.

The basic idea of this series is to note that gdbarch_adjust_dwarf2_addr
is only really needed when computing the address of a full symbol.  In
other cases, it is just extra work.

Part of this observation is from looking at the sole implementation of
this hook.  This is an abstraction violation, of course, and so maybe
the hook ought to be renamed.  Certainly other arches should not use it
-- in fact, even MIPS shouldn't use it, it's a giant hack, and probably
incorrectly implemented to boot (for example, is it intentional that
minsym lookups here examine all objfiles?  Because they do).

Anyway, the MIPS hook implementation looks to see if a given DWARF
symbol is really a MIPS16 (or MicroMIPS) symbol.

However, this information isn't relevant to the first round of DWARF
scanning.  For one thing, in the scanner, symbols don't have addresses.
So, fixing up the address doesn't matter.  Now, text addresses are
needed a little -- to map an address to a CU.  However, the "un-fixed"
address is perfectly suitable for this as well (and these mappings are
done by ranges anyway).

Andrew> A follow on question.  Looking through gdb/dwarf/ there seem to
Andrew> be several other places where the addrmap_mutable::set_empty is
Andrew> called, and in at least some of those places the addresses are
Andrew> still being adjusted.  E.g.:

Andrew>  dwarf2_ranges_read
Andrew>  cooked_indexer::check_bounds
Andrew>  cooked_indexer::scan_attributes

Andrew> Why do these not need changing?

These are all changed in patch #2.

Andrew> And an additional question.  Are lookups in these maps not done
Andrew> via the two ::find_per_cu functions?  Which are passed, and are
Andrew> documented to expected an un-adjusted (i.e. have
Andrew> text_section_offset removed) address.

Andrew> If we don't add text_section_offset in to begin with doesn't
Andrew> that cause problems?  Or maybe the bit I'm missing is that the
Andrew> two paths you've changed already are adjusted?

Yes, the lookups might be done via find_per_cu.  And yes, this takes an
unrelocated address.

The current code labors under the misapprehension that this gdbarch
transform is meaningful to the index.  IIRC the previous psymtab code
had this same incorrect idea, and so I preserved it in the cooked
indexer without looking too deeply into it.

Whether the addresses are relocated or not is a bit of a red herring.
The 'adjust' method took this into account, by applying the runtime
offset, calling the gdbarch method, and then un-applying the offset.

However, I believe there's just no need to call this arch hook at all in
the indexes -- only for full symbols, where it affects the MIPS ABI.


I hope this helps.  And if not, please let me know and I can try some
more.  I feel like I'm not explaining it very well.

thanks,
Tom