This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Srinath,

I looked at the patch in more details, I just have some minor comments about formatting.

> gdb/ChangeLog:
>
> 2019-07-19  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
>
> 	* gdb/arm-tdep.c (arm_skip_cmse_entry):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.

You can use this ChangeLog entry:

	* arm-tdep.c (arm_skip_cmse_entry): New function.

Note:

- Keep the entry succinct, about what changed ("New function." is typical).  You explained well what the code does in the commit message, which is good.
- Remove gdb/, as you want the filename to be relative to the location of the ChangeLog file (gdb/ChangeLog)
- Space after the colon.

> 	(arm_is_sgstubs_section):New function. To check the current section is
> 	".gnu.sgstubs".

Here too, just "New function.".

> 	(arm_skip_stub):Add call to arm_skip_cmse_entry function.
> 	* gdb/arm-tdep.h (arm_is_sgstubs_section):New function declaration.
>
> gdb/testsuite/ChangeLog:
>
> 2019-07-19  Srinath Parvathaneni  <srinath.parvathaneni@arm.com>
>
> 	* gdb.arch/arm-cmse-sgstubs.c: New test.
> 	* gdb.arch/arm-cmse-sgstubs.exp: New file.
>
>
>
> ###############     Attachment also inlined for ease of reply    ###############
>
>
> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
> index
> 23dd40ea8beb1b00289a4cd4e65647399d351580..9482e765a7fc5bb58676096f6b879eae2a7c858e
> 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);
> +bool arm_is_sgstubs_section (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..e3b7ab6f096eb91da067d772b8798ffd0737e3d6
> 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -8211,6 +8211,53 @@ arm_get_longjmp_target (struct frame_info
> *frame, CORE_ADDR *pc)
>    *pc = extract_unsigned_integer (buf, INT_REGISTER_SIZE, byte_order);
>    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".  For more details please refer to section 5.4
> +  (Entry functions) and section 3.4.4 (C level development flow of secure code)
> +  of
> "armv8-m-security-extensions-requirements-on-development-tools-engineering-specification"
> +  document on www.developer.arm.com.  */
> +
> +static CORE_ADDR
> +arm_skip_cmse_entry (CORE_ADDR pc, const char *name, struct objfile *objfile)
> +{
> +  struct bound_minimal_symbol minsym;
> +  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
> +  char *target_name  = (char *) alloca (target_len);

Extra space before the equal sign.

> +  xsnprintf (target_name, target_len, "%s%s", "__acle_se_",name);

Space after comma.

> +  minsym = lookup_minimal_symbol (target_name, NULL, objfile);
> +  if (minsym.minsym != nullptr)
> +    return BMSYMBOL_VALUE_ADDRESS (minsym);
> +  return 0;
> +}

Not a strict rule, but my personal taste would be to add a few empty lines between logical blocks in the
function above, to space things a bit.  I think it would help readability.  Something like:

static CORE_ADDR
arm_skip_cmse_entry (CORE_ADDR pc, const char *name, struct objfile *objfile)
{
  int target_len = strlen (name) + strlen ("__acle_se_") + 1;
  char *target_name = (char *) alloca (target_len);
  xsnprintf (target_name, target_len, "%s%s", "__acle_se_", name);

  bound_minimal_symbol minsym
    = lookup_minimal_symbol (target_name, NULL, objfile);

  if (minsym.minsym != nullptr)
    return BMSYMBOL_VALUE_ADDRESS (minsym);

  return 0;
}

> +
> +/* Return true when sec points to ".gnu.sgstubs" section.  */
> +bool
> +arm_is_sgstubs_section (struct obj_section *sec)
> +{
> + if (sec != nullptr && sec->the_bfd_section != nullptr
> +     && sec->the_bfd_section->name != nullptr
> +     && streq (sec->the_bfd_section->name,".gnu.sgstubs"))
> +   return true;
> + return false;
> +}

For non-static functions, what we do is, in the .c file:

/* See arm-tdep.h.  */

bool
arm_is_sgstubs_section (struct obj_section *sec)
{
  ...
}

And in the .h file, put the doc:

/* Return true when SEC points to the ".gnu.sgstubs" section.  */

bool arm_is_sgstubs_section (struct obj_section *);

I know the current code in arm-tdep.h/arm-tdep.c is not all like that, but we try to be
consistent for new code or code that we modify.

Note: when referring to a parameter name, we put it in caps (SEC).

But actually, I think this function doesn't need to be visible to other files, as it's only
used in arm-tdep.c.  So should it be static?

Watch the indentation, the function body is indented with a single space, it should be two.
Also, it can be simplified to a single return statement:

  return (sec != nullptr
	  && sec->the_bfd_section != nullptr
	  && sec->the_bfd_section->name != nullptr
	  && streq (sec->the_bfd_section->name,".gnu.sgstubs"));

>  /* 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;

You can declare this variable at the point where you use it.

>
>    /* 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 address pc holds belows to ".gnu.sgstubs" section.  */

Capital C to "checks".  Also, I don't really understand the sentence, could it be
clarified?  Alan, maybe you can help with this?

> +  if (arm_is_sgstubs_section (section))
> +    return arm_skip_cmse_entry (pc, name, section->objfile);
>    return 0;			/* not a stub */
>  }
>
> diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
> b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..7f3b40f20c67abfdd2410614e7ee29ae77d37966
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>
> +extern void func();
> +void __acle_se_func ()
> +{
> +  printf("__acle_se_func\n");
> +}
> +
> +/* The following code is written using asm so that the instructions in function
> + * "func" will be placed in .gnu.sgstubs section of the executable.  */
> +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");
> +
> +void fun1 ()
> +{
> +  printf("In fun1\n");
> +}
> +
> +int main (void)
> +{
> +  func();
> +  fun1();
> +  __acle_se_func();
> +  func();
> +  return 0;
> +}

We try to format the test source files according to GNU standards, just like our source files.  That means:

- Return type on its own line
- Space before parenthesis in declarations, definitions and calls
- Copyright header at the top

> diff --git a/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
> b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..3416e887d9ebe5ebc52336eff15ba83a6d16df21
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/arm-cmse-sgstubs.exp
> @@ -0,0 +1,60 @@
> +# 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
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile ]} {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +   untested "could not run to main"
> +   return -1
> +}
> +
> +set test "branch to func from main"
> +gdb_test "si" "0x.*" "$test"

I would suggest just putting the test name on the gdb_test line, as we do everywhere:

  gdb_test "si" "0x.*" "branch to func from main"

If the same test name is used at multiple places, then it's worth putting it in a variable
to avoid duplicating it.

Thanks,

Simon


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]