[PATCH 0/5] Fix race in DWARF reader, 2nd approach

[PATCH 0/5] Fix race in DWARF reader, 2nd approach

[Tom Tromey]

This series is a different approach to fixing the race pointed out in
PR gdb/31261.  The problem there is that the indexer can be running at
the same time that an objfile is relocated, resulting in races in
dwarf2_per_objfile::adjust.

The previous series to fix this problem is here:

https://inbox.sourceware.org/gdb-patches/20240217-dwarf-race-relocate-v1-0-d3d2d908c1e8@tromey.com/

This series is less invasive and works by justifying the eventual
removal of the 'adjust' method.

Regression tested on x86-64 Fedora 38.

I also tested this in a small way on a MIPS executable.  I don't have
access to a machine where I can truly test MIPS16 in its entirety, but
what I did is make an executable where one function was marked
__attribute__((mips16)).  Then, I tested that the symbol side of gdb
still correctly applies the address transform in "break" and
"disassemble" commands with this series applied.

---
Tom Tromey (5):
      Remove call to dwarf2_per_objfile::adjust from ranges readers
      Remove more calls to dwarf2_per_objfile::adjust
      Remove call to dwarf2_per_objfile::adjust from read_call_site_scope
      Remove call to dwarf2_per_objfile::adjust from read_attribute_value
      Remove dwarf2_per_objfile::adjust

 gdb/dwarf2/aranges.c        |  2 --
 gdb/dwarf2/read-gdb-index.c |  2 --
 gdb/dwarf2/read.c           | 45 ++++++++++-----------------------------------
 gdb/dwarf2/read.h           |  4 ----
 4 files changed, 10 insertions(+), 43 deletions(-)
---
base-commit: 12f5356130c2cda10e2589e74a8716563050dccb
change-id: 20240416-dwarf-race-relocate-2-621894b33715

Best regards,
-- 
Tom Tromey <tom@tromey.com>

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

[Tom Tromey]

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(-)

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

[PATCH 2/5] Remove more calls to dwarf2_per_objfile::adjust

[Tom Tromey]

As with the previous patch, this patch removes some calls to
dwarf2_per_objfile::adjust.  These calls are not needed by the cooked
indexer, as it does not create symbols or look up symbols by address.

The call in dwarf2_ranges_read is similarly not needed, as it is only
used to update an addrmap; and in any case I believe this particular
call is only reached by the indexer.
---
 gdb/dwarf2/read.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index b029c49a339..9670f232f05 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -10930,7 +10930,6 @@ dwarf2_ranges_read (unsigned offset, unrelocated_addr *low_return,
 		    unrelocated_addr *high_return, struct dwarf2_cu *cu,
 		    addrmap_mutable *map, void *datum, dwarf_tag tag)
 {
-  dwarf2_per_objfile *per_objfile = cu->per_objfile;
   int low_set = 0;
   unrelocated_addr low = {};
   unrelocated_addr high = {};
@@ -10941,13 +10940,10 @@ dwarf2_ranges_read (unsigned offset, unrelocated_addr *low_return,
     {
       if (map != nullptr)
 	{
-	  unrelocated_addr lowpc;
-	  unrelocated_addr highpc;
-
-	  lowpc = per_objfile->adjust (range_beginning);
-	  highpc = per_objfile->adjust (range_end);
 	  /* addrmap only accepts CORE_ADDR, so we must cast here.  */
-	  map->set_empty ((CORE_ADDR) lowpc, (CORE_ADDR) highpc - 1, datum);
+	  map->set_empty ((CORE_ADDR) range_beginning,
+			  (CORE_ADDR) range_end - 1,
+			  datum);
 	}
 
       /* FIXME: This is recording everything as a low-high
@@ -16004,14 +16000,11 @@ cooked_indexer::check_bounds (cutu_reader *reader)
 			    cu, m_index_storage->get_addrmap (), cu->per_cu);
   if (cu_bounds_kind == PC_BOUNDS_HIGH_LOW && best_lowpc < best_highpc)
     {
-      dwarf2_per_objfile *per_objfile = cu->per_objfile;
-      unrelocated_addr low = per_objfile->adjust (best_lowpc);
-      unrelocated_addr high = per_objfile->adjust (best_highpc);
       /* Store the contiguous range if it is not empty; it can be
 	 empty for CUs with no code.  addrmap requires CORE_ADDR, so
 	 we cast here.  */
-      m_index_storage->get_addrmap ()->set_empty ((CORE_ADDR) low,
-						  (CORE_ADDR) high - 1,
+      m_index_storage->get_addrmap ()->set_empty ((CORE_ADDR) best_lowpc,
+						  (CORE_ADDR) best_highpc - 1,
 						  cu->per_cu);
 
       cu->per_cu->addresses_seen = true;
@@ -16313,13 +16306,10 @@ cooked_indexer::scan_attributes (dwarf2_per_cu_data *scanning_per_cu,
 
 	  if (*high_pc > *low_pc)
 	    {
-	      dwarf2_per_objfile *per_objfile = reader->cu->per_objfile;
-	      unrelocated_addr lo = per_objfile->adjust (*low_pc);
-	      unrelocated_addr hi = per_objfile->adjust (*high_pc);
 	      /* Need CORE_ADDR casts for addrmap.  */
-	      m_index_storage->get_addrmap ()->set_empty ((CORE_ADDR) lo,
-							  (CORE_ADDR) hi - 1,
-							  scanning_per_cu);
+	      m_index_storage->get_addrmap ()->set_empty
+		((CORE_ADDR) *low_pc, (CORE_ADDR) *high_pc - 1,
+		 scanning_per_cu);
 	    }
 	}
 

-- 
2.43.0

[PATCH 3/5] Remove call to dwarf2_per_objfile::adjust from read_call_site_scope

[Tom Tromey]

read_call_site_scope does not need to call 'adjust', because in
general the call site is not a symbol address, but rather just the
address of some particular call.
---
 gdb/dwarf2/read.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 9670f232f05..4168cec5c1a 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -10242,7 +10242,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 		 sect_offset_str (die->sect_off), objfile_name (objfile));
       return;
     }
-  unrelocated_addr pc = per_objfile->adjust (attr->as_address ());
+  unrelocated_addr pc = attr->as_address ();
 
   if (cu->call_site_htab == NULL)
     cu->call_site_htab = htab_create_alloc_ex (16, call_site::hash,
@@ -10417,10 +10417,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 			 "low pc, for referencing DIE %s [in module %s]"),
 		       sect_offset_str (die->sect_off), objfile_name (objfile));
 	  else
-	    {
-	      lowpc = per_objfile->adjust (lowpc);
-	      call_site->target.set_loc_physaddr (lowpc);
-	    }
+	    call_site->target.set_loc_physaddr (lowpc);
 	}
     }
   else

-- 
2.43.0

[PATCH 4/5] Remove call to dwarf2_per_objfile::adjust from read_attribute_value

[Tom Tromey]

Currently, read_attribute_value calls dwarf2_per_objfile::adjust on
any address.  This seems wrong, because the address may not even be in
the text section.

Luckily, this call is also not needed, because read_func_scope calls
'relocate', which does the same work.
---
 gdb/dwarf2/read.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4168cec5c1a..731a645442d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -17024,7 +17024,6 @@ read_attribute_value (const struct die_reader_specs *reader,
       {
 	unrelocated_addr addr = cu_header->read_address (abfd, info_ptr,
 							 &bytes_read);
-	addr = per_objfile->adjust (addr);
 	attr->set_address (addr);
 	info_ptr += bytes_read;
       }

-- 
2.43.0

[PATCH 5/5] Remove dwarf2_per_objfile::adjust

[Tom Tromey]

All the calls to dwarf2_per_objfile::adjust have been removed, so we
can remove this function entirely.
---
 gdb/dwarf2/read.c | 11 -----------
 gdb/dwarf2/read.h |  4 ----
 2 files changed, 15 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 731a645442d..a0930e00ffe 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1208,17 +1208,6 @@ dwarf2_invalid_attrib_class_complaint (const char *arg1, const char *arg2)
 
 /* See read.h.  */
 
-unrelocated_addr
-dwarf2_per_objfile::adjust (unrelocated_addr addr)
-{
-  CORE_ADDR baseaddr = objfile->text_section_offset ();
-  CORE_ADDR tem = (CORE_ADDR) addr + baseaddr;
-  tem = gdbarch_adjust_dwarf2_addr (objfile->arch (), tem);
-  return (unrelocated_addr) (tem - baseaddr);
-}
-
-/* See read.h.  */
-
 CORE_ADDR
 dwarf2_per_objfile::relocate (unrelocated_addr addr)
 {
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 73def88c4c0..7ab61528ab7 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -692,10 +692,6 @@ struct dwarf2_per_objfile
      any that are too old.  */
   void age_comp_units ();
 
-  /* Apply any needed adjustments to ADDR, returning an adjusted but
-     still unrelocated address.  */
-  unrelocated_addr adjust (unrelocated_addr addr);
-
   /* Apply any needed adjustments to ADDR and then relocate the
      address according to the objfile's section offsets, returning a
      relocated address.  */

-- 
2.43.0

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

[Andrew Burgess]

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

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

[Andrew Burgess]

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

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

[Andrew Burgess]

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

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

[Tom Tromey]

>>>>> "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

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

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "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.

What you've written sounds sensible, but mapping it back onto the code
is tough.

That said, my opinion, for what it's worth, is that we should move ahead
and merge this series, and figure out any actual impacts for MIPS
if/when they surface.

It sounds like you don't think the original approach was entirely
correct anyway, so if any issues do crop up, and someone wants to try
and address them, then it sounds like you probably nudge them towards an
alternative approach anyway.

So on the understanding that I can't really comment on what the impact
might be for MIPS, but that it's more important to get this threading
issue fixed:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

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

[Tom Tromey]

Andrew> That said, my opinion, for what it's worth, is that we should move ahead
Andrew> and merge this series, and figure out any actual impacts for MIPS
Andrew> if/when they surface.

Ok, thank you.  I'm going to push this series.

Tom