[PATCH] DWZ aborted "write_types: Assertion `ref && ref->die_dup == NULL'"

[PATCH] DWZ aborted "write_types: Assertion `ref && ref->die_dup == NULL'"

[Sharma, Alok Kumar]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

Hi all,

Would you please review the attached patch containing modification of assert condition.
It was required to fix an abort which seem to be incorrect.

There is an assert condition as "ref && ref->die_dup == NULL".
As per definition of "struct dw_die", the structure fields starting
from 'die_dup' are present only if 'die_toplevel' is 1. In line with
this at multiple places in code, full/paritial memory is allocated for
pointer of type dw_die.
Ex.  die = pool_alloc (dw_die, offsetof (struct dw_die, die_dup));
Due to this, since memory is not allocated for field die_dup onwards,
it may contain junk values. Macro 'die_safe_dupe' must be used in place
of directly accessing 'die_dup' field whenever required.

      * dwz.c (write_types): Use 'die_safe_dup' to access field 'die_dup'.


Regards,
Alok

[-- Attachment #2: 0001-DWZ-aborted-write_types-Assertion-ref-ref-die_dup-NU.patch --]
[-- Type: application/octet-stream, Size: 1454 bytes --]

From a3274a19e5b73b7e41e74f6ddfcdd2073f83ab63 Mon Sep 17 00:00:00 2001
From: Alok Kumar Sharma <AlokKumar.Sharma@amd.com>
Date: Mon, 1 Feb 2021 23:57:51 +0530
Subject: [PATCH] DWZ aborted "write_types: Assertion `ref && ref->die_dup ==
 NULL'"

There is an assert condition as "ref && ref->die_dup == NULL".
As per definition of "struct dw_die", the structure fields starting
from 'die_dup' are present only if 'die_toplevel' is 1. In line with
this at multiple places in code, full/paritial memory is allocated for
pointer of type dw_die.
Ex.  die = pool_alloc (dw_die, offsetof (struct dw_die, die_dup));
Due to this, since memory is not allocated for field die_dup onwards,
it may contain junk values. Macro 'die_safe_dupe' must be used in place
of directly accessing 'die_dup' in assert condition here.

      * dwz.c (write_types): Use 'die_safe_dup' to access field 'die_dup'.
---
 dwz.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dwz.c b/dwz.c
index 1db93dd..920a9b8 100644
--- a/dwz.c
+++ b/dwz.c
@@ -12868,7 +12868,7 @@ write_types (void)
       memcpy (ptr, inptr - 8, 8);
       ptr += 8;
       ref = off_htab_lookup (cu, cu->cu_offset + read_32 (inptr));
-      assert (ref && ref->die_dup == NULL);
+      assert (ref && die_safe_dup(ref) == NULL);
       write_32 (ptr, ref->u.p2.die_new_offset);
       ptr = write_die (ptr, cu, cu->cu_die, NULL, NULL, NULL);
       assert (types + next_off == ptr);
-- 
2.17.1

Re: [PATCH] DWZ aborted "write_types: Assertion `ref && ref->die_dup == NULL'"

[Mark Wielaard]

Hi Alok,

On Fri, 2021-02-05 at 06:09 +0000, Sharma, Alok Kumar via Dwz wrote:
> Would you please review the attached patch containing modification of
> assert condition.
> It was required to fix an abort which seem to be incorrect.
> 
> There is an assert condition as "ref && ref->die_dup == NULL".
> As per definition of "struct dw_die", the structure fields starting
> from 'die_dup' are present only if 'die_toplevel' is 1. In line with
> this at multiple places in code, full/paritial memory is allocated for
> pointer of type dw_die.
> Ex.  die = pool_alloc (dw_die, offsetof (struct dw_die, die_dup));
> Due to this, since memory is not allocated for field die_dup onwards,
> it may contain junk values. Macro 'die_safe_dupe' must be used in place
> of directly accessing 'die_dup' field whenever required.
> 
>       * dwz.c (write_types): Use 'die_safe_dup' to access field 'die_dup'.

I think you analysis and the code fix are correct. Thanks.

This was probably missed before because -fdebug-types-section isn't the
default, so we are not seeing many DWARF files with debug types.

Do you have an example where this triggers?

Thanks,

Mark

RE: [PATCH] DWZ aborted "write_types: Assertion `ref && ref->die_dup == NULL'"

[George, Jini Susan]

[AMD Public Use]

Hi Mark, 

This was triggering with the gdb test case gdb.dwarf2/dw4-sig-types.exp  when compiled with clang. 

Thanks,
Jini.

-----Original Message-----
From: Mark Wielaard <mark@klomp.org> 
Sent: Friday, February 5, 2021 6:39 PM
To: Sharma, Alok Kumar <AlokKumar.Sharma@amd.com>; dwz@sourceware.org
Cc: George, Jini Susan <JiniSusan.George@amd.com>; E, Nagajyothi <Nagajyothi.E@amd.com>; Achra, Nitika <Nitika.Achra@amd.com>
Subject: Re: [PATCH] DWZ aborted "write_types: Assertion `ref && ref->die_dup == NULL'"

[CAUTION: External Email]

Hi Alok,

On Fri, 2021-02-05 at 06:09 +0000, Sharma, Alok Kumar via Dwz wrote:
> Would you please review the attached patch containing modification of 
> assert condition.
> It was required to fix an abort which seem to be incorrect.
>
> There is an assert condition as "ref && ref->die_dup == NULL".
> As per definition of "struct dw_die", the structure fields starting 
> from 'die_dup' are present only if 'die_toplevel' is 1. In line with 
> this at multiple places in code, full/paritial memory is allocated for 
> pointer of type dw_die.
> Ex.  die = pool_alloc (dw_die, offsetof (struct dw_die, die_dup)); Due 
> to this, since memory is not allocated for field die_dup onwards, it 
> may contain junk values. Macro 'die_safe_dupe' must be used in place 
> of directly accessing 'die_dup' field whenever required.
>
>       * dwz.c (write_types): Use 'die_safe_dup' to access field 'die_dup'.

I think you analysis and the code fix are correct. Thanks.

This was probably missed before because -fdebug-types-section isn't the default, so we are not seeing many DWARF files with debug types.

Do you have an example where this triggers?

Thanks,

Mark

Re: [PATCH] DWZ aborted "write_types: Assertion `ref && ref->die_dup == NULL'"

[Mark Wielaard]

Hi Jini,

On Fri, Feb 05, 2021 at 01:21:16PM +0000, George, Jini Susan via Dwz wrote:
> This was triggering with the gdb test case
> gdb.dwarf2/dw4-sig-types.exp when compiled with clang.

Thanks, I could replicate it with that.  The difference between g++
and clang++ building that testcase with -gdwarf-4
-fdebug-types-section is that g++ always creates the actual type DIE
as a top-level DIE in the type unit, while clang++ can create a deeper
embedded type DIE. If g++ needs the type to be nested (for example
inside a namespace, it will still create a top-level DIE with a
DW_AT_specification pointing to the nested type. Which is why we never
saw this assert accidentially trigger in the g++ case.

I pushed the patch.

Thanks,

Mark