Fix printing of non pointer types when memory tagging is enabled

Message ID 20210622013719.3750766-1-luis.machado@linaro.org
State New
Headers show
Series
  • Fix printing of non pointer types when memory tagging is enabled
Related show

Commit Message

Luis Machado via Gdb-patches June 22, 2021, 1:37 a.m.
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(-)

-- 
2.25.1

Comments

Luis Machado via Gdb-patches July 1, 2021, 1:50 p.m. | #1
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;

>   }

>   

>
Pedro Alves July 1, 2021, 7:52 p.m. | #2
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
Luis Machado via Gdb-patches July 2, 2021, 12:47 p.m. | #3
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

>
Pedro Alves July 2, 2021, 1:19 p.m. | #4
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.
Luis Machado via Gdb-patches July 2, 2021, 1:45 p.m. | #5
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.
Pedro Alves July 2, 2021, 3:14 p.m. | #6
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.
Luis Machado via Gdb-patches July 5, 2021, 2:32 p.m. | #7
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.
Pedro Alves July 5, 2021, 11:06 p.m. | #8
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
Luis Machado via Gdb-patches July 6, 2021, 4:32 p.m. | #9
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

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;
 }