Re: [PATCH 3/4] gdb: split get_discrete_bounds in two

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2020-12-06 2:29 a.m., Joel Brobecker wrote:
> Hi Simon,
> 
> On Mon, Nov 23, 2020 at 11:21:19AM -0500, Simon Marchi via Gdb-patches wrote:
>> get_discrete_bounds is not flexible for ranges (TYPE_CODE_RANGE), in the
>> sense that it returns true (success) only if both bounds are present and
>> constant values.
>>
>> This is a problem for code that only needs to know the low bound and
>> fails unnecessarily if the high bound is unknown.
>>
>> Split the function in two, get_discrete_low_bound and
>> get_discrete_high_bound, that both return an optional.  Provide a new
>> implementation of get_discrete_bounds based on the two others, so the
>> callers don't have to be changed.
>>
>> gdb/ChangeLog:
>>
>> 	* gdbtypes.c (get_discrete_bounds): Implement with
>> 	get_discrete_low_bound and get_discrete_high_bound.
>> 	(get_discrete_low_bound): New.
>> 	(get_discrete_high_bound): New.
>>
>> Change-Id: I986b5e9c0dd969800e3fb9546af9c827d52e80d0
> 
> Small reminder to remove the Change-Id in the ChangeLog.
> 
> I agree with the problem, and modulo the minor question I had below
> (which might not actually be an issue), the patch looks good to me.
> 
> I do have one minor concern. On the one hand, the code seems to be
> slightly simpler and clearer. On the other hand, I worry a bit about
> keeping the two functions in sync. I'm wondering if it might be worth
> investigating ways to have achieve the same end result without
> duplicating the skeleton of the code. I tried thinking about it
> for a while, and the only option that might be viable that I could
> think of was to allow users of get_discrete_bounds to pass nullptr
> for either LOWP or HIGHP, and then only handle the bounds whose
> parameter is not null. Then we can have the low/high functions
> if we wanted. In trying to visualize how this would look like,
> I found that two scenarios:
> 
>    - I would be bracketing small pieces of code with lots of
>      "if <param> != nullptr" (just for TYPE_CODE_RANGE, I counted
>      6 instances of an addional condition); FTR, this is what it
>      would look like for TYPE_CODE_RANGE:
> 
>          | case TYPE_CODE_RANGE:
>          |   if ((lowp != nullptr && type->bounds ()->low.kind () != PROP_CONST)
>          |       || (highp != nullptr && type->bounds ()->high.kind () != PROP_CONST))
>          |     return false;
> 
>          |   if (lowp != nullptr)
>          |     *lowp = type->bounds ()->low.const_val ();
>          |   if (highp != nullptr)
>          |     *highp = type->bounds ()->high.const_val ();
> 
>          |   if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>          |     {
>          |       if (lowp != nullptr)
>          |         {
>          |           gdb::optional<LONGEST> low_pos
>          |             = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
> 
>          |           if (low_pos.has_value ())
>          |             *lowp = *low_pos;
>          |         }
> 
>          |       if (highp != nullptr)
>          |         {
>          |           gdb::optional<LONGEST> high_pos
>          |             = discrete_position (TYPE_TARGET_TYPE (type), *highp);
> 
>          |           if (high_pos.has_value ())
>          |             *highp = *high_pos;
>          |         }
>          |     }
>          |   return true;
> 
> 
>    - I would be splitting the lowp and highp code in two inside
>      each case; in this case, when one looks at how things look like,
>      it takes away half of the reasons why we might want to keep
>      the code together. E.g. for TYPE_CODE_RANGE, it woudl look like this:
> 
>          | case TYPE_CODE_RANGE:
>          |   if (lowp != nullptr)
>          |     {
>          |       if (type->bounds ()->low.kind () != PROP_CONST)
>          |         return false;
> 
>          |       *lowp = type->bounds ()->low.const_val ();
> 
>          |       if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>          |         {
>          |           gdb::optional<LONGEST> low_pos
>          |             = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
> 
>          |           if (low_pos.has_value ())
>          |             *lowp = *low_pos;
>          |         }
>          |     }
>          |   if (lowp != nullptr)
>          |     {
>          |       if (type->bounds ()->high.kind () != PROP_CONST)
>          |         return false;
> 
>          |       *highp = type->bounds ()->high.const_val ();
> 
>          |       if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>          |         {
>          |           gdb::optional<LONGEST> high_pos
>          |             = discrete_position (TYPE_TARGET_TYPE (type), *highp);
> 
>          |           if (high_pos.has_value ())
>          |             *highp = *high_pos;
>          |         }
>          |     }
>          |   return true;
> 
> For the record, I thought about perhaps using templates, which would
> work, but I think at the cost of the templated code being more complex.
> 
> So, in the end, unless you have some better ideas, I am find with
> either your solution (splitting), or the solution above which has
> the property of better keeping the code together, but is far from
> perfect because it is at the cost of complexifying the implementation
> a bit (one has to always check that LOWP/HIGHP is not nullptr).

Yeah, I tried several solutions and splitting was the one I preferred, 
as I preferred having two simpler functions than one more complex one.

> 
> See comments below, though.
> 
>> ---
>>   gdb/gdbtypes.c | 187 ++++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 130 insertions(+), 57 deletions(-)
>>
>> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
>> index b47bd28945d..4df23cfe0fe 100644
>> --- a/gdb/gdbtypes.c
>> +++ b/gdb/gdbtypes.c
>> @@ -1036,71 +1036,127 @@ has_static_range (const struct range_bounds *bounds)
>>   	  && bounds->stride.kind () == PROP_CONST);
>>   }
>>   
>> -/* See gdbtypes.h.  */
>> +/* If TYPE's low bound is a known constant, return it, else return nullopt.  */
>>   
>> -bool
>> -get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
>> +static gdb::optional<LONGEST>
>> +get_discrete_low_bound (struct type *type)
>>   {
>>     type = check_typedef (type);
>>     switch (type->code ())
>>       {
>>       case TYPE_CODE_RANGE:
>> -      /* This function currently only works for ranges with two defined,
>> -	 constant bounds.  */
>> -      if (type->bounds ()->low.kind () != PROP_CONST
>> -	  || type->bounds ()->high.kind () != PROP_CONST)
>> +      {
>> +	/* This function only works for ranges with a constant low bound.  */
>> +	if (type->bounds ()->low.kind () != PROP_CONST)
>> +	  return {};
>> +
>> +	LONGEST low = type->bounds ()->low.const_val ();
>> +
>> +	if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>> +	  {
>> +	    gdb::optional<LONGEST> low_pos
>> +	      = discrete_position (TYPE_TARGET_TYPE (type), low);
>> +
>> +	    if (low_pos.has_value ())
>> +	      low = *low_pos;
>> +	  }
>> +
>> +	return low;
>> +      }
>> +
>> +    case TYPE_CODE_ENUM:
>> +      {
>> +	if (type->num_fields () > 0)
>> +	  {
>> +	    /* The enums may not be sorted by value, so search all
>> +	       entries.  */
>> +	    LONGEST low = TYPE_FIELD_ENUMVAL (type, 0);
>> +
>> +	    for (int i = 0; i < type->num_fields (); i++)
>> +	      {
>> +		if (TYPE_FIELD_ENUMVAL (type, i) < low)
>> +		  low = TYPE_FIELD_ENUMVAL (type, i);
>> +	      }
>> +
>> +	    /* Set unsigned indicator if warranted.  */
>> +	    if (low >= 0)
>> +	      type->set_is_unsigned (true);
>> +
>> +	    return low;
>> +	  }
>> +	else
>> +	  return 0;
>> +      }
>> +
>> +    case TYPE_CODE_BOOL:
>> +      return 0;
>> +
>> +    case TYPE_CODE_INT:
>> +      if (TYPE_LENGTH (type) > sizeof (LONGEST))	/* Too big */
>>   	return false;
> 
> Should this be "return {};" here?

Hmm, yes, nice catch!

>>   
>> -      *lowp = type->bounds ()->low.const_val ();
>> -      *highp = type->bounds ()->high.const_val ();
>> +      if (!type->is_unsigned ())
>> +	return -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
>>   
>> -      if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>> -	{
>> -	  gdb::optional<LONGEST> low_pos
>> -	    = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
>> +      /* fall through */
>> +    case TYPE_CODE_CHAR:
>> +      return 0;
>>   
>> -	  if (low_pos.has_value ())
>> -	    *lowp = *low_pos;
>> +    default:
>> +      return false;
> 
> Same as above.

Yes.

> 
>> +    }
>> +}
>>   
>> -	  gdb::optional<LONGEST> high_pos
>> -	    = discrete_position (TYPE_TARGET_TYPE (type), *highp);
>> +/* If TYPE's high bound is a known constant, return it, else return nullopt.  */
>>   
>> -	  if (high_pos.has_value ())
>> -	    *highp = *high_pos;
>> -	}
>> -      return true;
>> +static gdb::optional<LONGEST>
>> +get_discrete_high_bound (struct type *type)
>> +{
>> +  type = check_typedef (type);
>> +  switch (type->code ())
>> +    {
>> +    case TYPE_CODE_RANGE:
>> +      {
>> +	/* This function only works for ranges with a constant high bound.  */
>> +	if (type->bounds ()->high.kind () != PROP_CONST)
>> +	  return {};
>> +
>> +	LONGEST high = type->bounds ()->high.const_val ();
>> +
>> +	if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>> +	  {
>> +	    gdb::optional<LONGEST> high_pos
>> +	      = discrete_position (TYPE_TARGET_TYPE (type), high);
>> +
>> +	    if (high_pos.has_value ())
>> +	      high = *high_pos;
>> +	  }
>> +
>> +	return high;
>> +      }
>>   
>>       case TYPE_CODE_ENUM:
>> -      if (type->num_fields () > 0)
>> -	{
>> -	  /* The enums may not be sorted by value, so search all
>> -	     entries.  */
>> -	  int i;
>> +      {
>> +	if (type->num_fields () > 0)
>> +	  {
>> +	    /* The enums may not be sorted by value, so search all
>> +	       entries.  */
>> +	    LONGEST high = TYPE_FIELD_ENUMVAL (type, 0);
>>   
>> -	  *lowp = *highp = TYPE_FIELD_ENUMVAL (type, 0);
>> -	  for (i = 0; i < type->num_fields (); i++)
>> -	    {
>> -	      if (TYPE_FIELD_ENUMVAL (type, i) < *lowp)
>> -		*lowp = TYPE_FIELD_ENUMVAL (type, i);
>> -	      if (TYPE_FIELD_ENUMVAL (type, i) > *highp)
>> -		*highp = TYPE_FIELD_ENUMVAL (type, i);
>> -	    }
>> +	    for (int i = 0; i < type->num_fields (); i++)
>> +	      {
>> +		if (TYPE_FIELD_ENUMVAL (type, i) > high)
>> +		  high = TYPE_FIELD_ENUMVAL (type, i);
>> +	      }
>>   
>> -	  /* Set unsigned indicator if warranted.  */
>> -	  if (*lowp >= 0)
>> -	    type->set_is_unsigned (true);
>> -	}
>> -      else
>> -	{
>> -	  *lowp = 0;
>> -	  *highp = -1;
>> -	}
>> -      return true;
>> +	    return high;
>> +	  }
>> +	else
>> +	  return -1;
>> +      }
>>   
>>       case TYPE_CODE_BOOL:
>> -      *lowp = 0;
>> -      *highp = 1;
>> -      return true;
>> +      return 1;
>>   
>>       case TYPE_CODE_INT:
>>         if (TYPE_LENGTH (type) > sizeof (LONGEST))	/* Too big */
> 
> In that section, there is a "return false" that I'm wondering
> needs to be changed to "return {}" as well...

There were 2 in get_discrete_low_bound and 2 in get_discrete_high_bound, 
which I have now fixed locally, I think that's it.

The TYPE_CODE_ENUM case also looks suspicious.  When the enum type has 
no enumerators, the old code returns 0/-1, which looks like some kind of 
error value, but also returns true, which means "success".  I kept the 
existing behavior for now, but I am wondering if this should be turned 
into an error (returning nullopt) - maybe as a follow-up patch.  I would 
need to investigate all callers that might handle enums.

> 
>> @@ -1108,25 +1164,42 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
>>   
>>         if (!type->is_unsigned ())
>>   	{
>> -	  *lowp = -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
>> -	  *highp = -*lowp - 1;
>> -	  return true;
>> +	  LONGEST low = -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
>> +	  return -low - 1;
>>   	}
>> +
>>         /* fall through */
>>       case TYPE_CODE_CHAR:
>> -      *lowp = 0;
>> -      /* This round-about calculation is to avoid shifting by
>> -	 TYPE_LENGTH (type) * TARGET_CHAR_BIT, which will not work
>> -	 if TYPE_LENGTH (type) == sizeof (LONGEST).  */
>> -      *highp = 1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1);
>> -      *highp = (*highp - 1) | *highp;
>> -      return true;
>> +      {
>> +	/* This round-about calculation is to avoid shifting by
>> +	   TYPE_LENGTH (type) * TARGET_CHAR_BIT, which will not work
>> +	   if TYPE_LENGTH (type) == sizeof (LONGEST).  */
>> +	LONGEST high = 1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1);
>> +	return (high - 1) | high;
>> +      }
>>   
>>       default:
>>         return false;
> 
> Same as above.

Fixed.

> 
>>       }
>>   }
>>   
>> +/* See gdbtypes.h.  */
>> +
>> +bool
>> +get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
>> +{
>> +  gdb::optional<LONGEST> low = get_discrete_low_bound (type);
>> +  gdb::optional<LONGEST> high = get_discrete_high_bound (type);
>> +
>> +  if (!low.has_value () || !high.has_value ())
>> +    return false;
> 
> What do you think of computing high only after having verified
> that "low" has a value? It would only be a small optimization,
> but since it doesn't really complexify the code, and only adds
> a couple of extra lines of code, it seems worth having?

Ok, I'll do that.

> 
> One thing also that this patch makes me realize is that it answers
> one question I had in the back of my mind: LOWP and HIGHP are only
> set if both bounds can be computed -- otherwise, they are both
> left untouched. Perhaps we can in a followup patch update the function's
> doc to mention that?

I'll just add it in this patch, since I am changing this function in 
this patch.

Thanks,

Simon