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

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

[Luis Machado]

When the architecture supports memory tagging, we handle pointer types
in a special way, so we can validate tags and show mismatches.

I noticed some errors while printing composite types, floats, references,
member functions and other things that implicitly get dereferenced by GDB to
show the contents rather than the pointer.

Vector registers:

(gdb) p $v0
Value can't be converted to integer.

Non-existent internal variables:

(gdb) p $foo
Value can't be converted to integer.

The same happens for complex types and printing struct/union types.

There are a couple problems. The first one is that we try to evaluate the
expression to print and eventually call value_as_address (...) before making
sure we have a suitable TYPE_CODE_PTR type. That may throw for some types. We
fix this by making sure we have a TYPE_CODE_PTR first, and then proceed to
check if we need to validate tags.

The second problem is that the evaluation process may naturally throw an
error.  One such case is when we have an optimized out variable. Thus we
guard the evaluation path with a try/catch.

If the evaluation throws, we want to resume the default expression printing
path instead of erroring out and printing nothing useful.

This isn't ideal, but GDB does some magic, internally, in order to provide an
improved user experience. This allows users to print the contents of some types
without having to use the dereference operator.

With the patch, printing works correctly again:

(gdb) p $v0
$1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
      3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
      791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
      1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
      0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
      12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
      62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
(gdb) p $foo
$2 = void
(gdb) p 2 + 2i
$3 = 2 + 2i

gdb/ChangeLog

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

	* printcmd.c (should_validate_memtags): Make more robust against
	exceptions.
---
 gdb/printcmd.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index a5c03c3a830..1194c18358d 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1266,18 +1266,22 @@ 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 ();
+  enum type_code code = value_type (value)->code ();
 
-      return (code == TYPE_CODE_PTR
-	      || code == TYPE_CODE_REF
-	      || code == TYPE_CODE_METHODPTR
-	      || code == TYPE_CODE_MEMBERPTR);
+  try
+    {
+      if (code == TYPE_CODE_PTR
+	  && target_supports_memory_tagging ()
+	  && gdbarch_tagged_address_p (target_gdbarch (), value))
+	return true;
     }
+  catch (gdb_exception_error &ex)
+    {
+      /* Ignore the error and don't validate tags.  */
+    }
+
   return false;
 }
 
-- 
2.25.1

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

[Luis Machado]

On 5/18/21 5:20 PM, Luis Machado wrote:
> When the architecture supports memory tagging, we handle pointer types
> in a special way, so we can validate tags and show mismatches.
> 
> I noticed some errors while printing composite types, floats, references,
> member functions and other things that implicitly get dereferenced by GDB to
> show the contents rather than the pointer.
> 
> Vector registers:
> 
> (gdb) p $v0
> Value can't be converted to integer.
> 
> Non-existent internal variables:
> 
> (gdb) p $foo
> Value can't be converted to integer.
> 
> The same happens for complex types and printing struct/union types.
> 
> There are a couple problems. The first one is that we try to evaluate the
> expression to print and eventually call value_as_address (...) before making
> sure we have a suitable TYPE_CODE_PTR type. That may throw for some types. We
> fix this by making sure we have a TYPE_CODE_PTR first, and then proceed to
> check if we need to validate tags.
> 
> The second problem is that the evaluation process may naturally throw an
> error.  One such case is when we have an optimized out variable. Thus we
> guard the evaluation path with a try/catch.
> 
> If the evaluation throws, we want to resume the default expression printing
> path instead of erroring out and printing nothing useful.
> 
> This isn't ideal, but GDB does some magic, internally, in order to provide an
> improved user experience. This allows users to print the contents of some types
> without having to use the dereference operator.
> 
> With the patch, printing works correctly again:
> 
> (gdb) p $v0
> $1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
>        3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
>        791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
>        1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
>        0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
>        12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
>        62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
> (gdb) p $foo
> $2 = void
> (gdb) p 2 + 2i
> $3 = 2 + 2i
> 
> gdb/ChangeLog
> 
> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> 
> 	* printcmd.c (should_validate_memtags): Make more robust against
> 	exceptions.
> ---
>   gdb/printcmd.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index a5c03c3a830..1194c18358d 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1266,18 +1266,22 @@ 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 ();
> +  enum type_code code = value_type (value)->code ();
>   
> -      return (code == TYPE_CODE_PTR
> -	      || code == TYPE_CODE_REF
> -	      || code == TYPE_CODE_METHODPTR
> -	      || code == TYPE_CODE_MEMBERPTR);
> +  try
> +    {
> +      if (code == TYPE_CODE_PTR
> +	  && target_supports_memory_tagging ()
> +	  && gdbarch_tagged_address_p (target_gdbarch (), value))
> +	return true;
>       }
> +  catch (gdb_exception_error &ex)
> +    {
> +      /* Ignore the error and don't validate tags.  */
> +    }
> +
>     return false;
>   }
>   
> 

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

[John Baldwin]

On 5/18/21 1:20 PM, Luis Machado via Gdb-patches wrote:
> When the architecture supports memory tagging, we handle pointer types
> in a special way, so we can validate tags and show mismatches.
> @@ -1266,18 +1266,22 @@ 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 ();
> +  enum type_code code = value_type (value)->code ();
>   
> -      return (code == TYPE_CODE_PTR
> -	      || code == TYPE_CODE_REF
> -	      || code == TYPE_CODE_METHODPTR
> -	      || code == TYPE_CODE_MEMBERPTR);
> +  try
> +    {
> +      if (code == TYPE_CODE_PTR
> +	  && target_supports_memory_tagging ()
> +	  && gdbarch_tagged_address_p (target_gdbarch (), value))
> +	return true;

Hmm, so why only TYPE_CODE_PTR in the new code but TYPE_CODE_REF and
method/member pointers in the old?

-- 
John Baldwin

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

[Luis Machado]

On 5/28/21 4:08 PM, John Baldwin wrote:
> On 5/18/21 1:20 PM, Luis Machado via Gdb-patches wrote:
>> When the architecture supports memory tagging, we handle pointer types
>> in a special way, so we can validate tags and show mismatches.
>> @@ -1266,18 +1266,22 @@ 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 ();
>> +  enum type_code code = value_type (value)->code ();
>> -      return (code == TYPE_CODE_PTR
>> -          || code == TYPE_CODE_REF
>> -          || code == TYPE_CODE_METHODPTR
>> -          || code == TYPE_CODE_MEMBERPTR);
>> +  try
>> +    {
>> +      if (code == TYPE_CODE_PTR
>> +      && target_supports_memory_tagging ()
>> +      && gdbarch_tagged_address_p (target_gdbarch (), value))
>> +    return true;
> 
> Hmm, so why only TYPE_CODE_PTR in the new code but TYPE_CODE_REF and
> method/member pointers in the old?
> 

My assessment of the situation wasn't entirely correct (too optimistic). 
Trying to handle all of these types in such a high level function is 
very complex and very error-prone. This is, in part, due to how GDB 
bends the handling of some types in order to show the data the user 
wants. Even the type-handling functions run into some conversion issues 
sometimes.

The member pointers, more specifically, are not even regular pointers, 
but a TYPE_CODE_INT of 16 bytes. Trying to interpret that as a pointer 
so we can do memory tag operations runs into conversion limitations of 
GDB's type-casting system.

Handling only TYPE_CODE_PTR is guaranteed to have the desired effect, 
since we want to be able to show the pointer's logical tag and also 
potentially want to set the allocation tag for the memory tag granule 
that pointer points to. A value of type TYPE_CODE_PTR will always have a 
valid value_address () return.

We can handle all the other types in the default way. There is obviously 
room for improvement in terms of what types we should/should not handle, 
but right now handling TYPE_CODE_PTR gives very useful results for the 
most common use cases.

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

[John Baldwin]

On 5/28/21 12:18 PM, Luis Machado wrote:
> On 5/28/21 4:08 PM, John Baldwin wrote:
>> On 5/18/21 1:20 PM, Luis Machado via Gdb-patches wrote:
>>> When the architecture supports memory tagging, we handle pointer types
>>> in a special way, so we can validate tags and show mismatches.
>>> @@ -1266,18 +1266,22 @@ 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 ();
>>> +  enum type_code code = value_type (value)->code ();
>>> -      return (code == TYPE_CODE_PTR
>>> -          || code == TYPE_CODE_REF
>>> -          || code == TYPE_CODE_METHODPTR
>>> -          || code == TYPE_CODE_MEMBERPTR);
>>> +  try
>>> +    {
>>> +      if (code == TYPE_CODE_PTR
>>> +      && target_supports_memory_tagging ()
>>> +      && gdbarch_tagged_address_p (target_gdbarch (), value))
>>> +    return true;
>>
>> Hmm, so why only TYPE_CODE_PTR in the new code but TYPE_CODE_REF and
>> method/member pointers in the old?
>>
> 
> My assessment of the situation wasn't entirely correct (too optimistic).
> Trying to handle all of these types in such a high level function is
> very complex and very error-prone. This is, in part, due to how GDB
> bends the handling of some types in order to show the data the user
> wants. Even the type-handling functions run into some conversion issues
> sometimes.
> 
> The member pointers, more specifically, are not even regular pointers,
> but a TYPE_CODE_INT of 16 bytes. Trying to interpret that as a pointer
> so we can do memory tag operations runs into conversion limitations of
> GDB's type-casting system.
> 
> Handling only TYPE_CODE_PTR is guaranteed to have the desired effect,
> since we want to be able to show the pointer's logical tag and also
> potentially want to set the allocation tag for the memory tag granule
> that pointer points to. A value of type TYPE_CODE_PTR will always have a
> valid value_address () return.
> 
> We can handle all the other types in the default way. There is obviously
> room for improvement in terms of what types we should/should not handle,
> but right now handling TYPE_CODE_PTR gives very useful results for the
> most common use cases.

Ok, sounds good to me.  That was the only bit that didn't seem obvious to
me when reading the diff.

-- 
John Baldwin

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

[Simon Marchi]

On 2021-05-18 4:20 p.m., Luis Machado via Gdb-patches wrote:
> When the architecture supports memory tagging, we handle pointer types
> in a special way, so we can validate tags and show mismatches.
> 
> I noticed some errors while printing composite types, floats, references,
> member functions and other things that implicitly get dereferenced by GDB to
> show the contents rather than the pointer.
> 
> Vector registers:
> 
> (gdb) p $v0
> Value can't be converted to integer.
> 
> Non-existent internal variables:
> 
> (gdb) p $foo
> Value can't be converted to integer.
> 
> The same happens for complex types and printing struct/union types.
> 
> There are a couple problems. The first one is that we try to evaluate the
> expression to print and eventually call value_as_address (...) before making
> sure we have a suitable TYPE_CODE_PTR type. That may throw for some types. We
> fix this by making sure we have a TYPE_CODE_PTR first, and then proceed to
> check if we need to validate tags.
> 
> The second problem is that the evaluation process may naturally throw an
> error.  One such case is when we have an optimized out variable. Thus we
> guard the evaluation path with a try/catch.
> 
> If the evaluation throws, we want to resume the default expression printing
> path instead of erroring out and printing nothing useful.
> 
> This isn't ideal, but GDB does some magic, internally, in order to provide an
> improved user experience. This allows users to print the contents of some types
> without having to use the dereference operator.
> 
> With the patch, printing works correctly again:
> 
> (gdb) p $v0
> $1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
>       3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
>       791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
>       1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
>       0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
>       12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
>       62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
> (gdb) p $foo
> $2 = void
> (gdb) p 2 + 2i
> $3 = 2 + 2i
> 
> gdb/ChangeLog
> 
> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> 
> 	* printcmd.c (should_validate_memtags): Make more robust against
> 	exceptions.

It's a bit hard to tell if this is right, but I'm with trusting you on
that given that you have worked with this more than anyone else.

I'm just curious, can you give an example of something that can throw in

      if (code == TYPE_CODE_PTR
	  && target_supports_memory_tagging ()
	  && gdbarch_tagged_address_p (target_gdbarch (), value))
	return true;

?  Please give a backtrace of the spot that throws.

I wonder what kind of errors are caught by this "catch".  Are they
frequent and / or benign errors that it's fine to swallow, or would they
deserve a warning?  Would it deserve some logging (shown when some "set
debug foo" is enabled)?

Simon

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

[Luis Machado]

On 5/28/21 10:26 PM, Simon Marchi wrote:
> On 2021-05-18 4:20 p.m., Luis Machado via Gdb-patches wrote:
>> When the architecture supports memory tagging, we handle pointer types
>> in a special way, so we can validate tags and show mismatches.
>>
>> I noticed some errors while printing composite types, floats, references,
>> member functions and other things that implicitly get dereferenced by GDB to
>> show the contents rather than the pointer.
>>
>> Vector registers:
>>
>> (gdb) p $v0
>> Value can't be converted to integer.
>>
>> Non-existent internal variables:
>>
>> (gdb) p $foo
>> Value can't be converted to integer.
>>
>> The same happens for complex types and printing struct/union types.
>>
>> There are a couple problems. The first one is that we try to evaluate the
>> expression to print and eventually call value_as_address (...) before making
>> sure we have a suitable TYPE_CODE_PTR type. That may throw for some types. We
>> fix this by making sure we have a TYPE_CODE_PTR first, and then proceed to
>> check if we need to validate tags.
>>
>> The second problem is that the evaluation process may naturally throw an
>> error.  One such case is when we have an optimized out variable. Thus we
>> guard the evaluation path with a try/catch.
>>
>> If the evaluation throws, we want to resume the default expression printing
>> path instead of erroring out and printing nothing useful.
>>
>> This isn't ideal, but GDB does some magic, internally, in order to provide an
>> improved user experience. This allows users to print the contents of some types
>> without having to use the dereference operator.
>>
>> With the patch, printing works correctly again:
>>
>> (gdb) p $v0
>> $1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
>>        3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
>>        791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
>>        1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
>>        0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
>>        12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
>>        62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
>> (gdb) p $foo
>> $2 = void
>> (gdb) p 2 + 2i
>> $3 = 2 + 2i
>>
>> gdb/ChangeLog
>>
>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
>>
>> 	* printcmd.c (should_validate_memtags): Make more robust against
>> 	exceptions.
> 
> It's a bit hard to tell if this is right, but I'm with trusting you on
> that given that you have worked with this more than anyone else.

Given we're extending the print command, and given the scope of that 
command is very broad, it is a bit tricky to predict all the things a 
user can feed this command.

The original idea was to handle anything that looked like a pointer, but 
turns out this is a bit too complicated to get right without a lot of 
checks.

So right now the goal is to have the print command extended enough so 
that it is useful and easy to investigate memory tags with it. And 
TYPE_CODE_PTR is mainly what we're looking for.

> 
> I'm just curious, can you give an example of something that can throw in
> 
>        if (code == TYPE_CODE_PTR
> 	  && target_supports_memory_tagging ()
> 	  && gdbarch_tagged_address_p (target_gdbarch (), value))
> 	return true;
> 
> ?  Please give a backtrace of the spot that throws.

One such case is an optimized out variable. I don't have a handy 
backtrace for that right now, but they're very similar in terms of the 
path they take.

The problem comes from calling value_as_address (frame #2) and 
eventually calling unpack_long (frame #1), which may throw.

--

Thread 1 "gdb" hit Breakpoint 1, error (fmt=0xaaaaac336b68 "Value can't 
be converted to integer.") at 
../../../repos/binutils-gdb/gdbsupport/errors.cc:39
39      {
(gdb) bt
#0  error (fmt=0xaaaaac336b68 "Value can't be converted to integer.") at 
../../../repos/binutils-gdb/gdbsupport/errors.cc:39
#1  0x0000aaaaab9f76bc in unpack_long (type=0xaaaaada97e30, 
valaddr=0xaaaaad7b9530 '/' <repeats 16 times>) at 
../../../repos/binutils-gdb/gdb/value.c:2877
#2  0x0000aaaaab9f73bc in value_as_address (val=0xaaaaadb8c440) at 
../../../repos/binutils-gdb/gdb/value.c:2785
#3  0x0000aaaaab14d3c8 in aarch64_linux_tagged_address_p 
(gdbarch=0xaaaaada89920, address=0xaaaaadb8c440) at 
../../../repos/binutils-gdb/gdb/aarch64-linux-tdep.c:1564
#4  0x0000aaaaab4f869c in gdbarch_tagged_address_p 
(gdbarch=0xaaaaada89920, address=0xaaaaadb8c440) at 
../../../repos/binutils-gdb/gdb/gdbarch.c:3361
#5  0x0000aaaaab738074 in should_validate_memtags (value=0xaaaaadb8c440) 
at ../../../repos/binutils-gdb/gdb/printcmd.c:1270
#6  0x0000aaaaab738334 in print_command_1 (args=0xaaaaad442a22 "$v0", 
voidprint=1) at ../../../repos/binutils-gdb/gdb/printcmd.c:1324
#7  0x0000aaaaab7386f0 in print_command (exp=0xaaaaad442a22 "$v0", 
from_tty=1) at ../../../repos/binutils-gdb/gdb/printcmd.c:1435
#8  0x0000aaaaab31b414 in do_const_cfunc (c=0xaaaaad5eb440, 
args=0xaaaaad442a22 "$v0", from_tty=1) at 
../../../repos/binutils-gdb/gdb/cli/cli-decode.c:102
#9  0x0000aaaaab31fc58 in cmd_func (cmd=0xaaaaad5eb440, 
args=0xaaaaad442a22 "$v0", from_tty=1) at 
../../../repos/binutils-gdb/gdb/cli/cli-decode.c:2176
#10 0x0000aaaaab93af78 in execute_command (p=0xaaaaad442a24 "0", 
from_tty=1) at ../../../repos/binutils-gdb/gdb/top.c:674
#11 0x0000aaaaab4aa18c in command_handler (command=0xaaaaad442a20 "p 
$v0") at ../../../repos/binutils-gdb/gdb/event-top.c:588
#12 0x0000aaaaab4aa660 in command_line_handler (rl=...) at 
../../../repos/binutils-gdb/gdb/event-top.c:773
#13 0x0000aaaaab96c840 in tui_command_line_handler (rl=...) at 
../../../repos/binutils-gdb/gdb/tui/tui-interp.c:268
#14 0x0000aaaaab4a9778 in gdb_rl_callback_handler (rl=0xaaaaadb8e5c0 "p 
$v0") at ../../../repos/binutils-gdb/gdb/event-top.c:218
#15 0x0000aaaaaba5d728 in rl_callback_read_char () at 
../../../../repos/binutils-gdb/readline/readline/callback.c:281
#16 0x0000aaaaab4a9554 in gdb_rl_callback_read_char_wrapper_noexcept () 
at ../../../repos/binutils-gdb/gdb/event-top.c:176
#17 0x0000aaaaab4a9620 in gdb_rl_callback_read_char_wrapper 
(client_data=0xaaaaad442730) at 
../../../repos/binutils-gdb/gdb/event-top.c:193
#18 0x0000aaaaab4a9f34 in stdin_event_handler (error=0, 
client_data=0xaaaaad442730) at 
../../../repos/binutils-gdb/gdb/event-top.c:515
#19 0x0000aaaaac176a0c in handle_file_event (file_ptr=0xaaaaad7c09a0, 
ready_mask=1) at ../../../repos/binutils-gdb/gdbsupport/event-loop.cc:575
#20 0x0000aaaaac176f3c in gdb_wait_for_event (block=1) at 
../../../repos/binutils-gdb/gdbsupport/event-loop.cc:701
#21 0x0000aaaaac175b44 in gdb_do_one_event () at 
../../../repos/binutils-gdb/gdbsupport/event-loop.cc:237
#22 0x0000aaaaab63bb30 in start_event_loop () at 
../../../repos/binutils-gdb/gdb/main.c:421
#23 0x0000aaaaab63bc90 in captured_command_loop () at 
../../../repos/binutils-gdb/gdb/main.c:481
#24 0x0000aaaaab63d6e8 in captured_main (data=0xfffffffff1b0) at 
../../../repos/binutils-gdb/gdb/main.c:1353
#25 0x0000aaaaab63d740 in gdb_main (args=0xfffffffff1b0) at 
../../../repos/binutils-gdb/gdb/main.c:1368
#26 0x0000aaaaab1478a4 in main (argc=4, argv=0xfffffffff328) at 
../../../repos/binutils-gdb/gdb/gdb.c:32

--

> 
> I wonder what kind of errors are caught by this "catch".  Are they
> frequent and / or benign errors that it's fine to swallow, or would they
> deserve a warning?  Would it deserve some logging (shown when some "set
> debug foo" is enabled)?

The errors are basically type conversion errors (trying to cast a 
TYPE_CODE_MEMBERPTR to a TYPE_CODE_PTR, or a failure to fetch the 
contents of an optimized out variable. One example of such an error is 
in the backtrace above, when trying to print a vector register with an 
unpatched GDB.

If there's an error during memory tag validation, we want to catch it 
and continue printing the original expression without bothering with 
tags. So I consider the failure to validate tags a benign error.

With that said, we do want to catch errors not related to casting types 
or fetching the contents of optimized out variables. If a ptrace call 
fails or if the remote layer goes bad, we want to stop and report that.

I think I need to filter particular types of errors, letting some 
through and catching others. I wonder if I can do that in a simple way.

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

[Joel Brobecker]

Hi Luis,

On Sat, May 29, 2021 at 03:26:42AM -0300, Luis Machado via Gdb-patches wrote:
> On 5/28/21 10:26 PM, Simon Marchi wrote:
> > On 2021-05-18 4:20 p.m., Luis Machado via Gdb-patches wrote:
> > > When the architecture supports memory tagging, we handle pointer types
> > > in a special way, so we can validate tags and show mismatches.
> > > 
> > > I noticed some errors while printing composite types, floats, references,
> > > member functions and other things that implicitly get dereferenced by GDB to
> > > show the contents rather than the pointer.
> > > 
> > > Vector registers:
> > > 
> > > (gdb) p $v0
> > > Value can't be converted to integer.
> > > 
> > > Non-existent internal variables:
> > > 
> > > (gdb) p $foo
> > > Value can't be converted to integer.
> > > 
> > > The same happens for complex types and printing struct/union types.
> > > 
> > > There are a couple problems. The first one is that we try to evaluate the
> > > expression to print and eventually call value_as_address (...) before making
> > > sure we have a suitable TYPE_CODE_PTR type. That may throw for some types. We
> > > fix this by making sure we have a TYPE_CODE_PTR first, and then proceed to
> > > check if we need to validate tags.
> > > 
> > > The second problem is that the evaluation process may naturally throw an
> > > error.  One such case is when we have an optimized out variable. Thus we
> > > guard the evaluation path with a try/catch.
> > > 
> > > If the evaluation throws, we want to resume the default expression printing
> > > path instead of erroring out and printing nothing useful.
> > > 
> > > This isn't ideal, but GDB does some magic, internally, in order to provide an
> > > improved user experience. This allows users to print the contents of some types
> > > without having to use the dereference operator.
> > > 
> > > With the patch, printing works correctly again:
> > > 
> > > (gdb) p $v0
> > > $1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
> > >        3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
> > >        791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
> > >        1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
> > >        0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
> > >        12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
> > >        62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
> > > (gdb) p $foo
> > > $2 = void
> > > (gdb) p 2 + 2i
> > > $3 = 2 + 2i
> > > 
> > > gdb/ChangeLog
> > > 
> > > YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> > > 
> > > 	* printcmd.c (should_validate_memtags): Make more robust against
> > > 	exceptions.
> > 
> > It's a bit hard to tell if this is right, but I'm with trusting you on
> > that given that you have worked with this more than anyone else.
> 
> Given we're extending the print command, and given the scope of that command
> is very broad, it is a bit tricky to predict all the things a user can feed
> this command.
> 
> The original idea was to handle anything that looked like a pointer, but
> turns out this is a bit too complicated to get right without a lot of
> checks.
> 
> So right now the goal is to have the print command extended enough so that
> it is useful and easy to investigate memory tags with it. And TYPE_CODE_PTR
> is mainly what we're looking for.
> 
> > 
> > I'm just curious, can you give an example of something that can throw in
> > 
> >        if (code == TYPE_CODE_PTR
> > 	  && target_supports_memory_tagging ()
> > 	  && gdbarch_tagged_address_p (target_gdbarch (), value))
> > 	return true;
> > 
> > ?  Please give a backtrace of the spot that throws.
> 
> One such case is an optimized out variable. I don't have a handy backtrace
> for that right now, but they're very similar in terms of the path they take.
> 
> The problem comes from calling value_as_address (frame #2) and eventually
> calling unpack_long (frame #1), which may throw.

I am not opposed to an all encompassing exception handler in
should_validate_memtags, but what patch highlights is the fact
that gdbarch_tagged_address_p ends up throwing an exception,
which is a bit surprising to me. Out of curiosity, I searched
for other uses of that function, and I found a handful of calls
in printcmd.c. Does the above mean we need to protect them
from throwing as well?

Have you considered the idea of preventing that method from throwing
with an udpate to the documentation that says gdbarch_tagged_address_p
should never throw?

Otherwise, if we want to allow this method to throw (no strong opinion
in this case), I think the comment in the catch section should be
updated to also explain *why* it's OK to ignore the error. Is it
indicative of memory that's not tagged? Or is it indicative of memory
that *may* be tagged but we're not sure? In the latter case, could we
have a usability question on our hands, where a user is expecting to
see some memory being validated but is not, not understanding why
this is happening?


-- 
Joel

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

[Luis Machado]

Hi,

On 6/20/21 1:19 PM, Joel Brobecker wrote:
> Hi Luis,
> 
> On Sat, May 29, 2021 at 03:26:42AM -0300, Luis Machado via Gdb-patches wrote:
>> On 5/28/21 10:26 PM, Simon Marchi wrote:
>>> On 2021-05-18 4:20 p.m., Luis Machado via Gdb-patches wrote:
>>>> When the architecture supports memory tagging, we handle pointer types
>>>> in a special way, so we can validate tags and show mismatches.
>>>>
>>>> I noticed some errors while printing composite types, floats, references,
>>>> member functions and other things that implicitly get dereferenced by GDB to
>>>> show the contents rather than the pointer.
>>>>
>>>> Vector registers:
>>>>
>>>> (gdb) p $v0
>>>> Value can't be converted to integer.
>>>>
>>>> Non-existent internal variables:
>>>>
>>>> (gdb) p $foo
>>>> Value can't be converted to integer.
>>>>
>>>> The same happens for complex types and printing struct/union types.
>>>>
>>>> There are a couple problems. The first one is that we try to evaluate the
>>>> expression to print and eventually call value_as_address (...) before making
>>>> sure we have a suitable TYPE_CODE_PTR type. That may throw for some types. We
>>>> fix this by making sure we have a TYPE_CODE_PTR first, and then proceed to
>>>> check if we need to validate tags.
>>>>
>>>> The second problem is that the evaluation process may naturally throw an
>>>> error.  One such case is when we have an optimized out variable. Thus we
>>>> guard the evaluation path with a try/catch.
>>>>
>>>> If the evaluation throws, we want to resume the default expression printing
>>>> path instead of erroring out and printing nothing useful.
>>>>
>>>> This isn't ideal, but GDB does some magic, internally, in order to provide an
>>>> improved user experience. This allows users to print the contents of some types
>>>> without having to use the dereference operator.
>>>>
>>>> With the patch, printing works correctly again:
>>>>
>>>> (gdb) p $v0
>>>> $1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
>>>>         3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
>>>>         791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
>>>>         1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
>>>>         0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
>>>>         12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
>>>>         62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
>>>> (gdb) p $foo
>>>> $2 = void
>>>> (gdb) p 2 + 2i
>>>> $3 = 2 + 2i
>>>>
>>>> gdb/ChangeLog
>>>>
>>>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
>>>>
>>>> 	* printcmd.c (should_validate_memtags): Make more robust against
>>>> 	exceptions.
>>>
>>> It's a bit hard to tell if this is right, but I'm with trusting you on
>>> that given that you have worked with this more than anyone else.
>>
>> Given we're extending the print command, and given the scope of that command
>> is very broad, it is a bit tricky to predict all the things a user can feed
>> this command.
>>
>> The original idea was to handle anything that looked like a pointer, but
>> turns out this is a bit too complicated to get right without a lot of
>> checks.
>>
>> So right now the goal is to have the print command extended enough so that
>> it is useful and easy to investigate memory tags with it. And TYPE_CODE_PTR
>> is mainly what we're looking for.
>>
>>>
>>> I'm just curious, can you give an example of something that can throw in
>>>
>>>         if (code == TYPE_CODE_PTR
>>> 	  && target_supports_memory_tagging ()
>>> 	  && gdbarch_tagged_address_p (target_gdbarch (), value))
>>> 	return true;
>>>
>>> ?  Please give a backtrace of the spot that throws.
>>
>> One such case is an optimized out variable. I don't have a handy backtrace
>> for that right now, but they're very similar in terms of the path they take.
>>
>> The problem comes from calling value_as_address (frame #2) and eventually
>> calling unpack_long (frame #1), which may throw.
> 
> I am not opposed to an all encompassing exception handler in
> should_validate_memtags, but what patch highlights is the fact
> that gdbarch_tagged_address_p ends up throwing an exception,
> which is a bit surprising to me. Out of curiosity, I searched
> for other uses of that function, and I found a handful of calls
> in printcmd.c. Does the above mean we need to protect them
> from throwing as well?

The difference between should_validate_memtags and other places calling 
gdbarch_tagged_address_p is that should_validate_memtags may abruptly 
abort a print command just because there was a problem trying to fetch 
memory tags. That should not happen.

The other places calling gdbarch_tagged_address_p are the "x" command, 
which won't throw (due to how we pass the address value) and the memory 
tag commands, which only need to print memory tag information, so they 
can fail and don't need to fallback to doing things in a different way.

With that said, it does seem like guarding should_validate_memtags is a 
bit too broad and ...

> 
> Have you considered the idea of preventing that method from throwing
> with an udpate to the documentation that says gdbarch_tagged_address_p
> should never throw?

... yes, this may be a better approach here. Given 
gdbarch_tagged_address_p may be implemented multiple times, I wanted to 
avoid having each implementation include a try/catch block.

The main problem here is that we only want to validate pointers, but the 
user may feed anything to the "print" command.

Attempting to filter and check by value type is a bit of a lost battle. 
Too many cases to handle. Even the functions dealing with value types 
(eventually calling unpack_long) may throw.

I'll rewrite this with a try/catch block within the bounds of the 
gdbarch_tagged_address_p, a smaller scope and will update the 
documentation accordingly.

Thanks for the feedback!

> 
> Otherwise, if we want to allow this method to throw (no strong opinion
> in this case), I think the comment in the catch section should be
> updated to also explain *why* it's OK to ignore the error. Is it
> indicative of memory that's not tagged? Or is it indicative of memory
> that *may* be tagged but we're not sure? In the latter case, could we
> have a usability question on our hands, where a user is expecting to
> see some memory being validated but is not, not understanding why
> this is happening?
> 
> 

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

[Luis Machado]

Updates on v2:

- Guard against thrown exceptions in the gdbarch_tagged_address_p hook as
  opposed to doing it at a higher scope.

--

When the architecture supports memory tagging, we handle pointer types
in a special way, so we can validate tags and show mismatches.

I noticed some errors while printing composite types, floats, references,
member functions and other things that implicitly get dereferenced by GDB to
show the contents rather than the pointer.

Vector registers:

(gdb) p $v0
Value can't be converted to integer.

Non-existent internal variables:

(gdb) p $foo
Value can't be converted to integer.

The same happens for complex types and printing struct/union types.

There are a couple problems. The first one is that we try to evaluate the
expression to print and eventually call value_as_address (...) before making
sure we have a suitable TYPE_CODE_PTR type. That may throw for some types. We
fix this by making sure we have a TYPE_CODE_PTR first, and then proceed to
check if we need to validate tags.

The second problem is that the evaluation process may naturally throw an
error.  One such case is when we have an optimized out variable. Thus we
guard the evaluation path with a try/catch.

If the evaluation throws, we want to resume the default expression printing
path instead of erroring out and printing nothing useful.

This isn't ideal, but GDB does some magic, internally, in order to provide an
improved user experience. This allows users to print the contents of some types
without having to use the dereference operator.

With the patch, printing works correctly again:

(gdb) p $v0
$1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
      3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
      791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
      1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
      0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
      12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
      62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
(gdb) p $foo
$2 = void
(gdb) p 2 + 2i
$3 = 2 + 2i

gdb/ChangeLog

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

	* gdbarch.sh: Updated documentation for gdbarch_tagged_address_p.
	* gdbarch.h: Regenerate.
	* printcmd.c (should_validate_memtags): Reorder comparisons and only
	validate tags for TYPE_CODE_PTR types.
	* aarch64-linux-tdep.c (aarch64_linux_tagged_address_p): Guard call
	to value_as_address with a try/catch block.
---
 gdb/aarch64-linux-tdep.c | 14 +++++++++++++-
 gdb/gdbarch.h            |  3 ++-
 gdb/gdbarch.sh           |  3 ++-
 gdb/printcmd.c           | 20 ++++++++++----------
 4 files changed, 27 insertions(+), 13 deletions(-)

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.  */
+  try
+    {
+      addr = value_as_address (address);
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      /* Give up and assume the address is untagged.  */
+      return false;
+    }
 
   /* Remove the top byte for the memory range check.  */
   addr = address_significant (gdbarch, addr);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 7157e5596fd..4118e6c37ef 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.  The
+   implementation of this hook should never throw an exception. */
 
 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 43e51341f97..77088228d9a 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.  The
+# implementation of this hook should never throw an exception.
 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 22fa5c074d1..cd7d002c503 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1266,18 +1266,18 @@ 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);
+  if (value == nullptr)
+    return false;
 
-      enum type_code code = value_type (value)->code ();
+  enum type_code code = value_type (value)->code ();
+
+  /* Only validate memory tags if we have a pointer type and if the address
+     is within a tagged memory area.  */
+  if (code == TYPE_CODE_PTR
+      && target_supports_memory_tagging ()
+      && gdbarch_tagged_address_p (target_gdbarch (), value))
+	return true;
 
-      return (code == TYPE_CODE_PTR
-	      || code == TYPE_CODE_REF
-	      || code == TYPE_CODE_METHODPTR
-	      || code == TYPE_CODE_MEMBERPTR);
-    }
   return false;
 }
 
-- 
2.25.1

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

[Luis Machado]

On 6/21/21 10:37 PM, Luis Machado wrote:
> Updates on v2:
> 
> - Guard against thrown exceptions in the gdbarch_tagged_address_p hook as
>    opposed to doing it at a higher scope.
> 
> --
> 
> When the architecture supports memory tagging, we handle pointer types
> in a special way, so we can validate tags and show mismatches.
> 
> I noticed some errors while printing composite types, floats, references,
> member functions and other things that implicitly get dereferenced by GDB to
> show the contents rather than the pointer.
> 
> Vector registers:
> 
> (gdb) p $v0
> Value can't be converted to integer.
> 
> Non-existent internal variables:
> 
> (gdb) p $foo
> Value can't be converted to integer.
> 
> The same happens for complex types and printing struct/union types.
> 
> There are a couple problems. The first one is that we try to evaluate the
> expression to print and eventually call value_as_address (...) before making
> sure we have a suitable TYPE_CODE_PTR type. That may throw for some types. We
> fix this by making sure we have a TYPE_CODE_PTR first, and then proceed to
> check if we need to validate tags.
> 
> The second problem is that the evaluation process may naturally throw an
> error.  One such case is when we have an optimized out variable. Thus we
> guard the evaluation path with a try/catch.
> 
> If the evaluation throws, we want to resume the default expression printing
> path instead of erroring out and printing nothing useful.
> 
> This isn't ideal, but GDB does some magic, internally, in order to provide an
> improved user experience. This allows users to print the contents of some types
> without having to use the dereference operator.
> 
> With the patch, printing works correctly again:
> 
> (gdb) p $v0
> $1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
>        3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
>        791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
>        1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
>        0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
>        12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
>        62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
> (gdb) p $foo
> $2 = void
> (gdb) p 2 + 2i
> $3 = 2 + 2i
> 
> gdb/ChangeLog
> 
> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> 
> 	* gdbarch.sh: Updated documentation for gdbarch_tagged_address_p.
> 	* gdbarch.h: Regenerate.
> 	* printcmd.c (should_validate_memtags): Reorder comparisons and only
> 	validate tags for TYPE_CODE_PTR types.
> 	* aarch64-linux-tdep.c (aarch64_linux_tagged_address_p): Guard call
> 	to value_as_address with a try/catch block.
> ---
>   gdb/aarch64-linux-tdep.c | 14 +++++++++++++-
>   gdb/gdbarch.h            |  3 ++-
>   gdb/gdbarch.sh           |  3 ++-
>   gdb/printcmd.c           | 20 ++++++++++----------
>   4 files changed, 27 insertions(+), 13 deletions(-)
> 
> 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.  */
> +  try
> +    {
> +      addr = value_as_address (address);
> +    }
> +  catch (const gdb_exception_error &ex)
> +    {
> +      /* Give up and assume the address is untagged.  */
> +      return false;
> +    }
>   
>     /* Remove the top byte for the memory range check.  */
>     addr = address_significant (gdbarch, addr);
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 7157e5596fd..4118e6c37ef 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.  The
> +   implementation of this hook should never throw an exception. */
>   
>   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 43e51341f97..77088228d9a 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.  The
> +# implementation of this hook should never throw an exception.
>   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 22fa5c074d1..cd7d002c503 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1266,18 +1266,18 @@ 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);
> +  if (value == nullptr)
> +    return false;
>   
> -      enum type_code code = value_type (value)->code ();
> +  enum type_code code = value_type (value)->code ();
> +
> +  /* Only validate memory tags if we have a pointer type and if the address
> +     is within a tagged memory area.  */
> +  if (code == TYPE_CODE_PTR
> +      && target_supports_memory_tagging ()
> +      && gdbarch_tagged_address_p (target_gdbarch (), value))
> +	return true;
>   
> -      return (code == TYPE_CODE_PTR
> -	      || code == TYPE_CODE_REF
> -	      || code == TYPE_CODE_METHODPTR
> -	      || code == TYPE_CODE_MEMBERPTR);
> -    }
>     return false;
>   }
>   
> 

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

[Pedro Alves]

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.

And then, I think you should make the higher level code in printcmd.c try/catch as you
were doing in the original patch, but make it print the error and continue with normal
printing.

   warning/exception_fprintf (_("could not validate tag: %s", ex.message->c_str ());

I don't think you should guarantee that the hook doesn't throw an error.  After all,
you can always get a "remote target closed" exception or a "file not found" error
trying to read the tags.

Pedro Alves

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

[Luis Machado]

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.

The optimized out case is one example of how this can throw.

> 
> And then, I think you should make the higher level code in printcmd.c try/catch as you
> were doing in the original patch, but make it print the error and continue with normal
> printing.

I don't think that would be desirable. Since we're dealing with a print 
command, we want to fail gracefully and silently (when possible) so we 
don't disturb the user experience of attempting to print some 
expression. We only want to report non-recoverable errors, like a 
dropped remote connection, file not found etc.

> 
>     warning/exception_fprintf (_("could not validate tag: %s", ex.message->c_str ());
> 
> I don't think you should guarantee that the hook doesn't throw an error.  After all,
> you can always get a "remote target closed" exception or a "file not found" error
> trying to read the tags.

It is true. There is no guarantee it won't throw from reading a remote 
file right now. I'll update this comment. We only want this hook to 
throw for non-recoverable errors.

> 
> Pedro Alves
> 

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

[Pedro Alves]

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?

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.

> 
> The optimized out case is one example of how this can throw.
> 
>>
>> And then, I think you should make the higher level code in printcmd.c try/catch as you
>> were doing in the original patch, but make it print the error and continue with normal
>> printing.
> 
> I don't think that would be desirable. Since we're dealing with a print command, we want to fail gracefully and silently (when possible) so we don't disturb the user experience of attempting to print some expression. We only want to report non-recoverable errors, like a dropped remote connection, file not found etc.

Swallowing errors potentially hides useful information from the user, or even masks bugs.

Note that the "file not found" error here (failing to open the /proc file, maybe kernel
doesn't have it or something) is not an unrecoverable error -- it means you can't validate
the tags, but you can still go ahead and print the value.

> 
>>
>>     warning/exception_fprintf (_("could not validate tag: %s", ex.message->c_str ());
>>
>> I don't think you should guarantee that the hook doesn't throw an error.  After all,
>> you can always get a "remote target closed" exception or a "file not found" error
>> trying to read the tags.
> 
> It is true. There is no guarantee it won't throw from reading a remote file right now. I'll update this comment. We only want this hook to throw for non-recoverable errors.

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

[Luis Machado]

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.

> 
> 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.

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.

> 
>>
>> The optimized out case is one example of how this can throw.
>>
>>>
>>> And then, I think you should make the higher level code in printcmd.c try/catch as you
>>> were doing in the original patch, but make it print the error and continue with normal
>>> printing.
>>
>> I don't think that would be desirable. Since we're dealing with a print command, we want to fail gracefully and silently (when possible) so we don't disturb the user experience of attempting to print some expression. We only want to report non-recoverable errors, like a dropped remote connection, file not found etc.
> 
> Swallowing errors potentially hides useful information from the user, or even masks bugs.
> 

Sure, but not all errors are useful. "Invalid cast", "optimized out" and 
"That operation is not available on integers of more than X bytes" are 
not useful in this context. If the variable is optimized out, the print 
command will show that anyway.

This mechanism should blend in nicely. It shouldn't be intrusive and 
cause the verbosity to be annoying. More importantly, we don't want this 
to cause tons of failures in the testsuite either.

> Note that the "file not found" error here (failing to open the /proc file, maybe kernel
> doesn't have it or something) is not an unrecoverable error -- it means you can't validate
> the tags, but you can still go ahead and print the value.
> 

Yeah, that's true. But a missing file is a pretty serious problem, and I 
think that should be reported. It should not happen.

So we have two categories of errors, the ones we should ignore and the 
ones we should report.

>>
>>>
>>>      warning/exception_fprintf (_("could not validate tag: %s", ex.message->c_str ());
>>>
>>> I don't think you should guarantee that the hook doesn't throw an error.  After all,
>>> you can always get a "remote target closed" exception or a "file not found" error
>>> trying to read the tags.
>>
>> It is true. There is no guarantee it won't throw from reading a remote file right now. I'll update this comment. We only want this hook to throw for non-recoverable errors.

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

[Pedro Alves]

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?

> 
>>
>> 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?

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.

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

[Luis Machado]

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.

>>
>>>
>>> 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.

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.

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.

> 
> 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.

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.

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

[Pedro Alves]

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.

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

> 
>>>
>>>>
>>>> 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.

> 
> 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.

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.

> 
>>
>> 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.

> 
> 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.

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.

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
-- 
2.26.2

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

[Luis Machado]

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
> 

[PATCH v3] Fix printing of non-address types when memory tagging is enabled

[Pedro Alves]

On 2021-07-06 5:32 p.m., Luis Machado wrote:
> On 7/5/21 8:06 PM, Pedro Alves wrote:
>> Hi!
>>
>> On 2021-07-05 3:32 p.m., Luis Machado wrote:

>>>>>> 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.

Sorry, but that doesn't make sense -- there's no mainline arch with 128-bit pointers.  There can't be, it would
hit that issue all over the place.  Any arch with 128-bit pointers will need to implement gdbarch_pointer_to_address
and thus that exception won't be hit.

>>>
>>> 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.

OK.

> 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.

That's not the case for references, there's no bending here.  References are just pointers with a different sugar coat.
GDB automatically prints the object the reference references, but that's unrelated -- it'd be like GDB automatically
deferencing pointers.  Reading the reference itself is just like reading a pointer.

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


>> 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.

Heh.  Curious.  So it wasn't possible to run the testsuite on such a setup before the series went in?  Or what changed
since then?

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

Hmm, the fact that this wasn't noticed before made me assume that memory tag checking was off by default, so I was
thinking of some tests that turn it on and print non-pointer/reference things.  But I now see that it's on by default,
so forget about it.

Here's the patch again, now with updated git commit log.  Please double check.
The only code change is that I dropped the ending period in the fprintf call, as some
errors already end with a period.

I updated the ChangeLog as I suspect you'll want this in the release branch.

For putting it on the branch we'll need a bugzilla bug though, so I'll leave it to
you from here: file a bug, and then mentioning the bug in the git commit log, and
then merging this, master and branch.

From a20c524db9c58fa7ee57aa0cb05d934b267965ce Mon Sep 17 00:00:00 2001
From: Luis Machado <luis.machado@linaro.org>
Date: Mon, 5 Jul 2021 18:51:11 +0100
Subject: [PATCH] Fix printing of non-address types when memory tagging is
 enabled

When the architecture supports memory tagging, we handle
pointer/reference types in a special way, so we can validate tags and
show mismatches.

Unfortunately, the currently implementation errors out when the user
prints non-address values: composite types, floats, references, member
functions and other things.

Vector registers:

 (gdb) p $v0
 Value can't be converted to integer.

Non-existent internal variables:

 (gdb) p $foo
 Value can't be converted to integer.

The same happens for complex types and printing struct/union types.

There are a few problems here.

The first one is that after print_command_1 evaluates the expression
to print, the tag validation code call value_as_address
unconditionally, without making sure we have have a suitable type
where it makes to sense to call it.  That results in value_as_address
(if it isn't given a pointer-like type) trying to treat the value as
an integer and convert it to an address, which #1 - doesn't make sense
(i.e., no sense in validating tags after "print 1"), and throws for
non-integer-convertible types.  We fix this by making sure we have a
pointer or reference type first, and only if so then proceed to check
if the address-like value has tags.

The second is that we're calling value_as_address even if we have an
optimized out or unavailable value, which throws, because the value's
contents aren't fully accessible/readable.  This error currently
escapes out and aborts the print.  This case is fixed by checking for
optimized out / unavailable explicitly.

Third, the tag checking process does not gracefully handle exceptions.
If any exception is thrown from the tag validation code, we abort the
print.  E.g., the target may fail to access tags via a running thread.
Or the needed /proc files aren't available.  Or some other untold
reason.  This is a bit too rigid.  This commit changes print_command_1
to catch errors, print them, and still continue with the normal
expression printing path instead of erroring out and printing nothing
useful.

With this patch, printing works correctly again:

 (gdb) p $v0
 $1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
       3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
       791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
       1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
       0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
       12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
       62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
 (gdb) p $foo
 $2 = void
 (gdb) p 2 + 2i
 $3 = 2 + 2i

gdb/ChangeLog
YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
	    Pedro Alves  <pedro@palves.net>

	* gdbarch.sh: Updated documentation for gdbarch_tagged_address_p.
	* gdbarch.h: Regenerate.
	* printcmd.c (should_validate_memtags): Reorder comparisons and only
	validate tags for pointer and reference types.  Skip validation of
	optimized out or unavailable values.
	(print_command_1): Guard call memory tagging validation code with
	a try/catch block.

Co-Authored-By: Pedro Alves <pedro@palves.net>
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..416b87c69c6 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
-- 
2.26.2

Re: [PATCH v3] Fix printing of non-address types when memory tagging is enabled

[Joel Brobecker]

Hey Pedro, hey Luis,

> gdb/ChangeLog
> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> 	    Pedro Alves  <pedro@palves.net>
> 
> 	* gdbarch.sh: Updated documentation for gdbarch_tagged_address_p.
> 	* gdbarch.h: Regenerate.
> 	* printcmd.c (should_validate_memtags): Reorder comparisons and only
> 	validate tags for pointer and reference types.  Skip validation of
> 	optimized out or unavailable values.
> 	(print_command_1): Guard call memory tagging validation code with
> 	a try/catch block.

What's the status for this version of the patch that Pedro suggested?
Do we think this is something we could approve and push sometime soon?

Thanks!
-- 
Joel

Re: [PATCH v3] Fix printing of non-address types when memory tagging is enabled

[Luis Machado]

Hi,

On 7/17/21 3:53 PM, Joel Brobecker wrote:
> Hey Pedro, hey Luis,
> 
>> gdb/ChangeLog
>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
>> 	    Pedro Alves  <pedro@palves.net>
>>
>> 	* gdbarch.sh: Updated documentation for gdbarch_tagged_address_p.
>> 	* gdbarch.h: Regenerate.
>> 	* printcmd.c (should_validate_memtags): Reorder comparisons and only
>> 	validate tags for pointer and reference types.  Skip validation of
>> 	optimized out or unavailable values.
>> 	(print_command_1): Guard call memory tagging validation code with
>> 	a try/catch block.
> 
> What's the status for this version of the patch that Pedro suggested?
> Do we think this is something we could approve and push sometime soon?

I'm coming back from vacation today. I'll check.

> 
> Thanks!
> 

Re: [PATCH v3] Fix printing of non-address types when memory tagging is enabled

[Luis Machado]

On 7/7/21 7:49 AM, Pedro Alves wrote:
> On 2021-07-06 5:32 p.m., Luis Machado wrote:
>> On 7/5/21 8:06 PM, Pedro Alves wrote:
>>> Hi!
>>>
>>> On 2021-07-05 3:32 p.m., Luis Machado wrote:
> 
>>>>>>> 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.
> 
> Sorry, but that doesn't make sense -- there's no mainline arch with 128-bit pointers.  There can't be, it would
> hit that issue all over the place.  Any arch with 128-bit pointers will need to implement gdbarch_pointer_to_address
> and thus that exception won't be hit.
> 

Yes. But when I say this is mainline GDB, I mean I'm not aiming at 
fixing 128-bit pointers, but I'm merely anticipating that 128-bit 
pointers being fed to this code pose a problem that should be fixed.

>>>>
>>>> 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.
> 
> OK.
> 
>> 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.
> 
> That's not the case for references, there's no bending here.  References are just pointers with a different sugar coat.
> GDB automatically prints the object the reference references, but that's unrelated -- it'd be like GDB automatically
> deferencing pointers.  Reading the reference itself is just like reading a pointer.
> 

Sounds good then.

>>
>> These failures showed up in the testsuite by the way. That's why I wrote the patch.
>>
> 
> 
>>> 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.
> 
> Heh.  Curious.  So it wasn't possible to run the testsuite on such a setup before the series went in?  Or what changed
> since then?
> 

MTE is not available on hardware yet. It also wasn't available on a 
mainline kernel until some time ago. Therefore it needs to be exercised 
from within an emulator environment. A GDB testsuite run on a setup like 
that takes a few hours to complete.

So it was perfectly possible to run the testsuite on such a setup. I did 
it a number of times, and again when the changes went in. Hence why I 
wrote the patch when I noticed the failures (that weren't critical 
really, as aarch64 is the only target supporting memory tagging with the 
new interface, and that is not available on hardware yet).

Between copying GDB builds, updated kernels and filtering testsuite 
runs, a mistake might have happened that prevented me from noticing this 
earlier.


>>
>> What sort of test did you have in mind, other than gdb.arch/aarch64-mte.exp, which already covers printing things and adjusting tags?
> 
> Hmm, the fact that this wasn't noticed before made me assume that memory tag checking was off by default, so I was
> thinking of some tests that turn it on and print non-pointer/reference things.  But I now see that it's on by default,
> so forget about it.
> 
> Here's the patch again, now with updated git commit log.  Please double check.
> The only code change is that I dropped the ending period in the fprintf call, as some
> errors already end with a period.

I gave it a try again and it looks OK testsuite-wise.

> 
> I updated the ChangeLog as I suspect you'll want this in the release branch.
> 
> For putting it on the branch we'll need a bugzilla bug though, so I'll leave it to
> you from here: file a bug, and then mentioning the bug in the git commit log, and
> then merging this, master and branch.
> 
>  From a20c524db9c58fa7ee57aa0cb05d934b267965ce Mon Sep 17 00:00:00 2001
> From: Luis Machado <luis.machado@linaro.org>
> Date: Mon, 5 Jul 2021 18:51:11 +0100
> Subject: [PATCH] Fix printing of non-address types when memory tagging is
>   enabled
> 
> When the architecture supports memory tagging, we handle
> pointer/reference types in a special way, so we can validate tags and
> show mismatches.
> 
> Unfortunately, the currently implementation errors out when the user
> prints non-address values: composite types, floats, references, member
> functions and other things.
> 
> Vector registers:
> 
>   (gdb) p $v0
>   Value can't be converted to integer.
> 
> Non-existent internal variables:
> 
>   (gdb) p $foo
>   Value can't be converted to integer.
> 
> The same happens for complex types and printing struct/union types.
> 
> There are a few problems here.
> 
> The first one is that after print_command_1 evaluates the expression
> to print, the tag validation code call value_as_address
> unconditionally, without making sure we have have a suitable type
> where it makes to sense to call it.  That results in value_as_address
> (if it isn't given a pointer-like type) trying to treat the value as
> an integer and convert it to an address, which #1 - doesn't make sense
> (i.e., no sense in validating tags after "print 1"), and throws for
> non-integer-convertible types.  We fix this by making sure we have a
> pointer or reference type first, and only if so then proceed to check
> if the address-like value has tags.
> 
> The second is that we're calling value_as_address even if we have an
> optimized out or unavailable value, which throws, because the value's
> contents aren't fully accessible/readable.  This error currently
> escapes out and aborts the print.  This case is fixed by checking for
> optimized out / unavailable explicitly.
> 
> Third, the tag checking process does not gracefully handle exceptions.
> If any exception is thrown from the tag validation code, we abort the
> print.  E.g., the target may fail to access tags via a running thread.
> Or the needed /proc files aren't available.  Or some other untold
> reason.  This is a bit too rigid.  This commit changes print_command_1
> to catch errors, print them, and still continue with the normal
> expression printing path instead of erroring out and printing nothing
> useful.
> 
> With this patch, printing works correctly again:
> 
>   (gdb) p $v0
>   $1 = {d = {f = {2.0546950501119882e-81, 2.0546950501119882e-81}, u = {3399988123389603631, 3399988123389603631}, s = {
>         3399988123389603631, 3399988123389603631}}, s = {f = {1.59329203e-10, 1.59329203e-10, 1.59329203e-10, 1.59329203e-10}, u = {
>         791621423, 791621423, 791621423, 791621423}, s = {791621423, 791621423, 791621423, 791621423}}, h = {bf = {1.592e-10,
>         1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10, 1.592e-10}, f = {0.11224, 0.11224, 0.11224, 0.11224, 0.11224,
>         0.11224, 0.11224, 0.11224}, u = {12079, 12079, 12079, 12079, 12079, 12079, 12079, 12079}, s = {12079, 12079, 12079, 12079,
>         12079, 12079, 12079, 12079}}, b = {u = {47 <repeats 16 times>}, s = {47 <repeats 16 times>}}, q = {u = {
>         62718710765820030520700417840365121327}, s = {62718710765820030520700417840365121327}}}
>   (gdb) p $foo
>   $2 = void
>   (gdb) p 2 + 2i
>   $3 = 2 + 2i
> 
> gdb/ChangeLog
> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> 	    Pedro Alves  <pedro@palves.net>
> 
> 	* gdbarch.sh: Updated documentation for gdbarch_tagged_address_p.
> 	* gdbarch.h: Regenerate.
> 	* printcmd.c (should_validate_memtags): Reorder comparisons and only
> 	validate tags for pointer and reference types.  Skip validation of
> 	optimized out or unavailable values.
> 	(print_command_1): Guard call memory tagging validation code with
> 	a try/catch block.
> 
> Co-Authored-By: Pedro Alves <pedro@palves.net>
> 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..416b87c69c6 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
>