diff mbox series

[1/3] gdb/dwarf: remove redundant read of dwo_name

Message ID 20250325203338.116342-1-simon.marchi@efficios.com
State New
Headers
Series [1/3] gdb/dwarf: remove redundant read of dwo_name |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply

Commit Message

Simon Marchi March 25, 2025, 8:32 p.m. UTC 
  lookup_dwo_unit receives the name of the DWO unit to look up, as read
from the DW_AT_dwo_name attribute of the skeleton DIE.  But then, it
doesn't use it:

    /* Yeah, we look dwo_name up again, but it simplifies the code.  */
    dwo_name = dwarf2_dwo_name (comp_unit_die, cu);

Perhaps this comment made sense at some point, but with the code we have
today, I don't understand it.  It should be fine to use the name passed
as a parameter, which the caller also obtained by calling
dwarf2_dwo_name.

Change-Id: I84723e12726f77e4202d042428ee0eed9962ceb8
---
 gdb/dwarf2/read.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)


base-commit: ebfdb1089bc84fafa7001e7ff2e9c389c11437a6

Comments

Tom Tromey March 26, 2025, 1:50 p.m. UTC | #1 
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> lookup_dwo_unit receives the name of the DWO unit to look up, as read
Simon> from the DW_AT_dwo_name attribute of the skeleton DIE.  But then, it
Simon> doesn't use it:

Simon>     /* Yeah, we look dwo_name up again, but it simplifies the code.  */
Simon>     dwo_name = dwarf2_dwo_name (comp_unit_die, cu);

Simon> Perhaps this comment made sense at some point, but with the code we have
Simon> today, I don't understand it.  It should be fine to use the name passed
Simon> as a parameter, which the caller also obtained by calling
Simon> dwarf2_dwo_name.

Agreed.

I think some of these functions used to be callbacks in the C days and
so it may have been hard to pass an extra bit of data through.

Simon>    dwarf2_per_cu *per_cu = cu->per_cu;
Simon>    struct dwo_unit *dwo_unit;
Simon> -  const char *comp_dir;
Simon> +  const char *comp_dir = dwarf2_string_attr (comp_unit_die, DW_AT_comp_dir, cu);
 
Simon>    gdb_assert (cu != NULL);
 
I don't think this function changed in patch #2, so here I think it
doesn't really make sense to hoist the use of 'cu' before this assert.
Any obvious fix to that seems alright though.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
Simon Marchi March 26, 2025, 2:09 p.m. UTC | #2 
On 3/26/25 9:50 AM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> lookup_dwo_unit receives the name of the DWO unit to look up, as read
> Simon> from the DW_AT_dwo_name attribute of the skeleton DIE.  But then, it
> Simon> doesn't use it:
> 
> Simon>     /* Yeah, we look dwo_name up again, but it simplifies the code.  */
> Simon>     dwo_name = dwarf2_dwo_name (comp_unit_die, cu);
> 
> Simon> Perhaps this comment made sense at some point, but with the code we have
> Simon> today, I don't understand it.  It should be fine to use the name passed
> Simon> as a parameter, which the caller also obtained by calling
> Simon> dwarf2_dwo_name.
> 
> Agreed.
> 
> I think some of these functions used to be callbacks in the C days and
> so it may have been hard to pass an extra bit of data through.
> 
> Simon>    dwarf2_per_cu *per_cu = cu->per_cu;
> Simon>    struct dwo_unit *dwo_unit;
> Simon> -  const char *comp_dir;
> Simon> +  const char *comp_dir = dwarf2_string_attr (comp_unit_die, DW_AT_comp_dir, cu);
>  
> Simon>    gdb_assert (cu != NULL);
>  
> I don't think this function changed in patch #2, so here I think it
> doesn't really make sense to hoist the use of 'cu' before this assert.
> Any obvious fix to that seems alright though.

Oops I missed that this line used `cu`.  I reverted to the obvious thing
of just deleting the line assigning dwo_name.

> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

Thanks,

Simon
diff mbox series

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ff4f388daca9..dc2f4a747a81 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -2866,8 +2866,12 @@  lookup_dwo_id (struct dwarf2_cu *cu, struct die_info* comp_unit_die)
 }
 
 /* Subroutine of cutu_reader to simplify it.
-   Look up the DWO unit specified by COMP_UNIT_DIE of THIS_CU.
-   Returns NULL if the specified DWO unit cannot be found.  */
+   Look up the DWO unit specified by COMP_UNIT_DIE of CU.
+
+   DWO_NAME is the name (DW_AT_dwo_name) of the DWO unit already read from
+   COMP_UNIT_DIE.
+
+   Returns nullptr if the specified DWO unit cannot be found.  */
 
 static struct dwo_unit *
 lookup_dwo_unit (dwarf2_cu *cu, die_info *comp_unit_die, const char *dwo_name)
@@ -2881,14 +2885,10 @@  lookup_dwo_unit (dwarf2_cu *cu, die_info *comp_unit_die, const char *dwo_name)
 
   dwarf2_per_cu *per_cu = cu->per_cu;
   struct dwo_unit *dwo_unit;
-  const char *comp_dir;
+  const char *comp_dir = dwarf2_string_attr (comp_unit_die, DW_AT_comp_dir, cu);
 
   gdb_assert (cu != NULL);
 
-  /* Yeah, we look dwo_name up again, but it simplifies the code.  */
-  dwo_name = dwarf2_dwo_name (comp_unit_die, cu);
-  comp_dir = dwarf2_string_attr (comp_unit_die, DW_AT_comp_dir, cu);
-
   if (per_cu->is_debug_types)
     dwo_unit = lookup_dwo_type_unit (cu, dwo_name, comp_dir);
   else