diff mbox series

[1/1] gdb: Fix assertion in 'value_primitive_field'

Message ID 20240212124740.2734613-2-stephan.rohr@intel.com
State New
Headers
Series Fix assertion in 'value_primitive_field' |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed

Commit Message

Rohr, Stephan Feb. 12, 2024, 12:47 p.m. UTC 
  From: "Rohr, Stephan" <stephan.rohr@intel.com>

GDB asserts that the data location of a value's field with a dynamic
data location is resolved when fetching the field's value in
'value_primitive_field'.  This assertion was hit because of bogus DWARF
generated by the Intel Fortran compiler.  While that compiler should fix
the DWARF, we prefer GDB to error out here instead, e.g. to allow the
user to continue debugging other parts of the program.
---
 gdb/value.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Tom Tromey Feb. 12, 2024, 5:27 p.m. UTC | #1 
>>>>> "Stephan" == Stephan Rohr <stephan.rohr@intel.com> writes:

Stephan> From: "Rohr, Stephan" <stephan.rohr@intel.com>
Stephan> GDB asserts that the data location of a value's field with a dynamic
Stephan> data location is resolved when fetching the field's value in
Stephan> 'value_primitive_field'.  This assertion was hit because of bogus DWARF
Stephan> generated by the Intel Fortran compiler.  While that compiler should fix
Stephan> the DWARF, we prefer GDB to error out here instead, e.g. to allow the
Stephan> user to continue debugging other parts of the program.

It's hard to be sure in this code, but normally if there's an assert, it
means that there's a requirement that the caller do something.  So maybe
this is a bug somewhere else?

The code does seem self-contradictory though:

      /* We expect an already resolved data location.  */
      gdb_assert (TYPE_DATA_LOCATION (type)->is_constant ());
      /* For dynamic data types defer memory allocation
	 until we actual access the value.  */
      v = value::allocate_lazy (type);

Like how would it be resolved but also possibly dynamic.

But since this is allocating a lazy value, why does is_constant even matter?
What happens if you just remove that assert entirely?

I think some kind of test case here would be good.

Tom
Rohr, Stephan Feb. 13, 2024, 7:44 a.m. UTC | #2 
Hi Tom,

Thanks for the feedback. The bug is in the compiler because of the incorrect
DWARF. I would prefer not to crash the debugger in this case, the debug session
may still be useful for the user.

If we remove the assert, I get an `internal_error` in `value::fetch_lazy ()`:

    gdb/value.c:4024: internal-error: Unexpected lazy value type.

I will add a testcase in a second version of the patch. First, I would appreciate your
feedback if the provided patch is reasonable.

Thanks
Stephan

> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Monday, 12 February 2024 18:28
> To: Rohr, Stephan <stephan.rohr@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb: Fix assertion in 'value_primitive_field'
> 
> >>>>> "Stephan" == Stephan Rohr <stephan.rohr@intel.com> writes:
> 
> Stephan> From: "Rohr, Stephan" <stephan.rohr@intel.com>
> Stephan> GDB asserts that the data location of a value's field with a dynamic
> Stephan> data location is resolved when fetching the field's value in
> Stephan> 'value_primitive_field'.  This assertion was hit because of bogus
> DWARF
> Stephan> generated by the Intel Fortran compiler.  While that compiler should
> fix
> Stephan> the DWARF, we prefer GDB to error out here instead, e.g. to allow
> the
> Stephan> user to continue debugging other parts of the program.
> 
> It's hard to be sure in this code, but normally if there's an assert, it
> means that there's a requirement that the caller do something.  So maybe
> this is a bug somewhere else?
> 
> The code does seem self-contradictory though:
> 
>       /* We expect an already resolved data location.  */
>       gdb_assert (TYPE_DATA_LOCATION (type)->is_constant ());
>       /* For dynamic data types defer memory allocation
> 	 until we actual access the value.  */
>       v = value::allocate_lazy (type);
> 
> Like how would it be resolved but also possibly dynamic.
> 
> But since this is allocating a lazy value, why does is_constant even matter?
> What happens if you just remove that assert entirely?
> 
> I think some kind of test case here would be good.
> 
> Tom
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
diff mbox series

Patch

diff --git a/gdb/value.c b/gdb/value.c
index 8840aa41a33..a67e696ae4f 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3056,7 +3056,9 @@  value::primitive_field (LONGEST offset, int fieldno, struct type *arg_type)
 
       gdb_assert (0 == offset);
       /* We expect an already resolved data location.  */
-      gdb_assert (TYPE_DATA_LOCATION (type)->is_constant ());
+      if (PROP_CONST != TYPE_DATA_LOCATION_KIND (type))
+	error (_("cannot read %s, expected an already resolved data "
+		 "location."), arg_type->field (fieldno).name ());
       /* For dynamic data types defer memory allocation
 	 until we actual access the value.  */
       v = value::allocate_lazy (type);