[1/4,gdb/symtab] Fix htab_find_slot call in read_call_site_scope

Message ID 20211001123328.22314-1-tdevries@suse.de
State New
Headers show
Series
  • [1/4,gdb/symtab] Fix htab_find_slot call in read_call_site_scope
Related show

Commit Message

Tom de Vries via Gdb-patches Oct. 1, 2021, 12:33 p.m.
From: Simon Marchi <simon.marchi@polymtl.ca>


In read_call_site_scope we have:
...
  call_site_local.pc = pc;
  slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
...

The call passes a call_site pointer as element.  OTOH, the hashtab is created
using hash_f == core_addr_hash and eq_f == core_addr_eq, so the element
will be accessed through a CORE_ADDR pointer.

This is not wrong (at least in C), given that pc is the first field in
call_site.

Nevertheless, as in call_site_for_pc, make the htab_find_slot call match the
used hash_f and eq_f by using &pc instead:
...
  slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);
...

Tested on x86_64-linux.

Co-Authored-By: Tom de Vries <tdevries@suse.de>
---
 gdb/dwarf2/read.c | 5 ++---
 gdb/gdbtypes.h    | 4 +---
 2 files changed, 3 insertions(+), 6 deletions(-)


base-commit: e4860c08f990675c9b3535ab18cc7ff21e2a5639
-- 
2.26.2

Comments

Tom de Vries via Gdb-patches Oct. 1, 2021, 1:09 p.m. | #1
On 2021-10-01 08:33, Tom de Vries via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>

> 

> In read_call_site_scope we have:

> ...

>   call_site_local.pc = pc;

>   slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);

> ...

> 

> The call passes a call_site pointer as element.  OTOH, the hashtab is created

> using hash_f == core_addr_hash and eq_f == core_addr_eq, so the element

> will be accessed through a CORE_ADDR pointer.

> 

> This is not wrong (at least in C), given that pc is the first field in

> call_site.

> 

> Nevertheless, as in call_site_for_pc, make the htab_find_slot call match the

> used hash_f and eq_f by using &pc instead:

> ...

>   slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);

> ...

> 

> Tested on x86_64-linux.

> 

> Co-Authored-By: Tom de Vries <tdevries@suse.de>

> ---

>  gdb/dwarf2/read.c | 5 ++---

>  gdb/gdbtypes.h    | 4 +---

>  2 files changed, 3 insertions(+), 6 deletions(-)

> 

> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c

> index 00aa64dd0ab..23870c04e74 100644

> --- a/gdb/dwarf2/read.c

> +++ b/gdb/dwarf2/read.c

> @@ -13341,7 +13341,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)

>    struct gdbarch *gdbarch = objfile->arch ();

>    CORE_ADDR pc, baseaddr;

>    struct attribute *attr;

> -  struct call_site *call_site, call_site_local;

> +  struct call_site *call_site;

>    void **slot;

>    int nparams;

>    struct die_info *child_die;

> @@ -13369,8 +13369,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)

>      cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq,

>  					       NULL, &objfile->objfile_obstack,

>  					       hashtab_obstack_allocate, NULL);

> -  call_site_local.pc = pc;

> -  slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);

> +  slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);

>    if (*slot != NULL)

>      {

>        complaint (_("Duplicate PC %s for DW_TAG_call_site "

> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h

> index 2a641122aec..84b751e82e3 100644

> --- a/gdb/gdbtypes.h

> +++ b/gdb/gdbtypes.h

> @@ -1783,9 +1783,7 @@ struct call_site_parameter

>  

>  struct call_site

>    {

> -    /* * Address of the first instruction after this call.  It must be

> -       the first field as we overload core_addr_hash and core_addr_eq

> -       for it.  */


Ah, I had not seen this comment.  So it was on purpose.  Still, I think
that it makes it more confusing than anything.  The patch LGTM.

Simon
Tom de Vries via Gdb-patches Oct. 3, 2021, 7:34 p.m. | #2
On 10/1/21 3:09 PM, Simon Marchi wrote:
> 

> 

> On 2021-10-01 08:33, Tom de Vries via Gdb-patches wrote:

>> From: Simon Marchi <simon.marchi@polymtl.ca>

>>

>> In read_call_site_scope we have:

>> ...

>>   call_site_local.pc = pc;

>>   slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);

>> ...

>>

>> The call passes a call_site pointer as element.  OTOH, the hashtab is created

>> using hash_f == core_addr_hash and eq_f == core_addr_eq, so the element

>> will be accessed through a CORE_ADDR pointer.

>>

>> This is not wrong (at least in C), given that pc is the first field in

>> call_site.

>>

>> Nevertheless, as in call_site_for_pc, make the htab_find_slot call match the

>> used hash_f and eq_f by using &pc instead:

>> ...

>>   slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);

>> ...

>>

>> Tested on x86_64-linux.

>>

>> Co-Authored-By: Tom de Vries <tdevries@suse.de>

>> ---

>>  gdb/dwarf2/read.c | 5 ++---

>>  gdb/gdbtypes.h    | 4 +---

>>  2 files changed, 3 insertions(+), 6 deletions(-)

>>

>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c

>> index 00aa64dd0ab..23870c04e74 100644

>> --- a/gdb/dwarf2/read.c

>> +++ b/gdb/dwarf2/read.c

>> @@ -13341,7 +13341,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)

>>    struct gdbarch *gdbarch = objfile->arch ();

>>    CORE_ADDR pc, baseaddr;

>>    struct attribute *attr;

>> -  struct call_site *call_site, call_site_local;

>> +  struct call_site *call_site;

>>    void **slot;

>>    int nparams;

>>    struct die_info *child_die;

>> @@ -13369,8 +13369,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)

>>      cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq,

>>  					       NULL, &objfile->objfile_obstack,

>>  					       hashtab_obstack_allocate, NULL);

>> -  call_site_local.pc = pc;

>> -  slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);

>> +  slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);

>>    if (*slot != NULL)

>>      {

>>        complaint (_("Duplicate PC %s for DW_TAG_call_site "

>> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h

>> index 2a641122aec..84b751e82e3 100644

>> --- a/gdb/gdbtypes.h

>> +++ b/gdb/gdbtypes.h

>> @@ -1783,9 +1783,7 @@ struct call_site_parameter

>>  

>>  struct call_site

>>    {

>> -    /* * Address of the first instruction after this call.  It must be

>> -       the first field as we overload core_addr_hash and core_addr_eq

>> -       for it.  */

> 

> Ah, I had not seen this comment.  So it was on purpose.  Still, I think

> that it makes it more confusing than anything.  The patch LGTM.


And, this follow-up commit reverts everything except the comment.

Any comments?

Thanks,
- Tom
[gdb/symtab] Add call_site_eq and call_site_hash

In commit b4c919f7525 "[gdb/symtab] Fix htab_find_slot call in
read_call_site_scope" , I removed the comment:
...
It must be the first field as we overload core_addr_hash and core_addr_eq for
it.
...
for field pc of struct call_site.

However, this was not tested, and when indeed moving field pc to the second
location, we run into a testsuite failure in gdb.trace/entry-values.exp.

This is caused by core_addr_eq (the eq_f function for the htab) being
called with a pointer to the pc field (as passed into htab_find_slot) and a
pointer to a hash table element.  Now that pc is no longer the first field,
the pointer to hash table element no longer points to the pc field.

This could be fixed by simply reinstating the comment, but we're trying to
get rid of this kind of tricks that make refactoring more difficult.

Instead, fix this by:
- reverting commit b4c919f7525, apart from the comment removal, such that
  we're passing a pointer to element to htab_find_slot
- updating the htab_find_slot call in compunit_symtab::find_call_site
  in a similar manner
- adding a call_site_eq and call_site_hash, and using these in the hash table
  instead of core_addr_eq and core_addr_hash.

Tested on x86_64-linux, both with and without a trigger patch that moves pc to
the second location in struct call_site.

---
 gdb/dwarf2/read.c |  7 ++++---
 gdb/gdbtypes.h    | 15 +++++++++++++++
 gdb/symtab.c      |  5 ++++-
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 2d4ca08b667..75d6853fd5b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13341,7 +13341,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
   struct gdbarch *gdbarch = objfile->arch ();
   CORE_ADDR pc, baseaddr;
   struct attribute *attr;
-  struct call_site *call_site;
+  struct call_site *call_site, call_site_local;
   void **slot;
   int nparams;
   struct die_info *child_die;
@@ -13366,10 +13366,11 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
   pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc);
 
   if (cu->call_site_htab == NULL)
-    cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq,
+    cu->call_site_htab = htab_create_alloc_ex (16, call_site_hash, call_site_eq,
 					       NULL, &objfile->objfile_obstack,
 					       hashtab_obstack_allocate, NULL);
-  slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);
+  call_site_local.pc = pc;
+  slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
   if (*slot != NULL)
     {
       complaint (_("Duplicate PC %s for DW_TAG_call_site "
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 6d09576208d..8021cb21ecc 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1824,6 +1824,21 @@ struct call_site
     struct call_site_parameter parameter[1];
   };
 
+static inline int
+call_site_eq (const void *a_, const void *b_)
+{
+  const struct call_site *a = (const call_site *)a_;
+  const struct call_site *b = (const call_site *)b_;
+  return core_addr_eq (&a->pc, &b->pc);
+}
+
+static inline hashval_t
+call_site_hash (const void *a_)
+{
+  const struct call_site *a = (const call_site *)a_;
+  return core_addr_hash (&a->pc);
+}
+
 /* The type-specific info for TYPE_CODE_FIXED_POINT types.  */
 
 struct fixed_point_type_info
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 6ec5d95401e..84193ddaae3 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -334,10 +334,13 @@ search_domain_name (enum search_domain e)
 call_site *
 compunit_symtab::find_call_site (CORE_ADDR pc) const
 {
+  struct call_site call_site_local;
   if (m_call_site_htab == nullptr)
     return nullptr;
 
-  void **slot = htab_find_slot (m_call_site_htab, &pc, NO_INSERT);
+  call_site_local.pc = pc;
+  void **slot
+    = htab_find_slot (m_call_site_htab, &call_site_local, NO_INSERT);
   if (slot == nullptr)
     return nullptr;
Tom de Vries via Gdb-patches Oct. 4, 2021, 12:05 p.m. | #3
>> Ah, I had not seen this comment.  So it was on purpose.  Still, I think

>> that it makes it more confusing than anything.  The patch LGTM.

> 

> And, this follow-up commit reverts everything except the comment.

> 

> Any comments?

> 

> Thanks,

> - Tom

> 


Is there a problem with having the lookups done with just the pc?  If we
were to replace this with some C++ hash table, say std::unordered_map, it
would be std::unordered_map<CORE_ADDR /* pc */, call_site>.  So doing
the lookups using just the pc in the htab makes sense to me.

Simon
Tom de Vries via Gdb-patches Oct. 4, 2021, 12:46 p.m. | #4
On 10/4/21 2:05 PM, Simon Marchi wrote:
>>> Ah, I had not seen this comment.  So it was on purpose.  Still, I think

>>> that it makes it more confusing than anything.  The patch LGTM.

>>

>> And, this follow-up commit reverts everything except the comment.

>>

>> Any comments?

>>

>> Thanks,

>> - Tom

>>

> 

> Is there a problem with having the lookups done with just the pc? 


Well, there's the problem that I describe in the commit message.  I
don't known of any other problem.

> If we

> were to replace this with some C++ hash table, say std::unordered_map, it

> would be std::unordered_map<CORE_ADDR /* pc */, call_site>. 


Right, because there's a separation between key and element.

> So doing

> the lookups using just the pc in the htab makes sense to me.


I'm sorry, I don't really understand what you're trying to say here.

Do you agree with the patch?  Do you disagree with the patch.  Are you
suggesting an alternative solution?

Thanks,
- Tom
Tom de Vries via Gdb-patches Oct. 4, 2021, 3:41 p.m. | #5
On 2021-10-04 08:46, Tom de Vries wrote:
> On 10/4/21 2:05 PM, Simon Marchi wrote:

>>>> Ah, I had not seen this comment.  So it was on purpose.  Still, I think

>>>> that it makes it more confusing than anything.  The patch LGTM.

>>>

>>> And, this follow-up commit reverts everything except the comment.

>>>

>>> Any comments?

>>>

>>> Thanks,

>>> - Tom

>>>

>>

>> Is there a problem with having the lookups done with just the pc? 

> 

> Well, there's the problem that I describe in the commit message.  I

> don't known of any other problem.

> 

>> If we

>> were to replace this with some C++ hash table, say std::unordered_map, it

>> would be std::unordered_map<CORE_ADDR /* pc */, call_site>. 

> 

> Right, because there's a separation between key and element.

> 

>> So doing

>> the lookups using just the pc in the htab makes sense to me.

> 

> I'm sorry, I don't really understand what you're trying to say here.

> 

> Do you agree with the patch?  Do you disagree with the patch.  Are you

> suggesting an alternative solution?


My thinking was: why not keep core_addr_eq and core_addr_hash, and pass
a pointer to a CORE_ADDR when calling htab_find_slot.  And drop the
requirement for call_site::pc to be the first member of call_site.  But
I now realize that this may not be a correct use of htab: htab_find
passes entries to the eq func.  So if we have core_addr_eq as the eq
func and htab passes a call_site* entry to core_addr_eq, it won't work.
Now, we don't actually use htab_find, so it would still work for us.
But that would be like setting up a trap for whoever tries to use
htab_find in the future.

So, the patche LGTM.

Simon
Tom de Vries via Gdb-patches Oct. 4, 2021, 4:14 p.m. | #6
On 10/4/21 5:41 PM, Simon Marchi wrote:
> On 2021-10-04 08:46, Tom de Vries wrote:

>> On 10/4/21 2:05 PM, Simon Marchi wrote:

>>>>> Ah, I had not seen this comment.  So it was on purpose.  Still, I think

>>>>> that it makes it more confusing than anything.  The patch LGTM.

>>>>

>>>> And, this follow-up commit reverts everything except the comment.

>>>>

>>>> Any comments?

>>>>

>>>> Thanks,

>>>> - Tom

>>>>

>>>

>>> Is there a problem with having the lookups done with just the pc? 

>>

>> Well, there's the problem that I describe in the commit message.  I

>> don't known of any other problem.

>>

>>> If we

>>> were to replace this with some C++ hash table, say std::unordered_map, it

>>> would be std::unordered_map<CORE_ADDR /* pc */, call_site>. 

>>

>> Right, because there's a separation between key and element.

>>

>>> So doing

>>> the lookups using just the pc in the htab makes sense to me.

>>

>> I'm sorry, I don't really understand what you're trying to say here.

>>

>> Do you agree with the patch?  Do you disagree with the patch.  Are you

>> suggesting an alternative solution?

> 

> My thinking was: why not keep core_addr_eq and core_addr_hash, and pass

> a pointer to a CORE_ADDR when calling htab_find_slot.  And drop the

> requirement for call_site::pc to be the first member of call_site.  But

> I now realize that this may not be a correct use of htab: htab_find

> passes entries to the eq func.  So if we have core_addr_eq as the eq

> func and htab passes a call_site* entry to core_addr_eq, it won't work.

> Now, we don't actually use htab_find, so it would still work for us.

> But that would be like setting up a trap for whoever tries to use

> htab_find in the future.

> 


All find variants (with the explicit exception of
find_empty_slot_for_expand) call eq_f on both an element argument and a
hash table element.  Consequently, if we break first-field-type
compatibility between the two, the element argument must be of the same
type as the hash table element, and the eq function must use hash table
element type.

This is not a future problem, this is an actual problem I ran into when
I tried it out moving the pc to the second field position, and the
problem is described in the commit log.

> So, the patche LGTM.


Ack, thanks for the review, I'll commit.

Thanks,
- Tom
Tom de Vries via Gdb-patches Oct. 4, 2021, 4:34 p.m. | #7
> All find variants (with the explicit exception of

> find_empty_slot_for_expand) call eq_f on both an element argument and a

> hash table element.  Consequently, if we break first-field-type

> compatibility between the two, the element argument must be of the same

> type as the hash table element, and the eq function must use hash table

> element type.

> 

> This is not a future problem, this is an actual problem I ran into when

> I tried it out moving the pc to the second field position, and the

> problem is described in the commit log.


That makes sense.  I read the hashtab code wrong.

Simon

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 00aa64dd0ab..23870c04e74 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13341,7 +13341,7 @@  read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
   struct gdbarch *gdbarch = objfile->arch ();
   CORE_ADDR pc, baseaddr;
   struct attribute *attr;
-  struct call_site *call_site, call_site_local;
+  struct call_site *call_site;
   void **slot;
   int nparams;
   struct die_info *child_die;
@@ -13369,8 +13369,7 @@  read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
     cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq,
 					       NULL, &objfile->objfile_obstack,
 					       hashtab_obstack_allocate, NULL);
-  call_site_local.pc = pc;
-  slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
+  slot = htab_find_slot (cu->call_site_htab, &pc, INSERT);
   if (*slot != NULL)
     {
       complaint (_("Duplicate PC %s for DW_TAG_call_site "
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 2a641122aec..84b751e82e3 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1783,9 +1783,7 @@  struct call_site_parameter
 
 struct call_site
   {
-    /* * Address of the first instruction after this call.  It must be
-       the first field as we overload core_addr_hash and core_addr_eq
-       for it.  */
+    /* Address of the first instruction after this call.  */
 
     CORE_ADDR pc;