[PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Alan Hayward
Armv8.3-a Pointer Authentication causes the function return address to be
obfuscated on entry to some functions. GDB must unmask the link register in
order to produce a backtrace.

The following patch adds markers of <unmasked> to the bracktrace, to indicate
which addresses needed unmasking.

For example, consider the following backtrace:

(gdb) bt
0  0x0000000000400490 in puts@plt ()
1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
2  0x0000000000400604<unmasked> in bar () at cbreak-lib.c:12
3  0x0000000000400620<unmasked> in barbar () at cbreak.c:17
4  0x00000000004005b4 in main () at cbreak-3.c:10

The functions in the cbreak-lib use pointer auth, obfuscating the return address
to the previous function.  The caused the addresses of bar and barbar to require
unmasking in order to unwind the backtrace.

Alternatively, I considered replacing <unmasked> with a single chracter, such
as * for brevity reasons, but felt this would be non obvious for the user.

An extra bool is added alongside the prev_pc in the frame struture.  At the
point at which the link register is unmasked, the AArch64 port calls into frame
to sets the bool.  This seemed the most efficient way of doing it.

aarch64_frame_unmask_address is designed to work for any address, however at
the momoment it is only used for the link address.  An extra default parameter
is added to ensure it only caches for the link register.

I expect this will potentially cause issues with some tests in the testsuite
when Armv8.3 pointer authentication is used.  This should be fixed up in the
the future once real hardware is available for full testsuite testing.

2019-07-17  Alan Hayward  <[hidden email]>

        * NEWS: Expand the Pointer Authentication entry.
        * aarch64-tdep.c (aarch64_frame_unmask_address): Mark frame if
        unmasking link register.
        * frame.c (struct frame_info): Add masked variable.
        (frame_set_previous_pc_masked) (frame_get_pc_masked): New function.
        * frame.h (frame_set_previous_pc_masked) (frame_get_pc_masked): New
        declaration.
        * stack.c (print_frame): Check for masked pc.
---
 gdb/NEWS           |  3 ++-
 gdb/aarch64-tdep.c | 12 +++++++++---
 gdb/frame.c        | 17 +++++++++++++++++
 gdb/frame.h        |  9 +++++++++
 gdb/stack.c        |  6 +++++-
 5 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 4e479bf738..cea88491bd 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -16,7 +16,8 @@
   architectures require kernel changes.  TLS is not yet supported for
   amd64 and i386 process core dumps.
 
-* Support for Pointer Authentication on AArch64 Linux.
+* Support for Pointer Authentication on AArch64 Linux.  Return addresses that
+  required unmasking are shown in the backtrace with the postfix <masked>.
 
 * Two new convenience functions $_cimag and $_creal that extract the
   imaginary and real parts respectively from complex numbers.
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index e23cc3f956..eaac7960ec 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -251,12 +251,13 @@ class instruction_reader : public abstract_instruction_reader
 } // namespace
 
 /* If address signing is enabled, mask off the signature bits from ADDR, using
-   the register values in THIS_FRAME.  */
+   the register values in THIS_FRAME.  IS_LR indicates if ADDR is from the link
+   register for the current frame.  */
 
 static CORE_ADDR
 aarch64_frame_unmask_address (struct gdbarch_tdep *tdep,
-      struct frame_info *this_frame,
-      CORE_ADDR addr)
+      struct frame_info *this_frame, CORE_ADDR addr,
+      bool is_lr = true)
 {
   if (tdep->has_pauth ()
       && frame_unwind_register_unsigned (this_frame,
@@ -265,6 +266,11 @@ aarch64_frame_unmask_address (struct gdbarch_tdep *tdep,
       int cmask_num = AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
       CORE_ADDR cmask = frame_unwind_register_unsigned (this_frame, cmask_num);
       addr = addr & ~cmask;
+
+      /* If addr was taken from the Link Register, then record this in the
+ frame.  */
+      if (is_lr)
+ frame_set_previous_pc_masked (this_frame);
     }
 
   return addr;
diff --git a/gdb/frame.c b/gdb/frame.c
index 84e0397db9..b7bb44986e 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -124,6 +124,8 @@ struct frame_info
   struct {
     enum cached_copy_status status;
     CORE_ADDR value;
+    /* Did VALUE require unmasking when being read.  */
+    bool masked;
   } prev_pc;
   
   /* Cached copy of the previous frame's function address.  */
@@ -161,6 +163,21 @@ struct frame_info
   const char *stop_string;
 };
 
+/* See frame.h.  */
+
+void
+frame_set_previous_pc_masked (struct frame_info *frame)
+{
+  frame->prev_pc.masked = true;
+}
+
+/* See frame.h.  */
+
+bool frame_get_pc_masked (struct frame_info *frame)
+{
+  return frame->next->prev_pc.masked;
+}
+
 /* A frame stash used to speed up frame lookups.  Create a hash table
    to stash frames previously accessed from the frame cache for
    quicker subsequent retrieval.  The hash table is emptied whenever
diff --git a/gdb/frame.h b/gdb/frame.h
index a79eeeeab1..61c12ec0b2 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -930,4 +930,13 @@ extern const gdb::option::option_def set_backtrace_option_defs[2];
 /* The values behind the global "set backtrace ..." settings.  */
 extern set_backtrace_options user_set_backtrace_options;
 
+/* Mark that the PC value is masked for the previous frame.  */
+
+extern void frame_set_previous_pc_masked (struct frame_info *frame);
+
+/* Get whether the PC value is masked for the given frame.  */
+
+extern bool frame_get_pc_masked (struct frame_info *frame);
+
+
 #endif /* !defined (FRAME_H)  */
diff --git a/gdb/stack.c b/gdb/stack.c
index 9b1d1a6856..cef1c29a82 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1254,7 +1254,11 @@ print_frame (const frame_print_options &fp_opts,
  {
   annotate_frame_address ();
   if (pc_p)
-    uiout->field_core_addr ("addr", gdbarch, pc);
+    {
+      uiout->field_core_addr ("addr", gdbarch, pc);
+      if (frame_get_pc_masked (frame))
+ uiout->text ("<unmasked>");
+    }
   else
     uiout->field_string ("addr", "<unavailable>",
  ui_out_style_kind::ADDRESS);
--
2.20.1 (Apple Git-117)

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Pedro Alves-7
On 7/17/19 9:14 AM, Alan Hayward wrote:

> Armv8.3-a Pointer Authentication causes the function return address to be
> obfuscated on entry to some functions. GDB must unmask the link register in
> order to produce a backtrace.
>
> The following patch adds markers of <unmasked> to the bracktrace, to indicate
> which addresses needed unmasking.
>
> For example, consider the following backtrace:
>
> (gdb) bt
> 0  0x0000000000400490 in puts@plt ()
> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
> 2  0x0000000000400604<unmasked> in bar () at cbreak-lib.c:12
> 3  0x0000000000400620<unmasked> in barbar () at cbreak.c:17
> 4  0x00000000004005b4 in main () at cbreak-3.c:10
>
> The functions in the cbreak-lib use pointer auth, obfuscating the return address
> to the previous function.  The caused the addresses of bar and barbar to require
> unmasking in order to unwind the backtrace.
>
> Alternatively, I considered replacing <unmasked> with a single chracter, such
> as * for brevity reasons, but felt this would be non obvious for the user.

I don't have a particular suggestion, though my first reaction was that
it seemed a bit verbose.

IMHO, the marker doesn't have to stand out and be expressive, since users can
always look at the manual.  Once they learn something, often being concise
helps -- or in other words, once you learn what "<unmasked>" or "U" or whatever
is, and you're used to it, what would you rather see?  What's the main
information you're looking for when staring at the backtrace?  Thoughts
like that should guide the output too, IMO.

>
> An extra bool is added alongside the prev_pc in the frame struture.  At the
> point at which the link register is unmasked, the AArch64 port calls into frame
> to sets the bool.  This seemed the most efficient way of doing it.
>
> aarch64_frame_unmask_address is designed to work for any address, however at
> the momoment it is only used for the link address.  An extra default parameter
> is added to ensure it only caches for the link register.

typo: "momoment"

>
> I expect this will potentially cause issues with some tests in the testsuite
> when Armv8.3 pointer authentication is used.  This should be fixed up in the
> the future once real hardware is available for full testsuite testing.
>
> 2019-07-17  Alan Hayward  <[hidden email]>
>
> * NEWS: Expand the Pointer Authentication entry.
> * aarch64-tdep.c (aarch64_frame_unmask_address): Mark frame if
> unmasking link register.
> * frame.c (struct frame_info): Add masked variable.

  * frame.c (struct frame_info): Add "masked" variable.

> (frame_set_previous_pc_masked) (frame_get_pc_masked): New function.

  (frame_set_previous_pc_masked, frame_get_pc_masked): New functions.

> * frame.h (frame_set_previous_pc_masked) (frame_get_pc_masked): New
> declaration.

  * frame.h (frame_set_previous_pc_masked, frame_get_pc_masked): New
  declarations.

> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -251,12 +251,13 @@ class instruction_reader : public abstract_instruction_reader
>  } // namespace
>  
>  /* If address signing is enabled, mask off the signature bits from ADDR, using
> -   the register values in THIS_FRAME.  */
> +   the register values in THIS_FRAME.  IS_LR indicates if ADDR is from the link
> +   register for the current frame.  */
>  
>  static CORE_ADDR
>  aarch64_frame_unmask_address (struct gdbarch_tdep *tdep,
> -      struct frame_info *this_frame,
> -      CORE_ADDR addr)
> +      struct frame_info *this_frame, CORE_ADDR addr,
> +      bool is_lr = true)

I didn't see anywhere in the patch that passes is_lr == true?

Looks like the patch is a nop?


>  {
>    if (tdep->has_pauth ()
>        && frame_unwind_register_unsigned (this_frame,
> @@ -265,6 +266,11 @@ aarch64_frame_unmask_address (struct gdbarch_tdep *tdep,
>        int cmask_num = AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
>        CORE_ADDR cmask = frame_unwind_register_unsigned (this_frame, cmask_num);
>        addr = addr & ~cmask;
> +
> +      /* If addr was taken from the Link Register, then record this in the
> + frame.  */
> +      if (is_lr)
> + frame_set_previous_pc_masked (this_frame);
>      }
>  
>    return addr;
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 84e0397db9..b7bb44986e 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -124,6 +124,8 @@ struct frame_info
>    struct {
>      enum cached_copy_status status;
>      CORE_ADDR value;
> +    /* Did VALUE require unmasking when being read.  */
> +    bool masked;
>    } prev_pc;
>    
>    /* Cached copy of the previous frame's function address.  */
> @@ -161,6 +163,21 @@ struct frame_info
>    const char *stop_string;
>  };
>  
> +/* See frame.h.  */
> +
> +void
> +frame_set_previous_pc_masked (struct frame_info *frame)
> +{
> +  frame->prev_pc.masked = true;
> +}
> +
> +/* See frame.h.  */
> +
> +bool frame_get_pc_masked (struct frame_info *frame)

Formatting -- line break after bool.

> +{
> +  return frame->next->prev_pc.masked;

I think this should assert that there's a frame->next,
and that its status is CC_VALUE.

> +}
> +

How about making the naming of the setter/getter consistent
with other setters/getters in the file, get_frame_type,
get_frame_pc, get_frame_program_space, etc, etc.

 set_frame_previous_pc_masked
 get_frame_pc_masked

?

>  /* A frame stash used to speed up frame lookups.  Create a hash table
>     to stash frames previously accessed from the frame cache for
>     quicker subsequent retrieval.  The hash table is emptied whenever
> diff --git a/gdb/frame.h b/gdb/frame.h
> index a79eeeeab1..61c12ec0b2 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -930,4 +930,13 @@ extern const gdb::option::option_def set_backtrace_option_defs[2];
>  /* The values behind the global "set backtrace ..." settings.  */
>  extern set_backtrace_options user_set_backtrace_options;
>  
> +/* Mark that the PC value is masked for the previous frame.  */
> +
> +extern void frame_set_previous_pc_masked (struct frame_info *frame);
> +

> +/* Get whether the PC value is masked for the given frame.  */
> +
> +extern bool frame_get_pc_masked (struct frame_info *frame);
> +
> +
>  #endif /* !defined (FRAME_H)  */
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 9b1d1a6856..cef1c29a82 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1254,7 +1254,11 @@ print_frame (const frame_print_options &fp_opts,
>   {
>    annotate_frame_address ();
>    if (pc_p)
> -    uiout->field_core_addr ("addr", gdbarch, pc);
> +    {
> +      uiout->field_core_addr ("addr", gdbarch, pc);
> +      if (frame_get_pc_masked (frame))
> + uiout->text ("<unmasked>");

Did you consider whether a MI frontend would want to show this
in its address column?

Also, sounds like you'd want to tweak fprint_frame too,
for debugging.

> +    }
>    else
>      uiout->field_string ("addr", "<unavailable>",
>   ui_out_style_kind::ADDRESS);
>

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Alan Hayward


> On 17 Jul 2019, at 12:15, Pedro Alves <[hidden email]> wrote:
>
> On 7/17/19 9:14 AM, Alan Hayward wrote:
>> Armv8.3-a Pointer Authentication causes the function return address to be
>> obfuscated on entry to some functions. GDB must unmask the link register in
>> order to produce a backtrace.
>>
>> The following patch adds markers of <unmasked> to the bracktrace, to indicate
>> which addresses needed unmasking.
>>
>> For example, consider the following backtrace:
>>
>> (gdb) bt
>> 0  0x0000000000400490 in puts@plt ()
>> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
>> 2  0x0000000000400604<unmasked> in bar () at cbreak-lib.c:12
>> 3  0x0000000000400620<unmasked> in barbar () at cbreak.c:17
>> 4  0x00000000004005b4 in main () at cbreak-3.c:10
>>
>> The functions in the cbreak-lib use pointer auth, obfuscating the return address
>> to the previous function.  The caused the addresses of bar and barbar to require
>> unmasking in order to unwind the backtrace.
>>
>> Alternatively, I considered replacing <unmasked> with a single chracter, such
>> as * for brevity reasons, but felt this would be non obvious for the user.
>
> I don't have a particular suggestion, though my first reaction was that
> it seemed a bit verbose.
>
> IMHO, the marker doesn't have to stand out and be expressive, since users can
> always look at the manual.

Reading the manual is an assumption I’m not sure is anywhere near the common case.
Saying that, I agree we shouldn’t be designing the output for the non-readers.

This comment has reminded me I need to add something to the manual as part of this
patch.


>  Once they learn something, often being concise
> helps -- or in other words, once you learn what "<unmasked>" or "U" or whatever
> is, and you're used to it, what would you rather see?  What's the main
> information you're looking for when staring at the backtrace?  Thoughts
> like that should guide the output too, IMO.

PAC is the official abbreviation for the feature, so maybe :PAC works best.

(gdb) bt
0  0x0000000000400490 in puts@plt ()
1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
3  0x0000000000400620:PAC in barbar () at cbreak.c:17
4  0x00000000004005b4 in main () at cbreak-3.c:10


Some of my attempts at different representations:
2  0x0000000000400604* in bar () at cbreak-lib.c:12
2  0x0000000000400604! in bar () at cbreak-lib.c:12
2  0x0000000000400604U in bar () at cbreak-lib.c:122
2  0x0000000000400604:U in bar () at cbreak-lib.c:122
2  0x0000000000400604<U> in bar () at cbreak-lib.c:12
2  0x0000000000400604[U] in bar () at cbreak-lib.c:12
2  0x0000000000400604<M> in bar () at cbreak-lib.c:12
2  0x0000000000400604<P> in bar () at cbreak-lib.c:12
2  0x0000000000400604<PAC> in bar () at cbreak-lib.c:12
2  0x0000000000400604PAC in bar () at cbreak-lib.c:12
2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
2  0x0000000000400604,PAC in bar () at cbreak-lib.c:12

I found a single character was too hidden. A single character or symbol was also
a little confusing - my brain read U as unsigned, * as pointer, [] as an array.

I also like ,PAC as it might be easier to add future extensions.


>
>>
>> An extra bool is added alongside the prev_pc in the frame struture.  At the
>> point at which the link register is unmasked, the AArch64 port calls into frame
>> to sets the bool.  This seemed the most efficient way of doing it.
>>
>> aarch64_frame_unmask_address is designed to work for any address, however at
>> the momoment it is only used for the link address.  An extra default parameter
>> is added to ensure it only caches for the link register.
>
> typo: “momoment"

>
>>
>> I expect this will potentially cause issues with some tests in the testsuite
>> when Armv8.3 pointer authentication is used.  This should be fixed up in the
>> the future once real hardware is available for full testsuite testing.
>>
>> 2019-07-17  Alan Hayward  <[hidden email]>
>>
>> * NEWS: Expand the Pointer Authentication entry.
>> * aarch64-tdep.c (aarch64_frame_unmask_address): Mark frame if
>> unmasking link register.
>> * frame.c (struct frame_info): Add masked variable.
>
> * frame.c (struct frame_info): Add "masked" variable.
>
>> (frame_set_previous_pc_masked) (frame_get_pc_masked): New function.
>
> (frame_set_previous_pc_masked, frame_get_pc_masked): New functions.
>
>> * frame.h (frame_set_previous_pc_masked) (frame_get_pc_masked): New
>> declaration.
>
> * frame.h (frame_set_previous_pc_masked, frame_get_pc_masked): New
> declarations.

Ok to all these.

>
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -251,12 +251,13 @@ class instruction_reader : public abstract_instruction_reader
>> } // namespace
>>
>> /* If address signing is enabled, mask off the signature bits from ADDR, using
>> -   the register values in THIS_FRAME.  */
>> +   the register values in THIS_FRAME.  IS_LR indicates if ADDR is from the link
>> +   register for the current frame.  */
>>
>> static CORE_ADDR
>> aarch64_frame_unmask_address (struct gdbarch_tdep *tdep,
>> -      struct frame_info *this_frame,
>> -      CORE_ADDR addr)
>> +      struct frame_info *this_frame, CORE_ADDR addr,
>> +      bool is_lr = true)
>
> I didn't see anywhere in the patch that passes is_lr == true?
>
> Looks like the patch is a nop?

Yes, the is_lr check will always be true.
The function is designed to be for any address, but right now it’s only used for
the link register.  But, I didn’t want someone in the future using it as a generic
address unmask function and it causing the link register to be marked.

An other option was to add a warning to the comment block above the function. Or
do the lr marking in the caller (but then that adds extra logic).

Maybe the better option would be to rename the function to
aarch64_frame_unmask_lr_address.


>
>
>> {
>>   if (tdep->has_pauth ()
>>       && frame_unwind_register_unsigned (this_frame,
>> @@ -265,6 +266,11 @@ aarch64_frame_unmask_address (struct gdbarch_tdep *tdep,
>>       int cmask_num = AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base);
>>       CORE_ADDR cmask = frame_unwind_register_unsigned (this_frame, cmask_num);
>>       addr = addr & ~cmask;
>> +
>> +      /* If addr was taken from the Link Register, then record this in the
>> + frame.  */
>> +      if (is_lr)
>> + frame_set_previous_pc_masked (this_frame);
>>     }
>>
>>   return addr;
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index 84e0397db9..b7bb44986e 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -124,6 +124,8 @@ struct frame_info
>>   struct {
>>     enum cached_copy_status status;
>>     CORE_ADDR value;
>> +    /* Did VALUE require unmasking when being read.  */
>> +    bool masked;
>>   } prev_pc;
>>
>>   /* Cached copy of the previous frame's function address.  */
>> @@ -161,6 +163,21 @@ struct frame_info
>>   const char *stop_string;
>> };
>>
>> +/* See frame.h.  */
>> +
>> +void
>> +frame_set_previous_pc_masked (struct frame_info *frame)
>> +{
>> +  frame->prev_pc.masked = true;
>> +}
>> +
>> +/* See frame.h.  */
>> +
>> +bool frame_get_pc_masked (struct frame_info *frame)
>
> Formatting -- line break after bool.

>
>> +{
>> +  return frame->next->prev_pc.masked;
>
> I think this should assert that there's a frame->next,
> and that its status is CC_VALUE.

>
>> +}
>> +
>
> How about making the naming of the setter/getter consistent
> with other setters/getters in the file, get_frame_type,
> get_frame_pc, get_frame_program_space, etc, etc.
>
> set_frame_previous_pc_masked
> get_frame_pc_masked
>
> ?


Ok with all these.

>
>> /* A frame stash used to speed up frame lookups.  Create a hash table
>>    to stash frames previously accessed from the frame cache for
>>    quicker subsequent retrieval.  The hash table is emptied whenever
>> diff --git a/gdb/frame.h b/gdb/frame.h
>> index a79eeeeab1..61c12ec0b2 100644
>> --- a/gdb/frame.h
>> +++ b/gdb/frame.h
>> @@ -930,4 +930,13 @@ extern const gdb::option::option_def set_backtrace_option_defs[2];
>> /* The values behind the global "set backtrace ..." settings.  */
>> extern set_backtrace_options user_set_backtrace_options;
>>
>> +/* Mark that the PC value is masked for the previous frame.  */
>> +
>> +extern void frame_set_previous_pc_masked (struct frame_info *frame);
>> +
>
>> +/* Get whether the PC value is masked for the given frame.  */
>> +
>> +extern bool frame_get_pc_masked (struct frame_info *frame);
>> +
>> +
>> #endif /* !defined (FRAME_H)  */
>> diff --git a/gdb/stack.c b/gdb/stack.c
>> index 9b1d1a6856..cef1c29a82 100644
>> --- a/gdb/stack.c
>> +++ b/gdb/stack.c
>> @@ -1254,7 +1254,11 @@ print_frame (const frame_print_options &fp_opts,
>> {
>>  annotate_frame_address ();
>>  if (pc_p)
>> -    uiout->field_core_addr ("addr", gdbarch, pc);
>> +    {
>> +      uiout->field_core_addr ("addr", gdbarch, pc);
>> +      if (frame_get_pc_masked (frame))
>> + uiout->text ("<unmasked>");
>
> Did you consider whether a MI frontend would want to show this
> in its address column?
>
> Also, sounds like you'd want to tweak fprint_frame too,
> for debugging.

I’ll add both of those into the patch too.


Thanks for the review!

Alan.


>
>> +    }
>>  else
>>    uiout->field_string ("addr", "<unavailable>",
>> ui_out_style_kind::ADDRESS);
>>
>
> Thanks,
> Pedro Alves

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Pedro Alves-7
On 7/17/19 2:35 PM, Alan Hayward wrote:

>
>>  Once they learn something, often being concise
>> helps -- or in other words, once you learn what "<unmasked>" or "U" or whatever
>> is, and you're used to it, what would you rather see?  What's the main
>> information you're looking for when staring at the backtrace?  Thoughts
>> like that should guide the output too, IMO.
>
> PAC is the official abbreviation for the feature, so maybe :PAC works best.

Reading https://lwn.net/Articles/767695/ , I see mention of

 Insert a PAC into a pointer
 Strip a PAC from a pointer

Would ":PAC" make it read like the address _includes_ PAC bits?

>
> (gdb) bt
> 0  0x0000000000400490 in puts@plt ()
> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
> 2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
> 3  0x0000000000400620:PAC in barbar () at cbreak.c:17
> 4  0x00000000004005b4 in main () at cbreak-3.c:10
>
>
> Some of my attempts at different representations:
> 2  0x0000000000400604* in bar () at cbreak-lib.c:12
> 2  0x0000000000400604! in bar () at cbreak-lib.c:12
> 2  0x0000000000400604U in bar () at cbreak-lib.c:122
> 2  0x0000000000400604:U in bar () at cbreak-lib.c:122
> 2  0x0000000000400604<U> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604[U] in bar () at cbreak-lib.c:12
> 2  0x0000000000400604<M> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604<P> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604<PAC> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604PAC in bar () at cbreak-lib.c:12
> 2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
> 2  0x0000000000400604,PAC in bar () at cbreak-lib.c:12
>
> I found a single character was too hidden. A single character or symbol was also
> a little confusing - my brain read U as unsigned, * as pointer, [] as an array.
>
> I also like ,PAC as it might be easier to add future extensions.

I'd go with either:

 2  0x0000000000400604 (PAC) in bar () at cbreak-lib.c:12
 2  0x0000000000400604 [PAC] in bar () at cbreak-lib.c:12

Not having the space may make it a little bit harder
to focus on low digits of the address.

> my brain read U as unsigned, * as pointer, [] as an array.

If you make it like 0x0000000000400604U, then I can see that.

But not so much with:

 2  0x0000000000400604 [U] in bar () at cbreak-lib.c:12

You don't have to use a single letter, though:

 2  0x0000000000400604 [UN] in bar () at cbreak-lib.c:12

[] seems natural as a way to group some flags/properties to me.

We already use it here for example:

 (top-gdb) info registers $eflags
 eflags         0x206               [ PF IF ]


I guess I'm saying that it depends on context, and I wouldn't
be worried with [] being confused with C arrays.  Afterall,
< and > also have meaning in C/C++... More than one meaning,
actually.  :-)

>>> /* If address signing is enabled, mask off the signature bits from ADDR, using
>>> -   the register values in THIS_FRAME.  */
>>> +   the register values in THIS_FRAME.  IS_LR indicates if ADDR is from the link
>>> +   register for the current frame.  */
>>>
>>> static CORE_ADDR
>>> aarch64_frame_unmask_address (struct gdbarch_tdep *tdep,
>>> -      struct frame_info *this_frame,
>>> -      CORE_ADDR addr)
>>> +      struct frame_info *this_frame, CORE_ADDR addr,
>>> +      bool is_lr = true)
>>
>> I didn't see anywhere in the patch that passes is_lr == true?
>>
>> Looks like the patch is a nop?
>
> Yes, the is_lr check will always be true.
> The function is designed to be for any address, but right now it’s only used for
> the link register.  But, I didn’t want someone in the future using it as a generic
> address unmask function and it causing the link register to be marked.
>
> An other option was to add a warning to the comment block above the function. Or
> do the lr marking in the caller (but then that adds extra logic).
>
> Maybe the better option would be to rename the function to
> aarch64_frame_unmask_lr_address.

Yes, I think that renaming is the best option.  People remove
unnecessary parameters all the time, when they notice some
parameter isn't really necessary.  If someone needs a more
generic  aarch64_frame_unmask_address for all kinds of
addresses, they can always factor out a new aarch64_frame_unmask_address
that looks like today's version, and
make aarch64_frame_unmask_lr_address call it + mark the frame.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Simon Marchi
In reply to this post by Alan Hayward
On 2019-07-17 09:35, Alan Hayward wrote:

>> On 17 Jul 2019, at 12:15, Pedro Alves <[hidden email]> wrote:
>>
>> On 7/17/19 9:14 AM, Alan Hayward wrote:
>>> Armv8.3-a Pointer Authentication causes the function return address
>>> to be
>>> obfuscated on entry to some functions. GDB must unmask the link
>>> register in
>>> order to produce a backtrace.
>>>
>>> The following patch adds markers of <unmasked> to the bracktrace, to
>>> indicate
>>> which addresses needed unmasking.
>>>
>>> For example, consider the following backtrace:
>>>
>>> (gdb) bt
>>> 0  0x0000000000400490 in puts@plt ()
>>> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
>>> 2  0x0000000000400604<unmasked> in bar () at cbreak-lib.c:12
>>> 3  0x0000000000400620<unmasked> in barbar () at cbreak.c:17
>>> 4  0x00000000004005b4 in main () at cbreak-3.c:10
>>>
>>> The functions in the cbreak-lib use pointer auth, obfuscating the
>>> return address
>>> to the previous function.  The caused the addresses of bar and barbar
>>> to require
>>> unmasking in order to unwind the backtrace.
>>>
>>> Alternatively, I considered replacing <unmasked> with a single
>>> chracter, such
>>> as * for brevity reasons, but felt this would be non obvious for the
>>> user.
>>
>> I don't have a particular suggestion, though my first reaction was
>> that
>> it seemed a bit verbose.
>>
>> IMHO, the marker doesn't have to stand out and be expressive, since
>> users can
>> always look at the manual.
>
> Reading the manual is an assumption I’m not sure is anywhere near the
> common case.
> Saying that, I agree we shouldn’t be designing the output for the
> non-readers.
>
> This comment has reminded me I need to add something to the manual as
> part of this
> patch.
>
>
>>  Once they learn something, often being concise
>> helps -- or in other words, once you learn what "<unmasked>" or "U" or
>> whatever
>> is, and you're used to it, what would you rather see?  What's the main
>> information you're looking for when staring at the backtrace?  
>> Thoughts
>> like that should guide the output too, IMO.
>
> PAC is the official abbreviation for the feature, so maybe :PAC works
> best.
>
> (gdb) bt
> 0  0x0000000000400490 in puts@plt ()
> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
> 2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
> 3  0x0000000000400620:PAC in barbar () at cbreak.c:17
> 4  0x00000000004005b4 in main () at cbreak-3.c:10
>
>
> Some of my attempts at different representations:
> 2  0x0000000000400604* in bar () at cbreak-lib.c:12
> 2  0x0000000000400604! in bar () at cbreak-lib.c:12
> 2  0x0000000000400604U in bar () at cbreak-lib.c:122
> 2  0x0000000000400604:U in bar () at cbreak-lib.c:122
> 2  0x0000000000400604<U> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604[U] in bar () at cbreak-lib.c:12
> 2  0x0000000000400604<M> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604<P> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604<PAC> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604PAC in bar () at cbreak-lib.c:12
> 2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
> 2  0x0000000000400604,PAC in bar () at cbreak-lib.c:12
>
> I found a single character was too hidden. A single character or symbol
> was also
> a little confusing - my brain read U as unsigned, * as pointer, [] as
> an array.
>
> I also like ,PAC as it might be easier to add future extensions.

It might not be easily doable, but I think it would be nice if you could
somehow make it so the function names stay aligned (regardless of which
marker you end up choosing), like:

0  0x0000000000400490     in puts@plt ()
1  0x00000000004005dc     in foo ("hello") at cbreak-lib.c:6
2  0x0000000000400604 [U] in bar () at cbreak-lib.c:12
3  0x0000000000400620 [U] in barbar () at cbreak.c:17
4  0x00000000004005b4     in main () at cbreak-3.c:10

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Pedro Alves-7
On 7/17/19 4:01 PM, Simon Marchi wrote:

> On 2019-07-17 09:35, Alan Hayward wrote:
>>> On 17 Jul 2019, at 12:15, Pedro Alves <[hidden email]> wrote:
>>>
>>> On 7/17/19 9:14 AM, Alan Hayward wrote:
>>>> Armv8.3-a Pointer Authentication causes the function return address to be
>>>> obfuscated on entry to some functions. GDB must unmask the link register in
>>>> order to produce a backtrace.
>>>>
>>>> The following patch adds markers of <unmasked> to the bracktrace, to indicate
>>>> which addresses needed unmasking.
>>>>
>>>> For example, consider the following backtrace:
>>>>
>>>> (gdb) bt
>>>> 0  0x0000000000400490 in puts@plt ()
>>>> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
>>>> 2  0x0000000000400604<unmasked> in bar () at cbreak-lib.c:12
>>>> 3  0x0000000000400620<unmasked> in barbar () at cbreak.c:17
>>>> 4  0x00000000004005b4 in main () at cbreak-3.c:10
>>>>
>>>> The functions in the cbreak-lib use pointer auth, obfuscating the return address
>>>> to the previous function.  The caused the addresses of bar and barbar to require
>>>> unmasking in order to unwind the backtrace.
>>>>
>>>> Alternatively, I considered replacing <unmasked> with a single chracter, such
>>>> as * for brevity reasons, but felt this would be non obvious for the user.
>>>
>>> I don't have a particular suggestion, though my first reaction was that
>>> it seemed a bit verbose.
>>>
>>> IMHO, the marker doesn't have to stand out and be expressive, since users can
>>> always look at the manual.
>>
>> Reading the manual is an assumption I’m not sure is anywhere near the
>> common case.
>> Saying that, I agree we shouldn’t be designing the output for the non-readers.
>>
>> This comment has reminded me I need to add something to the manual as
>> part of this
>> patch.
>>
>>
>>>  Once they learn something, often being concise
>>> helps -- or in other words, once you learn what "<unmasked>" or "U" or whatever
>>> is, and you're used to it, what would you rather see?  What's the main
>>> information you're looking for when staring at the backtrace?  Thoughts
>>> like that should guide the output too, IMO.
>>
>> PAC is the official abbreviation for the feature, so maybe :PAC works best.
>>
>> (gdb) bt
>> 0  0x0000000000400490 in puts@plt ()
>> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
>> 2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
>> 3  0x0000000000400620:PAC in barbar () at cbreak.c:17
>> 4  0x00000000004005b4 in main () at cbreak-3.c:10
>>
>>
>> Some of my attempts at different representations:
>> 2  0x0000000000400604* in bar () at cbreak-lib.c:12
>> 2  0x0000000000400604! in bar () at cbreak-lib.c:12
>> 2  0x0000000000400604U in bar () at cbreak-lib.c:122
>> 2  0x0000000000400604:U in bar () at cbreak-lib.c:122
>> 2  0x0000000000400604<U> in bar () at cbreak-lib.c:12
>> 2  0x0000000000400604[U] in bar () at cbreak-lib.c:12
>> 2  0x0000000000400604<M> in bar () at cbreak-lib.c:12
>> 2  0x0000000000400604<P> in bar () at cbreak-lib.c:12
>> 2  0x0000000000400604<PAC> in bar () at cbreak-lib.c:12
>> 2  0x0000000000400604PAC in bar () at cbreak-lib.c:12
>> 2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
>> 2  0x0000000000400604,PAC in bar () at cbreak-lib.c:12
>>
>> I found a single character was too hidden. A single character or symbol was also
>> a little confusing - my brain read U as unsigned, * as pointer, [] as an array.
>>
>> I also like ,PAC as it might be easier to add future extensions.
>
> It might not be easily doable, but I think it would be nice if you could somehow make it so the function names stay aligned (regardless of which marker you end up choosing), like:
>
> 0  0x0000000000400490     in puts@plt ()
> 1  0x00000000004005dc     in foo ("hello") at cbreak-lib.c:6
> 2  0x0000000000400604 [U] in bar () at cbreak-lib.c:12
> 3  0x0000000000400620 [U] in barbar () at cbreak.c:17
> 4  0x00000000004005b4     in main () at cbreak-3.c:10

I almost suggested the same, but didn't when I realized that we
don't always print the addresses:

 (top-gdb) bt
 #0  gdb_main (args=0x7fffffffd3a0) at src/gdb/main.c:1186
 #1  0x0000000000469a7e in main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:32

But if you do want to align the addresses, you could do that by
specifying a width for the "addr" column.  If "[U]" is rare, given no column
headers, the spaces may look a bit odd, though.  Maybe you'd want to pre-compute
the max column width by looking at the max number of frames that fit on a
page, or something along those lines.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Alan Hayward


> On 17 Jul 2019, at 16:18, Pedro Alves <[hidden email]> wrote:
>
> On 7/17/19 4:01 PM, Simon Marchi wrote:
>> On 2019-07-17 09:35, Alan Hayward wrote:
>>>> On 17 Jul 2019, at 12:15, Pedro Alves <[hidden email]> wrote:
>>>>
>>>> On 7/17/19 9:14 AM, Alan Hayward wrote:
>>>>> Armv8.3-a Pointer Authentication causes the function return address to be
>>>>> obfuscated on entry to some functions. GDB must unmask the link register in
>>>>> order to produce a backtrace.
>>>>>
>>>>> The following patch adds markers of <unmasked> to the bracktrace, to indicate
>>>>> which addresses needed unmasking.
>>>>>
>>>>> For example, consider the following backtrace:
>>>>>
>>>>> (gdb) bt
>>>>> 0  0x0000000000400490 in puts@plt ()
>>>>> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
>>>>> 2  0x0000000000400604<unmasked> in bar () at cbreak-lib.c:12
>>>>> 3  0x0000000000400620<unmasked> in barbar () at cbreak.c:17
>>>>> 4  0x00000000004005b4 in main () at cbreak-3.c:10
>>>>>
>>>>> The functions in the cbreak-lib use pointer auth, obfuscating the return address
>>>>> to the previous function.  The caused the addresses of bar and barbar to require
>>>>> unmasking in order to unwind the backtrace.
>>>>>
>>>>> Alternatively, I considered replacing <unmasked> with a single chracter, such
>>>>> as * for brevity reasons, but felt this would be non obvious for the user.
>>>>
>>>> I don't have a particular suggestion, though my first reaction was that
>>>> it seemed a bit verbose.
>>>>
>>>> IMHO, the marker doesn't have to stand out and be expressive, since users can
>>>> always look at the manual.
>>>
>>> Reading the manual is an assumption I’m not sure is anywhere near the
>>> common case.
>>> Saying that, I agree we shouldn’t be designing the output for the non-readers.
>>>
>>> This comment has reminded me I need to add something to the manual as
>>> part of this
>>> patch.
>>>
>>>
>>>>  Once they learn something, often being concise
>>>> helps -- or in other words, once you learn what "<unmasked>" or "U" or whatever
>>>> is, and you're used to it, what would you rather see?  What's the main
>>>> information you're looking for when staring at the backtrace?  Thoughts
>>>> like that should guide the output too, IMO.
>>>
>>> PAC is the official abbreviation for the feature, so maybe :PAC works best.
>>>
>>> (gdb) bt
>>> 0  0x0000000000400490 in puts@plt ()
>>> 1  0x00000000004005dc in foo ("hello") at cbreak-lib.c:6
>>> 2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
>>> 3  0x0000000000400620:PAC in barbar () at cbreak.c:17
>>> 4  0x00000000004005b4 in main () at cbreak-3.c:10
>>>
>>>
>>> Some of my attempts at different representations:
>>> 2  0x0000000000400604* in bar () at cbreak-lib.c:12
>>> 2  0x0000000000400604! in bar () at cbreak-lib.c:12
>>> 2  0x0000000000400604U in bar () at cbreak-lib.c:122
>>> 2  0x0000000000400604:U in bar () at cbreak-lib.c:122
>>> 2  0x0000000000400604<U> in bar () at cbreak-lib.c:12
>>> 2  0x0000000000400604[U] in bar () at cbreak-lib.c:12
>>> 2  0x0000000000400604<M> in bar () at cbreak-lib.c:12
>>> 2  0x0000000000400604<P> in bar () at cbreak-lib.c:12
>>> 2  0x0000000000400604<PAC> in bar () at cbreak-lib.c:12
>>> 2  0x0000000000400604PAC in bar () at cbreak-lib.c:12
>>> 2  0x0000000000400604:PAC in bar () at cbreak-lib.c:12
>>> 2  0x0000000000400604,PAC in bar () at cbreak-lib.c:12
>>>
>>> I found a single character was too hidden. A single character or symbol was also
>>> a little confusing - my brain read U as unsigned, * as pointer, [] as an array.
>>>
>>> I also like ,PAC as it might be easier to add future extensions.
>>
>> It might not be easily doable, but I think it would be nice if you could somehow make it so the function names stay aligned (regardless of which marker you end up choosing), like:
>>
>> 0  0x0000000000400490     in puts@plt ()
>> 1  0x00000000004005dc     in foo ("hello") at cbreak-lib.c:6
>> 2  0x0000000000400604 [U] in bar () at cbreak-lib.c:12
>> 3  0x0000000000400620 [U] in barbar () at cbreak.c:17
>> 4  0x00000000004005b4     in main () at cbreak-3.c:10
>
> I almost suggested the same, but didn't when I realized that we
> don't always print the addresses:
>
> (top-gdb) bt
> #0  gdb_main (args=0x7fffffffd3a0) at src/gdb/main.c:1186
> #1  0x0000000000469a7e in main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:32
>

What’s the reason for that? Surely we always know the address of a function
in the backtrace? Can it happen in the middle of a backtrace?


> But if you do want to align the addresses, you could do that by
> specifying a width for the "addr" column.

>  If "[U]" is rare, given no column
> headers, the spaces may look a bit odd, though.

In general, it depends how a binary/library was compiled. But I’d expect a binary
to either have it in most functions or none.

Should be easy enough to remove the extra spaces if the system doesn’t support PAC.


>  Maybe you'd want to pre-compute
> the max column width by looking at the max number of frames that fit on a
> page, or something along those lines.
>

hmmm... ok. I’ll see what I can do there.



> On 17 Jul 2019, at 15:43, Pedro Alves <[hidden email]> wrote:
<SNIP>

>
> I'd go with either:
>
> 2  0x0000000000400604 (PAC) in bar () at cbreak-lib.c:12
> 2  0x0000000000400604 [PAC] in bar () at cbreak-lib.c:12
>
> Not having the space may make it a little bit harder
> to focus on low digits of the address.
>
>> my brain read U as unsigned, * as pointer, [] as an array.
>
> If you make it like 0x0000000000400604U, then I can see that.
>
> But not so much with:
>
> 2  0x0000000000400604 [U] in bar () at cbreak-lib.c:12
>
> You don't have to use a single letter, though:
>
> 2  0x0000000000400604 [UN] in bar () at cbreak-lib.c:12
>
> [] seems natural as a way to group some flags/properties to me.
>
> We already use it here for example:
>
> (top-gdb) info registers $eflags
> eflags         0x206               [ PF IF ]
>
>
> I guess I'm saying that it depends on context, and I wouldn't
> be worried with [] being confused with C arrays.  Afterall,
> < and > also have meaning in C/C++... More than one meaning,
> actually.  :-)
>

The extra space really does help there.

Given PAC really is an AArch64 thing (as opposed to something more
generic like Unmasked) might be worth adding a
gdbarch_print_function_address () or something like that so that I
can override it in aarch64.  Assuming it fits with all the width
calculations.



Alan.




Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Pedro Alves-7
On 7/17/19 5:07 PM, Alan Hayward wrote:

>> I almost suggested the same, but didn't when I realized that we
>> don't always print the addresses:
>>
>> (top-gdb) bt
>> #0  gdb_main (args=0x7fffffffd3a0) at src/gdb/main.c:1186
>> #1  0x0000000000469a7e in main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:32
>>
>
> What’s the reason for that? Surely we always know the address of a function
> in the backtrace? Can it happen in the middle of a backtrace?

"It always worked that way", at least for me.

We show an address if the PC is pointing to the middle
of a line, or we don't have debug info.  If pointing at a line
exactly, then we show no address.

 (gdb) frame
 #0  main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:29
 29        args.argc = argc;
 (gdb) si
 0x0000000000469a5f      29        args.argc = argc;
 (gdb) frame
 #0  0x0000000000469a5f in main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:29
 29        args.argc = argc;


Same logic for when displaying the frame where a program stops, when
stepping, ctrl-c, breakpoint hits, etc.

 Breakpoint 5, main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:28
               ^^^^
 28        memset (&args, 0, sizeof args);
 (gdb) p /x $pc
 $1 = 0x469a46
 (top-gdb) del
 Delete all breakpoints? (y or n) y
 (top-gdb) r
 The program being debugged has been started already.
 Start it from the beginning? (y or n) y
 Starting program: build/gdb/gdb
 Breakpoint 6, 0x0000000000469a4a in main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:28
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
 28        memset (&args, 0, sizeof args);

>
>
>> But if you do want to align the addresses, you could do that by
>> specifying a width for the "addr" column.
>
>>  If "[U]" is rare, given no column
>> headers, the spaces may look a bit odd, though.
>
> In general, it depends how a binary/library was compiled. But I’d expect a binary
> to either have it in most functions or none.
>
> Should be easy enough to remove the extra spaces if the system doesn’t support PAC.
>
>
>>  Maybe you'd want to pre-compute
>> the max column width by looking at the max number of frames that fit on a
>> page, or something along those lines.
>>
>
> hmmm... ok. I’ll see what I can do there.

If most functions have it, then I wouldn't bother trying to compute
the max column width.

But then if most functions have it, I wonder what's the point of
showing the marker, though.  :-)  Would it make sense to reverse
the logic?

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Alan Hayward


> On 17 Jul 2019, at 17:41, Pedro Alves <[hidden email]> wrote:
>
> On 7/17/19 5:07 PM, Alan Hayward wrote:
>
>>> I almost suggested the same, but didn't when I realized that we
>>> don't always print the addresses:
>>>
>>> (top-gdb) bt
>>> #0  gdb_main (args=0x7fffffffd3a0) at src/gdb/main.c:1186
>>> #1  0x0000000000469a7e in main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:32
>>>
>>
>> What’s the reason for that? Surely we always know the address of a function
>> in the backtrace? Can it happen in the middle of a backtrace?
>
> "It always worked that way", at least for me.
>
> We show an address if the PC is pointing to the middle
> of a line, or we don't have debug info.  If pointing at a line
> exactly, then we show no address.
>
> (gdb) frame
> #0  main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:29
> 29        args.argc = argc;
> (gdb) si
> 0x0000000000469a5f      29        args.argc = argc;
> (gdb) frame
> #0  0x0000000000469a5f in main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:29
> 29        args.argc = argc;
>
>
> Same logic for when displaying the frame where a program stops, when
> stepping, ctrl-c, breakpoint hits, etc.
>
> Breakpoint 5, main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:28
>               ^^^^
> 28        memset (&args, 0, sizeof args);
> (gdb) p /x $pc
> $1 = 0x469a46
> (top-gdb) del
> Delete all breakpoints? (y or n) y
> (top-gdb) r
> The program being debugged has been started already.
> Start it from the beginning? (y or n) y
> Starting program: build/gdb/gdb
> Breakpoint 6, 0x0000000000469a4a in main (argc=1, argv=0x7fffffffd4a8) at src/gdb/gdb.c:28
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 28        memset (&args, 0, sizeof args);


Excellent, thanks.

>
>>
>>
>>> But if you do want to align the addresses, you could do that by
>>> specifying a width for the "addr" column.
>>
>>> If "[U]" is rare, given no column
>>> headers, the spaces may look a bit odd, though.
>>
>> In general, it depends how a binary/library was compiled. But I’d expect a binary
>> to either have it in most functions or none.
>>
>> Should be easy enough to remove the extra spaces if the system doesn’t support PAC.
>>
>>
>>> Maybe you'd want to pre-compute
>>> the max column width by looking at the max number of frames that fit on a
>>> page, or something along those lines.
>>>
>>
>> hmmm... ok. I’ll see what I can do there.
>
> If most functions have it, then I wouldn't bother trying to compute
> the max column width.
>
> But then if most functions have it, I wonder what's the point of
> showing the marker, though.  :-)  Would it make sense to reverse
> the logic?
>

I think it’s still better this way around. It indicates that PAC is being used.
You might, for example, be running 8.0 binaries on 8.3 system, which has then
linked against your libc etc which is using 8.3 PAC.
(You can also run 8.3 PAC on 8.0, as the relevant stuff will just turn into nops).


> Thanks,
> Pedro Alves

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

Tom Tromey-2
In reply to this post by Alan Hayward
>>>>> "Alan" == Alan Hayward <[hidden email]> writes:

Alan> diff --git a/gdb/stack.c b/gdb/stack.c
Alan> index 9b1d1a6856..cef1c29a82 100644
Alan> --- a/gdb/stack.c
Alan> +++ b/gdb/stack.c
Alan> @@ -1254,7 +1254,11 @@ print_frame (const frame_print_options &fp_opts,
Alan>   {
Alan>    annotate_frame_address ();
Alan>    if (pc_p)
Alan> -    uiout->field_core_addr ("addr", gdbarch, pc);
Alan> +    {
Alan> +      uiout->field_core_addr ("addr", gdbarch, pc);
Alan> +      if (frame_get_pc_masked (frame))
Alan> + uiout->text ("<unmasked>");
Alan> +    }

Usually a change to frame printing will require a change in the python
layer as well, because it has its own frame printer.  The ideal is that
a no-op frame filter should result in no change to the output.

thanks,
Tom