gdbsupport: add support for references to checked_static_cast

  Add a checked_static_cast overload that works with references.  A bad
dynamic cast with references throws std::bad_cast, it would be possible
to implement the new overload based on that, but it seemed simpler to
just piggy back off the existing function.

I found some potential uses of this new overload in amd-dbgapi-target.c,
update them to illustrate the use of the new overload.  To build
amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
expect many people to have.  But I have it, and it builds fine here.  I
did test the new overload by making a purposely bad cast and it did
catch it.

Change-Id: Id6b6a7db09fe3b4aa43cddb60575ff5f46761e96
---
 gdb/amdgpu-tdep.c                    | 28 +++++++++++++++++++---------
 gdbsupport/gdb-checked-static-cast.h | 16 ++++++++++++++++
 2 files changed, 35 insertions(+), 9 deletions(-)


base-commit: c96452ad168cf42ad42f0d57214dddb38d5fae88
  

On Thu, 18 May 2023 16:57:37 -0400
Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:

> Add a checked_static_cast overload that works with references.  A bad
> dynamic cast with references throws std::bad_cast, it would be possible
> to implement the new overload based on that, but it seemed simpler to
> just piggy back off the existing function.
> 
> I found some potential uses of this new overload in amd-dbgapi-target.c,
> update them to illustrate the use of the new overload.  To build
> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
> expect many people to have.  But I have it, and it builds fine here.  I
> did test the new overload by making a purposely bad cast and it did
> catch it.

I'm not especially knowledgeable in this area, but it LGTM.

Acked-by: Kevin Buettner <kevinb@redhat.com>
  

On 5/19/23 17:31, Kevin Buettner wrote:
> On Thu, 18 May 2023 16:57:37 -0400
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
>> Add a checked_static_cast overload that works with references.  A bad
>> dynamic cast with references throws std::bad_cast, it would be possible
>> to implement the new overload based on that, but it seemed simpler to
>> just piggy back off the existing function.
>>
>> I found some potential uses of this new overload in amd-dbgapi-target.c,
>> update them to illustrate the use of the new overload.  To build
>> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
>> expect many people to have.  But I have it, and it builds fine here.  I
>> did test the new overload by making a purposely bad cast and it did
>> catch it.
> 
> I'm not especially knowledgeable in this area, but it LGTM.
> 
> Acked-by: Kevin Buettner <kevinb@redhat.com>

Thanks, I'll add your Acked-By.  I'll wait to see if Andrew has anything
to say about it, since he wrote the original feature.

Simon
  

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> Add a checked_static_cast overload that works with references.  A bad
> dynamic cast with references throws std::bad_cast, it would be possible
> to implement the new overload based on that, but it seemed simpler to
> just piggy back off the existing function.
>
> I found some potential uses of this new overload in amd-dbgapi-target.c,
> update them to illustrate the use of the new overload.  To build
> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
> expect many people to have.  But I have it, and it builds fine here.  I
> did test the new overload by making a purposely bad cast and it did
> catch it.

Looks great.  Thanks for expanding this feature.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>
> Change-Id: Id6b6a7db09fe3b4aa43cddb60575ff5f46761e96
> ---
>  gdb/amdgpu-tdep.c                    | 28 +++++++++++++++++++---------
>  gdbsupport/gdb-checked-static-cast.h | 16 ++++++++++++++++
>  2 files changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/amdgpu-tdep.c b/gdb/amdgpu-tdep.c
> index 1077fab5c65e..21a7a3ae1b79 100644
> --- a/gdb/amdgpu-tdep.c
> +++ b/gdb/amdgpu-tdep.c
> @@ -674,7 +674,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
>      case amd_dbgapi_register_type::kind::INTEGER:
>        {
>  	const auto &integer_type
> -	  = static_cast<const amd_dbgapi_register_type_integer &> (type);
> +	  = gdb::checked_static_cast<const amd_dbgapi_register_type_integer &>
> +	      (type);
>  	switch (integer_type.bit_size ())
>  	  {
>  	  case 32:
> @@ -697,7 +698,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
>      case amd_dbgapi_register_type::kind::VECTOR:
>        {
>  	const auto &vector_type
> -	  = static_cast<const amd_dbgapi_register_type_vector &> (type);
> +	  = gdb::checked_static_cast<const amd_dbgapi_register_type_vector &>
> +	      (type);
>  	struct type *element_type
>  	  = amd_dbgapi_register_type_to_gdb_type (vector_type.element_type (),
>  						  gdbarch);
> @@ -716,7 +718,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
>      case amd_dbgapi_register_type::kind::FLAGS:
>        {
>  	const auto &flags_type
> -	  = static_cast<const amd_dbgapi_register_type_flags &> (type);
> +	  = gdb::checked_static_cast<const amd_dbgapi_register_type_flags &>
> +	      (type);
>  	struct type *gdb_type
>  	  = arch_flags_type (gdbarch, flags_type.name ().c_str (),
>  			     flags_type.bit_size ());
> @@ -747,7 +750,8 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
>      case amd_dbgapi_register_type::kind::ENUM:
>        {
>  	const auto &enum_type
> -	  = static_cast<const amd_dbgapi_register_type_enum &> (type);
> +	  = gdb::checked_static_cast<const amd_dbgapi_register_type_enum &>
> +	      (type);
>  	struct type *gdb_type
>  	  = (type_allocator (gdbarch)
>  	     .new_type (TYPE_CODE_ENUM, enum_type.bit_size (),
> @@ -1310,7 +1314,8 @@ amdgpu_register_type_parse_test ()
>  
>      gdb_assert (type.kind () == amd_dbgapi_register_type::kind::FLAGS);
>  
> -    const auto &f = static_cast<const amd_dbgapi_register_type_flags &> (type);
> +    const auto &f
> +      = gdb::checked_static_cast<const amd_dbgapi_register_type_flags &> (type);
>      gdb_assert (f.size () == 23);
>  
>      /* Check the two "FP_ROUND" fields.  */
> @@ -1322,7 +1327,8 @@ amdgpu_register_type_parse_test ()
>  		      == amd_dbgapi_register_type::kind::ENUM);
>  
>  	  const auto &e
> -	    = static_cast<const amd_dbgapi_register_type_enum &> (*field.type);
> +	    = gdb::checked_static_cast<const amd_dbgapi_register_type_enum &>
> +	      (*field.type);
>  	  gdb_assert (e.size () == 4);
>  	  gdb_assert (e[0].name == "NEAREST_EVEN");
>  	  gdb_assert (e[0].value == 0);
> @@ -1338,7 +1344,8 @@ amdgpu_register_type_parse_test ()
>      gdb_assert (f[22].type->kind () == amd_dbgapi_register_type::kind::INTEGER);
>  
>      const auto &i
> -      = static_cast<const amd_dbgapi_register_type_integer &> (*f[22].type);
> +      = gdb::checked_static_cast<const amd_dbgapi_register_type_integer &>
> +	  (*f[22].type);
>      gdb_assert (i.bit_size () == 32);
>      gdb_assert (i.is_unsigned ());
>    }
> @@ -1352,13 +1359,16 @@ amdgpu_register_type_parse_test ()
>  
>      gdb_assert (type.kind () == amd_dbgapi_register_type::kind::VECTOR);
>  
> -    const auto &v = static_cast<const amd_dbgapi_register_type_vector &> (type);
> +    const auto &v
> +      = gdb::checked_static_cast<const amd_dbgapi_register_type_vector &>
> +	  (type);
>      gdb_assert (v.count () == 64);
>  
>      const auto &et = v.element_type ();
>      gdb_assert (et.kind () == amd_dbgapi_register_type::kind::INTEGER);
>  
> -    const auto &i = static_cast<const amd_dbgapi_register_type_integer &> (et);
> +    const auto &i
> +      = gdb::checked_static_cast<const amd_dbgapi_register_type_integer &> (et);
>      gdb_assert (i.bit_size () == 32);
>      gdb_assert (!i.is_unsigned ());
>    }
> diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h
> index bc75244bddd0..7e5a69a6474d 100644
> --- a/gdbsupport/gdb-checked-static-cast.h
> +++ b/gdbsupport/gdb-checked-static-cast.h
> @@ -66,6 +66,22 @@ checked_static_cast (V *v)
>    return result;
>  }
>  
> +/* Same as the above, but to cast from a reference type to another.  */
> +
> +template<typename T, typename V>
> +T
> +checked_static_cast (V &v)
> +{
> +  static_assert (std::is_reference<T>::value, "target must be a reference type");
> +
> +  using T_no_R = typename std::remove_reference<T>::type;
> +  using T_P = typename std::add_pointer<T_no_R>::type;
> +
> +  using V_no_R = typename std::remove_reference<V>::type;
> +
> +  return *checked_static_cast<T_P, V_no_R> (&v);
> +}
> +
>  }
>  
>  #endif /* COMMON_GDB_CHECKED_DYNAMIC_CAST_H */
>
> base-commit: c96452ad168cf42ad42f0d57214dddb38d5fae88
> -- 
> 2.40.1
  

Hi Simon,

> diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h
> index bc75244bddd0..7e5a69a6474d 100644
> --- a/gdbsupport/gdb-checked-static-cast.h
> +++ b/gdbsupport/gdb-checked-static-cast.h
> @@ -66,6 +66,22 @@ checked_static_cast (V *v)
>    return result;
>  }
>  
> +/* Same as the above, but to cast from a reference type to another.  */
> +
> +template<typename T, typename V>
> +T
> +checked_static_cast (V &v)
> +{
> +  static_assert (std::is_reference<T>::value, "target must be a reference type");

Have you considered incorporating this constraint inside
checked_static_cast's signature?  It could look like something like
this:

    template<typename T, typename V>
    typename std::enable_if<std::is_reference<T>::value, T>::type
    checked_static_cast (V &v)
    {
      ...
    }

If T is not a reference, then this template won't be instantiated at all
instead of instantiating it and having an error.  This could allow other
templates / functions to be considered if they matched instead of just
producing an error.

I do agree that this can end-up a bit more difficult to read for someone
not used to read this kind of code.

Best,
Lancelot.

> +
> +  using T_no_R = typename std::remove_reference<T>::type;
> +  using T_P = typename std::add_pointer<T_no_R>::type;
> +
> +  using V_no_R = typename std::remove_reference<V>::type;
> +
> +  return *checked_static_cast<T_P, V_no_R> (&v);
> +}
> +
>  }
>  
>  #endif /* COMMON_GDB_CHECKED_DYNAMIC_CAST_H */
> 
> base-commit: c96452ad168cf42ad42f0d57214dddb38d5fae88
> -- 
> 2.40.1
>
  

On 5/24/23 09:22, Lancelot SIX wrote:
> Hi Simon,
> 
>> diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h
>> index bc75244bddd0..7e5a69a6474d 100644
>> --- a/gdbsupport/gdb-checked-static-cast.h
>> +++ b/gdbsupport/gdb-checked-static-cast.h
>> @@ -66,6 +66,22 @@ checked_static_cast (V *v)
>>    return result;
>>  }
>>  
>> +/* Same as the above, but to cast from a reference type to another.  */
>> +
>> +template<typename T, typename V>
>> +T
>> +checked_static_cast (V &v)
>> +{
>> +  static_assert (std::is_reference<T>::value, "target must be a reference type");
> 
> Have you considered incorporating this constraint inside
> checked_static_cast's signature?  It could look like something like
> this:
> 
>     template<typename T, typename V>
>     typename std::enable_if<std::is_reference<T>::value, T>::type
>     checked_static_cast (V &v)
>     {
>       ...
>     }

I like static_assert because it's easier to write and read, and I find
that the error messages it produces are easier to understand.  They
point you directly to the static_assert line that says "target must be a
reference", so you know instantly what is wrong.  With template
parameters, you have to decrypt the template substitution message, it's
really not obvious.  However...

> If T is not a reference, then this template won't be instantiated at all
> instead of instantiating it and having an error.  This could allow other
> templates / functions to be considered if they matched instead of just
> producing an error.

... it seems like there are some pragmatic advantages of using the
template parameter method, it's probably the right C++ idiom to use,
even if I don't like it much.  We have the gdb::Requires util, which
makes it at least more straightforward to read the code:

    template<typename T, typename V, typename = gdb::Requires<std::is_reference<T>>>

I'll push it with that change.  Should I have your RB?

Simon
  

On 5/24/23 09:07, Andrew Burgess wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> Add a checked_static_cast overload that works with references.  A bad
>> dynamic cast with references throws std::bad_cast, it would be possible
>> to implement the new overload based on that, but it seemed simpler to
>> just piggy back off the existing function.
>>
>> I found some potential uses of this new overload in amd-dbgapi-target.c,
>> update them to illustrate the use of the new overload.  To build
>> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
>> expect many people to have.  But I have it, and it builds fine here.  I
>> did test the new overload by making a purposely bad cast and it did
>> catch it.
> 
> Looks great.  Thanks for expanding this feature.
> 
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks.  Are you also fine with the switch to use
gdb::Requires<std::is_reference<T> to do the "target is reference"
check, as Lancelot suggested?

Simon
  

On Wed, May 24, 2023 at 10:51:08AM -0400, Simon Marchi via Gdb-patches wrote:
> On 5/24/23 09:22, Lancelot SIX wrote:
> > Hi Simon,
> > 
> >> diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h
> >> index bc75244bddd0..7e5a69a6474d 100644
> >> --- a/gdbsupport/gdb-checked-static-cast.h
> >> +++ b/gdbsupport/gdb-checked-static-cast.h
> >> @@ -66,6 +66,22 @@ checked_static_cast (V *v)
> >>    return result;
> >>  }
> >>  
> >> +/* Same as the above, but to cast from a reference type to another.  */
> >> +
> >> +template<typename T, typename V>
> >> +T
> >> +checked_static_cast (V &v)
> >> +{
> >> +  static_assert (std::is_reference<T>::value, "target must be a reference type");
> > 
> > Have you considered incorporating this constraint inside
> > checked_static_cast's signature?  It could look like something like
> > this:
> > 
> >     template<typename T, typename V>
> >     typename std::enable_if<std::is_reference<T>::value, T>::type
> >     checked_static_cast (V &v)
> >     {
> >       ...
> >     }
> 
> I like static_assert because it's easier to write and read, and I find
> that the error messages it produces are easier to understand.  They
> point you directly to the static_assert line that says "target must be a
> reference", so you know instantly what is wrong.  With template
> parameters, you have to decrypt the template substitution message, it's
> really not obvious.  However...
> 
> > If T is not a reference, then this template won't be instantiated at all
> > instead of instantiating it and having an error.  This could allow other
> > templates / functions to be considered if they matched instead of just
> > producing an error.
> 
> ... it seems like there are some pragmatic advantages of using the
> template parameter method, it's probably the right C++ idiom to use,
> even if I don't like it much.  We have the gdb::Requires util, which
> makes it at least more straightforward to read the code:
> 
>     template<typename T, typename V, typename = gdb::Requires<std::is_reference<T>>>
>

C++20 would make this more readable:

    template<typename T, typename V>
      requires std::is_reference_v<T>
    T
    checked_static_cast (V &v) { ... }

but this is currently not available to us.

With the use of gdb::Requires, the static_assert becomes redundant, but
It does not hurt to keep it.  If you find it easier to capture the
intent of the code when reading, I do not mind keeping it.

Feel free to use my RB
Reviewed-By: Lancelot Six <lancelot.six@amd.com>

Lancelot.

> I'll push it with that change.  Should I have your RB?
> 
> Simon
  

On 5/24/23 11:33, Lancelot SIX wrote:
> On Wed, May 24, 2023 at 10:51:08AM -0400, Simon Marchi via Gdb-patches wrote:
>> On 5/24/23 09:22, Lancelot SIX wrote:
>>> Hi Simon,
>>>
>>>> diff --git a/gdbsupport/gdb-checked-static-cast.h b/gdbsupport/gdb-checked-static-cast.h
>>>> index bc75244bddd0..7e5a69a6474d 100644
>>>> --- a/gdbsupport/gdb-checked-static-cast.h
>>>> +++ b/gdbsupport/gdb-checked-static-cast.h
>>>> @@ -66,6 +66,22 @@ checked_static_cast (V *v)
>>>>    return result;
>>>>  }
>>>>  
>>>> +/* Same as the above, but to cast from a reference type to another.  */
>>>> +
>>>> +template<typename T, typename V>
>>>> +T
>>>> +checked_static_cast (V &v)
>>>> +{
>>>> +  static_assert (std::is_reference<T>::value, "target must be a reference type");
>>>
>>> Have you considered incorporating this constraint inside
>>> checked_static_cast's signature?  It could look like something like
>>> this:
>>>
>>>     template<typename T, typename V>
>>>     typename std::enable_if<std::is_reference<T>::value, T>::type
>>>     checked_static_cast (V &v)
>>>     {
>>>       ...
>>>     }
>>
>> I like static_assert because it's easier to write and read, and I find
>> that the error messages it produces are easier to understand.  They
>> point you directly to the static_assert line that says "target must be a
>> reference", so you know instantly what is wrong.  With template
>> parameters, you have to decrypt the template substitution message, it's
>> really not obvious.  However...
>>
>>> If T is not a reference, then this template won't be instantiated at all
>>> instead of instantiating it and having an error.  This could allow other
>>> templates / functions to be considered if they matched instead of just
>>> producing an error.
>>
>> ... it seems like there are some pragmatic advantages of using the
>> template parameter method, it's probably the right C++ idiom to use,
>> even if I don't like it much.  We have the gdb::Requires util, which
>> makes it at least more straightforward to read the code:
>>
>>     template<typename T, typename V, typename = gdb::Requires<std::is_reference<T>>>
>>
> 
> C++20 would make this more readable:
> 
>     template<typename T, typename V>
>       requires std::is_reference_v<T>
>     T
>     checked_static_cast (V &v) { ... }
> 
> but this is currently not available to us.

Ah nice, it will be easy to convert gdb::Requires to that.

> With the use of gdb::Requires, the static_assert becomes redundant, but
> It does not hurt to keep it.  If you find it easier to capture the
> intent of the code when reading, I do not mind keeping it.

I would remove it, no point in having both (I think the static_assert
would never trigger anyway).

> Feel free to use my RB
> Reviewed-By: Lancelot Six <lancelot.six@amd.com>

Thanks, I'll add it.  Just waiting for the final feedback from Andrew.

Simon
  

Simon Marchi <simon.marchi@efficios.com> writes:

> On 5/24/23 09:07, Andrew Burgess wrote:
>> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>>> Add a checked_static_cast overload that works with references.  A bad
>>> dynamic cast with references throws std::bad_cast, it would be possible
>>> to implement the new overload based on that, but it seemed simpler to
>>> just piggy back off the existing function.
>>>
>>> I found some potential uses of this new overload in amd-dbgapi-target.c,
>>> update them to illustrate the use of the new overload.  To build
>>> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
>>> expect many people to have.  But I have it, and it builds fine here.  I
>>> did test the new overload by making a purposely bad cast and it did
>>> catch it.
>> 
>> Looks great.  Thanks for expanding this feature.
>> 
>> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
>
> Thanks.  Are you also fine with the switch to use
> gdb::Requires<std::is_reference<T> to do the "target is reference"
> check, as Lancelot suggested?

Yes, that looks good too.

Thanks,
Andrew
  

On 5/24/23 14:04, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
> 
>> On 5/24/23 09:07, Andrew Burgess wrote:
>>> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>>>
>>>> Add a checked_static_cast overload that works with references.  A bad
>>>> dynamic cast with references throws std::bad_cast, it would be possible
>>>> to implement the new overload based on that, but it seemed simpler to
>>>> just piggy back off the existing function.
>>>>
>>>> I found some potential uses of this new overload in amd-dbgapi-target.c,
>>>> update them to illustrate the use of the new overload.  To build
>>>> amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
>>>> expect many people to have.  But I have it, and it builds fine here.  I
>>>> did test the new overload by making a purposely bad cast and it did
>>>> catch it.
>>>
>>> Looks great.  Thanks for expanding this feature.
>>>
>>> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
>>
>> Thanks.  Are you also fine with the switch to use
>> gdb::Requires<std::is_reference<T> to do the "target is reference"
>> check, as Lancelot suggested?
> 
> Yes, that looks good too.

Thanks, pushed.

Simon