[PATCH 0/2] Fix two crashes with Ada "bt"

[PATCH 0/2] Fix two crashes with Ada "bt"

[Tom Tromey]

This series fixes a couple of crashes when doing "bt full" in a
certain Ada program.

In both cases the cause is somewhat mysterious and difficult to reduce
from the original program, which is quite large (and built with
optimization, etc, further complicating things).

So, I don't have new tests.  However, I think the patches are
relatively straightforward nevertheless.

Let me know what you think.

Tom

[PATCH 1/2] Avoid crash in resolve_dynamic_struct

[Tom Tromey]

resolve_dynamic_struct says:

  gdb_assert (type->num_fields () > 0);

However, a certain Ada program has a structure with no fields but with
a dynamic size, causing this assertion to fire.

It is difficult to be certain, but we think this is a compiler bug.
However, in the meantime this assertion does not seem to be checking
any kind of internal consistency; so this patch removes it.

gdb/ChangeLog
2021-02-08  Tom Tromey  <tromey@adacore.com>

	* gdbtypes.c (resolve_dynamic_struct): Handle structure with no
	fields.
---
 gdb/ChangeLog  | 5 +++++
 gdb/gdbtypes.c | 8 ++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index c736dff2ca8..1b2d4836959 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2545,7 +2545,6 @@ resolve_dynamic_struct (struct type *type,
   unsigned resolved_type_bit_length = 0;
 
   gdb_assert (type->code () == TYPE_CODE_STRUCT);
-  gdb_assert (type->num_fields () > 0);
 
   resolved_type = copy_type (type);
 
@@ -2564,9 +2563,10 @@ resolve_dynamic_struct (struct type *type,
 	((struct field *)
 	 TYPE_ALLOC (resolved_type,
 		     resolved_type->num_fields () * sizeof (struct field)));
-      memcpy (resolved_type->fields (),
-	      type->fields (),
-	      resolved_type->num_fields () * sizeof (struct field));
+      if (type->num_fields () > 0)
+	memcpy (resolved_type->fields (),
+		type->fields (),
+		resolved_type->num_fields () * sizeof (struct field));
     }
 
   for (i = 0; i < resolved_type->num_fields (); ++i)
-- 
2.26.2

[PATCH 2/2] Avoid crash from coerce_unspec_val_to_type

[Tom Tromey]

With a certain Ada program, ada-lang.c:coerce_unspec_val_to_type can
cause a crash.  This function may copy a value, and in the particular
case in the crash, the new value's type is smaller than the original
type.  This causes coerce_unspec_val_to_type to create a lazy value --
but the original value is also not_lval, so later, when the value is
un-lazied, gdb asserts.

As with the previous patch, we believe there is a compiler bug here,
but it is difficult to reproduce, so we're not completely certain.

In the particular case we saw, the original value has record type, and
the record holds some variable-length arrays.  This leads to the
type's length being 0.  At the same time, the value is optimized out.

This patch changes coerce_unspec_val_to_type to handle an
optimized-out value correctly.

It also slightly restructures this code to avoid a crash should a
not_lval value wind up here.  This is a purely defensive change.

This change also made it clear that value_contents_copy_raw can now be
made static, so that is also done.

gdb/ChangeLog
2021-02-08  Tom Tromey  <tromey@adacore.com>

	* ada-lang.c (coerce_unspec_val_to_type): Avoid making lazy
	not_lval value.
	* value.c (value_contents_copy_raw): Now static.
	* value.h (value_contents_copy_raw): Don't declare.
---
 gdb/ChangeLog  |  7 +++++++
 gdb/ada-lang.c | 10 +++++++---
 gdb/value.c    |  2 +-
 gdb/value.h    |  3 ---
 4 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 70296f97797..416a45be58e 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -601,13 +601,17 @@ coerce_unspec_val_to_type (struct value *val, struct type *type)
 	 trying to allocate some memory for it.  */
       ada_ensure_varsize_limit (type);
 
-      if (value_lazy (val)
-	  || TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val)))
+      if (value_optimized_out (val))
+	result = allocate_optimized_out_value (type);
+      else if (value_lazy (val)
+	       /* Be careful not to make a lazy not_lval value.  */
+	       || (VALUE_LVAL (val) != not_lval
+		   && TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val))))
 	result = allocate_value_lazy (type);
       else
 	{
 	  result = allocate_value (type);
-	  value_contents_copy_raw (result, 0, val, 0, TYPE_LENGTH (type));
+	  value_contents_copy (result, 0, val, 0, TYPE_LENGTH (type));
 	}
       set_value_component_location (result, val);
       set_value_bitsize (result, value_bitsize (val));
diff --git a/gdb/value.c b/gdb/value.c
index 4135d5ec339..bddf9a47923 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1304,7 +1304,7 @@ value_ranges_copy_adjusted (struct value *dst, int dst_bit_offset,
    It is assumed the contents of DST in the [DST_OFFSET,
    DST_OFFSET+LENGTH) range are wholly available.  */
 
-void
+static void
 value_contents_copy_raw (struct value *dst, LONGEST dst_offset,
 			 struct value *src, LONGEST src_offset, LONGEST length)
 {
diff --git a/gdb/value.h b/gdb/value.h
index 39e94f45ea6..60a831c38c4 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -739,9 +739,6 @@ extern struct value *allocate_value_lazy (struct type *type);
 extern void value_contents_copy (struct value *dst, LONGEST dst_offset,
 				 struct value *src, LONGEST src_offset,
 				 LONGEST length);
-extern void value_contents_copy_raw (struct value *dst, LONGEST dst_offset,
-				     struct value *src, LONGEST src_offset,
-				     LONGEST length);
 
 extern struct value *allocate_repeat_value (struct type *type, int count);
 
-- 
2.26.2

Re: [PATCH 1/2] Avoid crash in resolve_dynamic_struct

[Andrew Burgess]

* Tom Tromey <tromey@adacore.com> [2021-02-08 08:24:47 -0700]:

> resolve_dynamic_struct says:
> 
>   gdb_assert (type->num_fields () > 0);
> 
> However, a certain Ada program has a structure with no fields but with
> a dynamic size, causing this assertion to fire.
> 
> It is difficult to be certain, but we think this is a compiler bug.
> However, in the meantime this assertion does not seem to be checking
> any kind of internal consistency; so this patch removes it.
> 
> gdb/ChangeLog
> 2021-02-08  Tom Tromey  <tromey@adacore.com>
> 
> 	* gdbtypes.c (resolve_dynamic_struct): Handle structure with no
> 	fields.

Seems reasonable to me.

Thanks,
Andrew


> ---
>  gdb/ChangeLog  | 5 +++++
>  gdb/gdbtypes.c | 8 ++++----
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index c736dff2ca8..1b2d4836959 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -2545,7 +2545,6 @@ resolve_dynamic_struct (struct type *type,
>    unsigned resolved_type_bit_length = 0;
>  
>    gdb_assert (type->code () == TYPE_CODE_STRUCT);
> -  gdb_assert (type->num_fields () > 0);
>  
>    resolved_type = copy_type (type);
>  
> @@ -2564,9 +2563,10 @@ resolve_dynamic_struct (struct type *type,
>  	((struct field *)
>  	 TYPE_ALLOC (resolved_type,
>  		     resolved_type->num_fields () * sizeof (struct field)));
> -      memcpy (resolved_type->fields (),
> -	      type->fields (),
> -	      resolved_type->num_fields () * sizeof (struct field));
> +      if (type->num_fields () > 0)
> +	memcpy (resolved_type->fields (),
> +		type->fields (),
> +		resolved_type->num_fields () * sizeof (struct field));
>      }
>  
>    for (i = 0; i < resolved_type->num_fields (); ++i)
> -- 
> 2.26.2
> 

Re: [PATCH 2/2] Avoid crash from coerce_unspec_val_to_type

[Andrew Burgess]

* Tom Tromey <tromey@adacore.com> [2021-02-08 08:24:48 -0700]:

> With a certain Ada program, ada-lang.c:coerce_unspec_val_to_type can
> cause a crash.  This function may copy a value, and in the particular
> case in the crash, the new value's type is smaller than the original
> type.  This causes coerce_unspec_val_to_type to create a lazy value --
> but the original value is also not_lval, so later, when the value is
> un-lazied, gdb asserts.
> 
> As with the previous patch, we believe there is a compiler bug here,
> but it is difficult to reproduce, so we're not completely certain.
> 
> In the particular case we saw, the original value has record type, and
> the record holds some variable-length arrays.  This leads to the
> type's length being 0.  At the same time, the value is optimized out.
> 
> This patch changes coerce_unspec_val_to_type to handle an
> optimized-out value correctly.
> 
> It also slightly restructures this code to avoid a crash should a
> not_lval value wind up here.  This is a purely defensive change.
> 
> This change also made it clear that value_contents_copy_raw can now be
> made static, so that is also done.
> 
> gdb/ChangeLog
> 2021-02-08  Tom Tromey  <tromey@adacore.com>
> 
> 	* ada-lang.c (coerce_unspec_val_to_type): Avoid making lazy
> 	not_lval value.
> 	* value.c (value_contents_copy_raw): Now static.
> 	* value.h (value_contents_copy_raw): Don't declare.

LGTM.

Thanks,
Andrew

> ---
>  gdb/ChangeLog  |  7 +++++++
>  gdb/ada-lang.c | 10 +++++++---
>  gdb/value.c    |  2 +-
>  gdb/value.h    |  3 ---
>  4 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 70296f97797..416a45be58e 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -601,13 +601,17 @@ coerce_unspec_val_to_type (struct value *val, struct type *type)
>  	 trying to allocate some memory for it.  */
>        ada_ensure_varsize_limit (type);
>  
> -      if (value_lazy (val)
> -	  || TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val)))
> +      if (value_optimized_out (val))
> +	result = allocate_optimized_out_value (type);
> +      else if (value_lazy (val)
> +	       /* Be careful not to make a lazy not_lval value.  */
> +	       || (VALUE_LVAL (val) != not_lval
> +		   && TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val))))
>  	result = allocate_value_lazy (type);
>        else
>  	{
>  	  result = allocate_value (type);
> -	  value_contents_copy_raw (result, 0, val, 0, TYPE_LENGTH (type));
> +	  value_contents_copy (result, 0, val, 0, TYPE_LENGTH (type));
>  	}
>        set_value_component_location (result, val);
>        set_value_bitsize (result, value_bitsize (val));
> diff --git a/gdb/value.c b/gdb/value.c
> index 4135d5ec339..bddf9a47923 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1304,7 +1304,7 @@ value_ranges_copy_adjusted (struct value *dst, int dst_bit_offset,
>     It is assumed the contents of DST in the [DST_OFFSET,
>     DST_OFFSET+LENGTH) range are wholly available.  */
>  
> -void
> +static void
>  value_contents_copy_raw (struct value *dst, LONGEST dst_offset,
>  			 struct value *src, LONGEST src_offset, LONGEST length)
>  {
> diff --git a/gdb/value.h b/gdb/value.h
> index 39e94f45ea6..60a831c38c4 100644
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -739,9 +739,6 @@ extern struct value *allocate_value_lazy (struct type *type);
>  extern void value_contents_copy (struct value *dst, LONGEST dst_offset,
>  				 struct value *src, LONGEST src_offset,
>  				 LONGEST length);
> -extern void value_contents_copy_raw (struct value *dst, LONGEST dst_offset,
> -				     struct value *src, LONGEST src_offset,
> -				     LONGEST length);
>  
>  extern struct value *allocate_repeat_value (struct type *type, int count);
>  
> -- 
> 2.26.2
>