Re: [PATCH] Fix printing of non pointer types when memory tagging is enabled

[Luis Machado <luis.machado@linaro.org>]

On 7/5/21 8:06 PM, Pedro Alves wrote:
> Hi!
> 
> On 2021-07-05 3:32 p.m., Luis Machado wrote:
>> On 7/2/21 12:14 PM, Pedro Alves wrote:
>>> On 2021-07-02 2:45 p.m., Luis Machado wrote:
>>>> On 7/2/21 10:19 AM, Pedro Alves wrote:
>>>>> On 2021-07-02 1:47 p.m., Luis Machado wrote:
>>>>>> On 7/1/21 4:52 PM, Pedro Alves wrote:
>>>>>>> On 2021-06-22 2:37 a.m., Luis Machado via Gdb-patches wrote:
>>>>>>>
>>>>>>>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>>>>>>>> index e9761ed2189..84ef616ee35 100644
>>>>>>>> --- a/gdb/aarch64-linux-tdep.c
>>>>>>>> +++ b/gdb/aarch64-linux-tdep.c
>>>>>>>> @@ -1559,7 +1559,19 @@ aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
>>>>>>>>      {
>>>>>>>>        gdb_assert (address != nullptr);
>>>>>>>>      -  CORE_ADDR addr = value_as_address (address);
>>>>>>>> +  CORE_ADDR addr;
>>>>>>>> +
>>>>>>>> +  /* value_as_address may throw if, for example, the value is optimized
>>>>>>>> +     out.  Make sure we handle that gracefully.  */
>>>>>>>
>>>>>>> This seems like a too-large hammer to me.  I think you should check whether the value
>>>>>>> is optimized out explicitly instead.
>>>>>>>
>>>>>>>          if (value_optimized_out (address)
>>>>>>>             || !value_entirely_available (address))
>>>>>>>            return false;
>>>>>>>
>>>>>>> ... and you can do that in common code somewhere so that archs don't need to worry about it.
>>>>>>
>>>>>> It's not just an optimized out case. If we have a 128-bit pointer, for example, GDB's conversion machinery will refuse to convert that value (I think GDB is hardcoded to handle at most 64-bit pointers). I think this throws from unpack_long, hence why I decided to guard this conversion call.
>>>>>
>>>>> 128-bit pointers?  Is that a thing?  I'd think would run into a lot more troubles
>>>>> throughout?  Can you see about reproducing that?
>>>>
>>>> Yes. Morello/CHERI ports would run into this situation. I'm just giving an example of how a conversion problem may derail a simple operation of fetching an address.
>>>
>>> I don't understand.  I have never heard of Morello/CHERI, but are you saying it has 128-bit-wide
>>> pointers, and somehow GDB has been made to work for it with 64-bit CORE_ADDR?
>>>
>>
>> Yes.
> 
> So it has 128-bit fat pointers with extra bits for encoding "things", but still 64-bit addresses?
> Then GDB will still need to be adjusted so that value_as_address works on such pointers.

Yes. That's true.

> 
> Is this the architecture that led to the gdbarch interface being based on struct value instead
> of plain CORE_ADDR ?

That's right.

> 
>>
>>>>
>>>>>
>>>>> My thinking is that it should be possible to tell upfront whether you're asking to
>>>>> convert something to an address that it doesn't make sense to try.
>>>>
>>>> It is certainly possible, but the complexity of trying to handle all of the corner cases of working with types/values makes it not worth the effort in my opinion. Even GDB's type/value handling code is lengthy and doesn't handle all the possible casts. The user can throw all sorts of weird expressions at the print command, and unfortunately this code would need to get through that to figure out if it has any tags.
>>>
>>> I think I must be missing something.  You're working with the result of the expression, not the
>>> intermediate values of the expression.  And you're looking at the value and only converting it
>>> to address if it is TYPE_CODE_PTR, which means that value is a scalar.  What do the user
>>> expressions have to do with it
>>
>> Right. We're only dealing with the result of the expression. If that result has a type/content that causes GDB's value/type functions to throw, then the optimized out checks won't catch that.
>>
>> For example, if we feed a 128-bit pointer to the print command, value_as_address will throw with an invalid cast.
> 
> Then how can GDB ever work with such pointers even without the tag checking in printcmd.c?
> Such a port would have to fix that anyhow, no?  Likely with a custom
> gdbarch_pointer_to_address implementation.
> 

Yes, but this is mainline GDB, and the rationale is to anticipate that 
need by covering that case in this patch.

>>
>> I don't recall exactly if there are other ways to make value_as_address throw with a TYPE_CODE_PTR. The current code will throw with other types though, but I'm proposing restricting it to TYPE_CODE_PTR. So those can be ignored.
> 
> value_as_address on non-pointer and non-reference types wants to treat the
> value as integer, so yeah, it doesn't make sense to me to try to validate tags in
> that case:
> 
>    if (value_type (val)->code () != TYPE_CODE_PTR
>        && !TYPE_IS_REFERENCE (value_type (val))
>        && gdbarch_integer_to_address_p (gdbarch))
>      return gdbarch_integer_to_address (gdbarch, value_type (val),
> 				       value_contents (val));
> 
>    return unpack_long (value_type (val), value_contents (val));
> 
> But if you have a pointer or a reference, value_as_address goes straight to
> unpack_long.  How can that throw?  unpack_long will reach here:
> 
>      case TYPE_CODE_PTR:
>      case TYPE_CODE_REF:
>      case TYPE_CODE_RVALUE_REF:
>        /* Assume a CORE_ADDR can fit in a LONGEST (for now).  Not sure
> 	 whether we want this to be true eventually.  */
>        return extract_typed_address (valaddr, type);
> 
> that ends up in
>    gdbarch_pointer_to_address
>     -> unsigned_pointer_to_address (for most archs, including Aarch64)
>       -> extract_unsigned_integer
>         -> extract_integer
> 
> So the only error possible that I can see is this in extract_integer:
> 
>    if (len > (int) sizeof (T))
>      error (_("\
> That operation is not available on integers of more than %d bytes."),
> 	   (int) sizeof (T));
> 
> This would be the case of 128-bit pointers, but this would need to
> be fixed for archs that have them anyhow -- value_as_address is called all
> over the place to convert pointers to addresses.  Likely such port would
> install a custom gdbarch_pointer_to_address callback that would understand
> how to decode a 64-bit address from a 128-bit pointer.

That's a correct assessment.

> 
> Going back to this:
> 
>> Right. We're only dealing with the result of the expression. If that result has a type/content that causes GDB's value/type functions to throw, then the optimized out checks won't catch that.
> 
> Let's look at the "contents" concern.  value_as_address calls value_contents, which only fails if
> you have an optimized out or unavailable value:
> 
>   const gdb_byte *
>   value_contents (struct value *value)
>   {
>     const gdb_byte *result = value_contents_writeable (value);
>     require_not_optimized_out (value);
>     require_available (value);
>     return result;
>   }
> 
> and we've already discussed adding checks for those.
> 
>>
>> So, will a try/catch in printcmd.c be a reasonable solution then? I can change the code back to do that. And also check the optimized out condition in the arch hook.
> 
> I think so, and with printing the error, because I think that an error here would
> either mean something unexpected happened that we should improve and we best not swallow
> it so we can address it in future, or, it is something interesting for the user.
> 
> See patch at the bottom.
> 

I gave this a go and it looks good testsuite-wise.

>>
>>>
>>> Can you show a backtrace of an error you're concerned about, other than optimized out?
>>>
>>>>
>>>> So relying on trying a conversion and then catching a failure seems simple enough for now. And good enough for TYPE_CODE_PTR types and our use cases.
>>>>
>>>> Ideally we'd handle references, method pointers and other pointer-like types, but those have their own intricacies. I think even method pointers are 128 bits in size, if I'm not mistaken.
>>>
>>> I still fail to see the intricacies you mention.
>>>
>> Ultimately we want a scalar we can work with to check the tag. Fetching a scalar from a TYPE_CODE_PTR is reasonably simple. Trying to fetch that scalar from a method pointer, references and other types require different operations that may or may not throw.
> 
> ISTM that the right view is that it only makes sense to check the tags when you're going to print an
> address value, which means you're going to print a pointer or a reference.  Checking tags when you're going
> to print an rvalue, an integer, a struct object, etc., doesn't make sense, since those are not addresses.
> That's the direction your patch was going.  But I don't understand why check tags for pointers
> and not for references.  I don't think there should be nothing special wrt to references compared
> to TYPE_CODE_PTR?  What errors did you run into with references -- references are basically pointers
> under the hood.

Unfortunately I don't recall exactly what was the case for references. 
GDB does bend things a little so users have a better experience. GDB 
will sometimes implicitly dereference some things when the language 
wouldn't, for example.

These failures showed up in the testsuite by the way. That's why I wrote 
the patch.

> 
>>
>> I don't want to have to handle fetching a scalar from all of those types within an arch-specific hook. Hence why I'm narrowing it down to handling only TYPE_CODE_PTR.
>>
> 
> It is really incorrect to call value_as_address for TYPE_CODE_METHODPTR and TYPE_CODE_MEMBERPTR.  That's
> not how you get the address of the corresponding methods/members.  I'm not sure it even makes any
> sense to validate tags for TYPE_CODE_MEMBERPTR -- that is an offset into the class, not an address.
> TYPE_CODE_METHODPTR I'm not so sure, but if it does, it should be possible to factor out some
> code out of the expression evaluator, e.g., structop_member_base::evaluate_funcall.  But as you
> say, let's ignore this for now.

Yes. Those are the intricacies I was talking about. I'll need to check 
what the compiler does for those.

> 
> So, that leaves us normal pointers, and references.
> 
> Below is what I was thinking.  Build tested with --enable-targets=all, but otherwise not tested.
> 
> Also, can you add some tests?  The need for this clearly went unnoticed due to lack of test coverage.

The testsuite did catch these. There were problems with optimized out 
values, method pointers, references (so it seemed at the time) and 
trying to fetch addresses from non-pointer types. All of those were 
caught by running the testsuite on a MTE-capable setup.

What sort of test did you have in mind, other than 
gdb.arch/aarch64-mte.exp, which already covers printing things and 
adjusting tags?

> 
>  From 0d6c0c70bedccae81eb369cbefbf9f9f5f04a3c3 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Mon, 5 Jul 2021 18:51:11 +0100
> Subject: [PATCH] Fix printing of non pointer types when memory tagging is
>   enabled
> 
> Change-Id: I82bf00ac88d23553b3f7563c9872dfa6ca1f2207
> ---
>   gdb/gdbarch.h  |  3 +-
>   gdb/gdbarch.sh |  3 +-
>   gdb/printcmd.c | 81 ++++++++++++++++++++++++++++++++------------------
>   3 files changed, 56 insertions(+), 31 deletions(-)
> 
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index ece765b826f..7db3e36d76a 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -730,7 +730,8 @@ typedef std::string (gdbarch_memtag_to_string_ftype) (struct gdbarch *gdbarch, s
>   extern std::string gdbarch_memtag_to_string (struct gdbarch *gdbarch, struct value *tag);
>   extern void set_gdbarch_memtag_to_string (struct gdbarch *gdbarch, gdbarch_memtag_to_string_ftype *memtag_to_string);
>   
> -/* Return true if ADDRESS contains a tag and false otherwise. */
> +/* Return true if ADDRESS contains a tag and false otherwise.  ADDRESS
> +   must be either a pointer or a reference type. */
>   
>   typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, struct value *address);
>   extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index d9332c2103e..9bc9de91c30 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -608,7 +608,8 @@ v;int;significant_addr_bit;;;;;;0
>   # Return a string representation of the memory tag TAG.
>   m;std::string;memtag_to_string;struct value *tag;tag;;default_memtag_to_string;;0
>   
> -# Return true if ADDRESS contains a tag and false otherwise.
> +# Return true if ADDRESS contains a tag and false otherwise.  ADDRESS
> +# must be either a pointer or a reference type.
>   m;bool;tagged_address_p;struct value *address;address;;default_tagged_address_p;;0
>   
>   # Return true if the tag from ADDRESS matches the memory tag for that
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 3cd42f817f5..6aee49afd2e 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1266,19 +1266,26 @@ print_value (value *val, const value_print_options &opts)
>   static bool
>   should_validate_memtags (struct value *value)
>   {
> -  if (target_supports_memory_tagging ()
> -      && gdbarch_tagged_address_p (target_gdbarch (), value))
> -    {
> -      gdb_assert (value != nullptr && value_type (value) != nullptr);
> +  gdb_assert (value != nullptr && value_type (value) != nullptr);
>   
> -      enum type_code code = value_type (value)->code ();
> +  if (!target_supports_memory_tagging ())
> +    return false;
>   
> -      return (code == TYPE_CODE_PTR
> -	      || code == TYPE_CODE_REF
> -	      || code == TYPE_CODE_METHODPTR
> -	      || code == TYPE_CODE_MEMBERPTR);
> -    }
> -  return false;
> +  enum type_code code = value_type (value)->code ();
> +
> +  /* Skip non-address values.  */
> +  if (code != TYPE_CODE_PTR
> +      && !TYPE_IS_REFERENCE (value_type (value)))
> +    return false;
> +
> +  /* OK, we have an address value.  Check we have a complete value we
> +     can extract.  */
> +  if (value_optimized_out (value)
> +      || !value_entirely_available (value))
> +    return false;
> +
> +  /* We do.  Check whether it includes any tags.  */
> +  return gdbarch_tagged_address_p (target_gdbarch (), value);
>   }
>   
>   /* Helper for parsing arguments for print_command_1.  */
> @@ -1321,26 +1328,42 @@ print_command_1 (const char *args, int voidprint)
>   		    value_type (val)->code () != TYPE_CODE_VOID))
>       {
>         /* If memory tagging validation is on, check if the tag is valid.  */
> -      if (print_opts.memory_tag_violations && should_validate_memtags (val)
> -	  && !gdbarch_memtag_matches_p (target_gdbarch (), val))
> +      if (print_opts.memory_tag_violations)
>   	{
> -	  /* Fetch the logical tag.  */
> -	  struct value *tag
> -	    = gdbarch_get_memtag (target_gdbarch (), val,
> -				  memtag_type::logical);
> -	  std::string ltag
> -	    = gdbarch_memtag_to_string (target_gdbarch (), tag);
> -
> -	  /* Fetch the allocation tag.  */
> -	  tag = gdbarch_get_memtag (target_gdbarch (), val,
> -				    memtag_type::allocation);
> -	  std::string atag
> -	    = gdbarch_memtag_to_string (target_gdbarch (), tag);
> -
> -	  printf_filtered (_("Logical tag (%s) does not match the "
> -			     "allocation tag (%s).\n"),
> -			   ltag.c_str (), atag.c_str ());
> +	  try
> +	    {
> +	      if (should_validate_memtags (val)
> +		  && !gdbarch_memtag_matches_p (target_gdbarch (), val))
> +		{
> +		  /* Fetch the logical tag.  */
> +		  struct value *tag
> +		    = gdbarch_get_memtag (target_gdbarch (), val,
> +					  memtag_type::logical);
> +		  std::string ltag
> +		    = gdbarch_memtag_to_string (target_gdbarch (), tag);
> +
> +		  /* Fetch the allocation tag.  */
> +		  tag = gdbarch_get_memtag (target_gdbarch (), val,
> +					    memtag_type::allocation);
> +		  std::string atag
> +		    = gdbarch_memtag_to_string (target_gdbarch (), tag);
> +
> +		  printf_filtered (_("Logical tag (%s) does not match the "
> +				     "allocation tag (%s).\n"),
> +				   ltag.c_str (), atag.c_str ());
> +		}
> +	    }
> +	  catch (gdb_exception_error &ex)
> +	    {
> +	      if (ex.error == TARGET_CLOSE_ERROR)
> +		throw;
> +
> +	      fprintf_filtered (gdb_stderr,
> +				_("Could not validate memory tag: %s.\n"),
> +				ex.message->c_str ());
> +	    }
>   	}
> +
>         print_value (val, print_opts);
>       }
>   }
> 
> base-commit: 74ace054855321bc5271dd7b354131cd0b71333a
>