[PATCH][gdb/build] Fix build with g++-4.8

[PATCH][gdb/build] Fix build with g++-4.8

[Tom de Vries]

Hi,

When building g++-4.8, we run into:
...
src/gdb/dwarf2/read.c:919:5: error: multiple fields in union \
  'partial_die_info::<anonymous union>' initialized
...

This is due to:
...
    union
    {
      struct
      {
       CORE_ADDR lowpc = 0;
       CORE_ADDR highpc = 0;
      };
      ULONGEST ranges_offset;
    };
...

The error looks incorrect, given that only one union member is initialized,
and does not reproduce with newer g++.

Nevertheless, work around this by moving the initialization to a constructor.

[ I considered just removing the initialization, with the idea that access
should be guarded by has_pc_info, but I ran into one failure in the testsuite,
for gdb.base/check-psymtab.exp due to add_partial_symbol using lowpc without
checking has_pc_info. ]

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/build] Fix build with g++-4.8

---
 gdb/dwarf2/read.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d2501c9dd56..00aa64dd0ab 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -920,8 +920,8 @@ struct partial_die_info : public allocate_on_obstack
       /* If HAS_PC_INFO, the PC range associated with this DIE.  */
       struct
       {
-	CORE_ADDR lowpc = 0;
-	CORE_ADDR highpc = 0;
+	CORE_ADDR lowpc;
+	CORE_ADDR highpc;
       };
       /* If HAS_RANGE_INFO, the ranges offset associated with this DIE.  */
       ULONGEST ranges_offset;
@@ -974,6 +974,10 @@ struct partial_die_info : public allocate_on_obstack
       is_dwz = 0;
       spec_is_dwz = 0;
       canonical_name = 0;
+      /* Don't set these using NSDMI (Non-static data member initialisation),
+	 because g++-4.8 will error out.  */
+      lowpc = 0;
+      highpc = 0;
     }
   };
 

Re: [PATCH][gdb/build] Fix build with g++-4.8

[Simon Marchi]

> [ I considered just removing the initialization, with the idea that access
> should be guarded by has_pc_info, but I ran into one failure in the testsuite,
> for gdb.base/check-psymtab.exp due to add_partial_symbol using lowpc without
> checking has_pc_info. ]

Does that mean you've found a bug in add_partial_symbol?

Simon

Re: [PATCH][gdb/build] Fix build with g++-4.8

[Tom de Vries]

On 9/26/21 9:33 PM, Simon Marchi wrote:
>> [ I considered just removing the initialization, with the idea that access
>> should be guarded by has_pc_info, but I ran into one failure in the testsuite,
>> for gdb.base/check-psymtab.exp due to add_partial_symbol using lowpc without
>> checking has_pc_info. ]
> 
> Does that mean you've found a bug in add_partial_symbol?

Good question, I'm not sure.

Basically, we're processing this DIE:
...
 <1><134>: Abbrev Number: 5 (DW_TAG_subprogram)
    <135>   DW_AT_name        : foo
    <139>   DW_AT_decl_file   : 1
    <13a>   DW_AT_decl_line   : 19
    <13b>   DW_AT_prototyped  : 1
    <13b>   DW_AT_type        : <0x12d>
    <13f>   DW_AT_inline      : 3       (declared as inline and inlined)
...
which indeed does not have low_pc/high_pc or ranges, so has_pc_info
remains false, and lowpc remains set to the initialization value of 0,
end we end up with foo at addresss 0:
...
(gdb) maint print psymbols^M
  ...
  Global partial symbols:^M
    `main', function, 0x4004a7^M
  Static partial symbols:^M
    `int', type, 0x0^M
    `foo', function, 0x0^M
...

There actually is an entry:
...
 <2><115>: Abbrev Number: 3 (DW_TAG_inlined_subroutine)
    <116>   DW_AT_abstract_origin: <0x134>
    <11a>   DW_AT_low_pc      : 0x4004ab
    <122>   DW_AT_high_pc     : 0x5
    <12a>   DW_AT_call_file   : 1
    <12b>   DW_AT_call_line   : 27
...
with low_pc/high_pc, but that one is ignored because of being a child of
DW_TAG_subprogram main rather than a top-level DIE.

So, does foo having address 0 cause problems?  I suspect not.  It's
inlined into main, so the address range is covered there.

Then, is it a bug to access pdi->low_pc without has_pc_info == true?  If
so, it's fixed by doing:
...
     case DW_TAG_subprogram:
-      addr = ... pdi->lowpc ... ;
+      if (pdi->has_pc_info)
+        addr = ... pdi->lowpc ... ;
...
with as only effect that the initialization could be removed.

I think addressing this in a more consistent way is to add accessor
functions that can assert when used incorrectly, and perhaps adding an
enum { none, high_low, high_low_non_contiguous, range_offset } to more
precisely encode what kind of information we have.

Thanks,
- Tom

Re: [PATCH][gdb/build] Fix build with g++-4.8

[Simon Marchi]

On 2021-09-27 05:29, Tom de Vries wrote:
> On 9/26/21 9:33 PM, Simon Marchi wrote:
>>> [ I considered just removing the initialization, with the idea that access
>>> should be guarded by has_pc_info, but I ran into one failure in the testsuite,
>>> for gdb.base/check-psymtab.exp due to add_partial_symbol using lowpc without
>>> checking has_pc_info. ]
>>
>> Does that mean you've found a bug in add_partial_symbol?
> 
> Good question, I'm not sure.
> 
> Basically, we're processing this DIE:
> ...
>  <1><134>: Abbrev Number: 5 (DW_TAG_subprogram)
>     <135>   DW_AT_name        : foo
>     <139>   DW_AT_decl_file   : 1
>     <13a>   DW_AT_decl_line   : 19
>     <13b>   DW_AT_prototyped  : 1
>     <13b>   DW_AT_type        : <0x12d>
>     <13f>   DW_AT_inline      : 3       (declared as inline and inlined)
> ...
> which indeed does not have low_pc/high_pc or ranges, so has_pc_info
> remains false, and lowpc remains set to the initialization value of 0,
> end we end up with foo at addresss 0:
> ...
> (gdb) maint print psymbols^M
>   ...
>   Global partial symbols:^M
>     `main', function, 0x4004a7^M
>   Static partial symbols:^M
>     `int', type, 0x0^M
>     `foo', function, 0x0^M
> ...

That looks a little odd to pretend this function has code at address
0.  But if that doesn't have direct unwanted consequences, ok.

> There actually is an entry:
> ...
>  <2><115>: Abbrev Number: 3 (DW_TAG_inlined_subroutine)
>     <116>   DW_AT_abstract_origin: <0x134>
>     <11a>   DW_AT_low_pc      : 0x4004ab
>     <122>   DW_AT_high_pc     : 0x5
>     <12a>   DW_AT_call_file   : 1
>     <12b>   DW_AT_call_line   : 27
> ...
> with low_pc/high_pc, but that one is ignored because of being a child of
> DW_TAG_subprogram main rather than a top-level DIE.
> 
> So, does foo having address 0 cause problems?  I suspect not.  It's
> inlined into main, so the address range is covered there.
> 
> Then, is it a bug to access pdi->low_pc without has_pc_info == true?  If
> so, it's fixed by doing:
> ...
>      case DW_TAG_subprogram:
> -      addr = ... pdi->lowpc ... ;
> +      if (pdi->has_pc_info)
> +        addr = ... pdi->lowpc ... ;
> ...
> with as only effect that the initialization could be removed.
> 
> I think addressing this in a more consistent way is to add accessor
> functions that can assert when used incorrectly, and perhaps adding an
> enum { none, high_low, high_low_non_contiguous, range_offset } to more
> precisely encode what kind of information we have.

Yes, that would make sense.

Simon