Re: [PATCH][ARM][GDB]: gdb cannot step across CMSE secure entry function code.

[Alan Hayward <Alan dot Hayward at arm dot com>]

> On 17 Jul 2019, at 16:20, Srinath Parvathaneni <Srinath.Parvathaneni@arm.com> wrote:
> 
> Hi All,

Hi, thanks for the patch.
There are a few nits to clean up (see comments below).
The only big issue is that when I ran your test without your fixes it passed for me (see
final comment).

> 
> GDB is not able to execute "step" command on function calls of Armv8-M cmse secure entry functions.
> Everytime GNU linker come across definition of any cmse secure entry function in object file(s),
> it creates two new instructions secure gateway (sg) and original branch destination (b.w),
> place those two instructions in ".gnu.sgstubs" section of executable.
> Any function calls to these cmse secure entry functions is re-directed through secure gateway (sg)
> present in ".gnu.sgstubs" section.
> 
> Example:
> Following is a function call to cmse secure entry function "foo":
>        ...
>        bl xxxx <foo>   --->(a)
>        ...
>        <foo>
>        xxxx: push    {r7, lr}
> 
> GNU linker on finding out "foo" is a cmse secure entry function, created sg and b.w instructions and
> place them in ".gnu.sgstubs" section (marked by c).
> 
> The "bl" instruction (marked by a) which is a call to cmse secure entry function is modified by GNU linker
> (as marked by b) and call flow is re-directly through secure gateway (sg) in ".gnu.sgstubs" section.
>       ...
>       bl yyyy <foo>  ---> (b)
>       ...
>       section .gnu.sgstubs: ---> (c)
>       yyyy <foo>
>       yyyy: sg   // secure gateway
>     b.w xxxx <__acle_se_foo>  // original_branch_dest
>       ...
>       0000xxxx <__acle_se_foo>
>       xxxx: push    {r7, lr} ---> (d)
> 
> On invoking GDB, when the control is at "b" and we pass "step" command, the pc returns "yyyy"
> (sg address) which is a trampoline and which does not exist in source code. So GDB jumps
> to next line without jumping to "__acle_se_foo" (marked by d).
> 

Could you please include a reference to the document on developer.arm.com where this is
specified, plus the relevant section number in the document.


> This patch fixes above problem by returning target pc "xxxx" to GDB on executing "step"
> command at "b", so that the control jumps to "__acle_se_foo" (marked by d).
> 
> This patch is tested by debugging the CMSE executable using GDB on Aarch32 box.
> Regression tested for armv7hl-redhat-linux-gnueabi and found no regressions.
> 
> Ok for master?
> 
> Hi All,
> 
> GDB is not able to execute "step" command on function calls of Armv8-M cmse secure entry functions.
> Everytime GNU linker come across definition of any cmse secure entry function in object file(s),
> it creates two new instructions secure gateway (sg) and original branch destination (b.w),
> place those two instructions in ".gnu.sgstubs" section of executable.
> Any function calls to these cmse secure entry functions is re-directed through secure gateway (sg)
> present in ".gnu.sgstubs" section.
> 
> Example:
> Following is a function call to cmse secure entry function "foo":
>        ...
>        bl xxxx <foo>   --->(a)
>        ...
>        <foo>
>        xxxx: push    {r7, lr}
> 
> GNU linker on finding out "foo" is a cmse secure entry function, created sg and b.w instructions and
> place them in ".gnu.sgstubs" section (marked by c).
> 
> The "bl" instruction (marked by a) which is a call to cmse secure entry function is modified by GNU linker
> (as marked by b) and call flow is re-directly through secure gateway (sg) in ".gnu.sgstubs" section.
>       ...
>       bl yyyy <foo>  ---> (b)
>       ...
>       section .gnu.sgstubs: ---> (c)
>       yyyy <foo>
>       yyyy: sg   // secure gateway
>     b.w xxxx <__acle_se_foo>  // original_branch_dest
>       ...
>       0000xxxx <__acle_se_foo>
>       xxxx: push    {r7, lr} ---> (d)
> 
> On invoking GDB, when the control is at "b" and we pass "step" command, the pc returns "yyyy"
> (sg address) which is a trampoline and which does not exist in source code. So GDB jumps
> to next line without jumping to "__acle_se_foo" (marked by d).
> 
> This patch fixes above problem by returning target pc "xxxx" to GDB on executing "step"
> command at "b", so that the control jumps to "__acle_se_foo" (marked by d).
> 
> This patch is tested by debugging the CMSE executable using GDB on Aarch32 box.
> Regression tested for armv7hl-redhat-linux-gnueabi and found no regressions.
> 
> Ok for master?
> 
> gdb/ChangeLog:
> 
> 2019-07-17  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
> 
> * arm-tdep.h (check_section_name): New function declaration.
> * arm-tdep.c (arm_skip_sg_jump_to_bw): New function. When gdb
> encounters a "step" command on cmse secure entry function (eg:func),
> this function return an address of "__acle_se_<func>" to PC instead
> of secure gateaway (sg) address which is present in ".gnu.sgstubs"
> section.
> (check_section_name): New function. To check the current section is
> ".gnu.sgstubs".
> (arm_skip_stub): Modify to call arm_skip_sg_jump_to_bw function.
> 
> gdb/testsuite/ChangeLog:
> 
> 2019-07-17  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
> 
> * gdb.base/arm-main-cmse.c: New test.
> * gdb.base/arm-main-cmse.exp: New file.
> <diff>

If possible, it’s better to send the patch inline rather than as an attachment.
However, email clients have a habit of editing the format of patches when you
put them inline. Therefore I’d highly recommend using git send-email.

> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
> index 23dd40ea8beb1b00289a4cd4e65647399d351580..c56ed5dc6508a9c0994eebe1c90e60ede2df892f 100644
> --- a/gdb/arm-tdep.h
> +++ b/gdb/arm-tdep.h
> @@ -259,6 +259,7 @@ ULONGEST arm_get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
>  
>  CORE_ADDR arm_get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self,
>  					     CORE_ADDR val);
> +int check_section_name (struct obj_section *);
>  
>  int arm_get_next_pcs_is_thumb (struct arm_get_next_pcs *self);
>  
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index d244707210628ab045f677c0cbad3d8b0c6d6269..b51663031b45bda1df61ed6caca37deb531a5cd7 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -8212,6 +8212,53 @@ arm_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
>    return 1;
>  }
>  
> +/* A call to cmse secure entry function "foo" at "a" is modified by
> +     GNU ld as "b".
> +     a) bl xxxx <foo>
> +
> +     <foo>
> +     xxxx:
> +
> +     b) bl yyyy <__acle_se_foo>
> +
> +     section .gnu.sgstubs:
> +     <foo>
> +     yyyy: sg   // secure gateway
> +	   b.w xxxx <__acle_se_foo>  // original_branch_dest
> +
> +     <__acle_se_foo>
> +     xxxx:
> +
> +  When the control at "b", the pc contains "yyyy" (sg address) which is a
> +  trampoline and does not exist in source code.  This function returns
> +  the target pc "xxxx".  */

Add the name of the reference document, plus section.
(don’t need a full url link here).


> +
> +static CORE_ADDR
> +arm_skip_sg_jump_to_bw (CORE_ADDR pc, const char *name, struct objfile *objfile)

I think this is better named as arm_skip_cmse_entry or something similar.
It keeps the function name at a “higher” level.


> +{
> +  struct bound_minimal_symbol minsym;
> +  char *target_name;
> +  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
> +  target_name = (char *) alloca (target_len);

You might as well put “char *” on the start of the line, and remove the
declaration of target_name two lines ago.

> +  snprintf (target_name, target_len, "%s%s", "__acle_se_",name);
> +  minsym = lookup_minimal_symbol (target_name, NULL, objfile);
> +  if (minsym.minsym != NULL)
> +    return BMSYMBOL_VALUE_ADDRESS (minsym);

Could you use nullptr instead of NULL. nullptr is for where a pointer is
expected.

(You’ll find lots of places where NULL is used, but that’s because GDB has
 moved from being C to C++ and has lots of old code).


> +  return 0;
> +}
> +
> +/* Return 1 when pc holds an address that belongs to ".gnu.sgstubs"
> +   section.  */
> +int

Can you use bool instead of int. (again, you’ll see int elsewhere in the code
due to old C code).

> +check_section_name (struct obj_section *sec)

Needs a better name. Maybe arm_is_cmse_section.

> +{
> + if (sec != NULL && sec->the_bfd_section != NULL
> +     && sec->the_bfd_section->name != NULL
> +     && !strcmp (sec->the_bfd_section->name,".gnu.sgstubs”))

"0 == strcmp” is easier to parse than !strcmp.

Same NULL issues.


> +   return 1;
> + return 0;
> +}
> +
>  /* Recognize GCC and GNU ld's trampolines.  If we are in a trampoline,
>     return the target PC.  Otherwise return 0.  */
>  
> @@ -8221,6 +8268,7 @@ arm_skip_stub (struct frame_info *frame, CORE_ADDR pc)
>    const char *name;
>    int namelen;
>    CORE_ADDR start_addr;
> +  struct obj_section *section;
>  
>    /* Find the starting address and name of the function containing the PC.  */
>    if (find_pc_partial_function (pc, &name, &start_addr, NULL) == 0)
> @@ -8290,6 +8338,10 @@ arm_skip_stub (struct frame_info *frame, CORE_ADDR pc)
>  	return 0;
>      }
>  
> +  section = find_pc_section (pc);
> +  /* checks whether target pc belows to ".gnu.sgstubs" section.  */
> +  if (check_section_name (section))
> +    return arm_skip_sg_jump_to_bw (pc, name, section->objfile);
>    return 0;			/* not a stub */
>  }
>  
> diff --git a/gdb/testsuite/gdb.base/arm-main-cmse.c b/gdb/testsuite/gdb.base/arm-main-cmse.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..bde55075fdc21bc781a5dd6ac8221a550faca4c5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/arm-main-cmse.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>
> +extern void func();
> +void __acle_se_func ()
> +{
> +  printf("__acle_se_func\n");
> +}
> +
> +asm ("\t.section .gnu.sgstubs,\"ax\",%progbits\n"
> +     "\t.global func\n"
> +     "\t.type func, %function\n"
> +     "func:\n"
> +     "\tnop @sg\n"
> +     "\tb __acle_se_func @b.w”);

Add a comment to state why you are writing this in asm instead of C.

> +
> +void fun1 ()
> +{
> +  printf("In fun1\n");
> +}
> +
> +int main (void)
> +{
> +  while(1)
> +   {

Remove the while. Test programs should exit by themselves when run.

If GDB dies/exits in a specific way when the test is running, there is a chance the
test application may continue to run by itself An infinite loop would mean it runs
forever stealing cpu.


> +     func();
> +     fun1();
> +     __acle_se_func();
> +   }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/arm-main-cmse.exp b/gdb/testsuite/gdb.base/arm-main-cmse.exp


arm-main-cmse should either be arm-cmse or even better add something to specify
the cmse functionality being tested, for example arm-cmse-sgstubs.

Also, it should be in gdb.arch, not gdb.base.


> new file mode 100644
> index 0000000000000000000000000000000000000000..5c9abf43908f505f13151f3753f81db44b9eb6b3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/arm-main-cmse.exp
> @@ -0,0 +1,56 @@
> +# Copyright 2019 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This file is part of the gdb testsuite.
> +
> +if { ![istarget "arm*-*-*"]} {
> +     return 1
> +}
> +


> +standard_testfile arm-main-cmse.c
> +set exefile [standard_output_file ${testfile}.out]
> +set opts [list debug "additional_flags=-g -dA"]
> +
> +if { [gdb_compile ${srcdir}/${subdir}/${testfile}.c ${exefile} executable $opts] != ""} {
> +    return -1
> +}
> +
> +clean_restart ${exefile}

You can reduce the above section of code to:

standard_testfile
if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
    return -1
}


> +if ![runto_main] {

Add an untested line here if the runto fails.

> +   return -1
> +}
> +
> +set test "branch to func from main"
> +gdb_test "si" "0x.*" "$test"
> +
> +set test "next instruction in func"
> +gdb_test "ni" "0x.*" "$test"
> +
> +set test "branch to __acle_se_func from func"
> +gdb_test "ni" "__acle_se_func .*" "$test"
> +
> +set test "next in __acle_se_func function"
> +gdb_test "next" "5	  .*" "$test"
> +
> +set test "next in __acle_se_func function outputs __acle_se_func"
> +gdb_test "next" "__acle_se_func.*" "$test"
> +
> +gdb_test "continue" "Continuing.*" "continue to func() in main"
> +
> +set test "control branches to __acle_se_func from main via func"
> +gdb_test "step" "__acle_se_fun.*" "$test"
> +
> +set test "next in __acle_se_func function via func"
> +gdb_test "next" "__acle_se_func.*" "$test”
> 

When I run this test on HEAD without your patch it passes for me. I think there
might be something missing from the test?



Alan.