From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128222 invoked by alias); 19 Jul 2019 16:24:48 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 127998 invoked by uid 89); 19 Jul 2019 16:24:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LOTSOFHASH,KAM_SHORT,KAM_STOCKGEN,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=caps X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 19 Jul 2019 16:24:24 +0000 Received: from [172.16.0.120] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C4DB11E077; Fri, 19 Jul 2019 12:24:20 -0400 (EDT) Subject: Re: [PATCH v2][GDB][ARM]: gdb cannot step across CMSE secure entry function code. To: Srinath Parvathaneni , "gdb-patches@sourceware.org" Cc: nd , Alan Hayward , "tom@tromey.com" References: From: Simon Marchi Message-ID: Date: Fri, 19 Jul 2019 16:24:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-07/txt/msg00473.txt.bz2 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 > > * 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_" 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 > > * 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 > + > + > + xxxx: > + > + b) bl yyyy <__acle_se_foo> > + > + section .gnu.sgstubs: > + > + 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 > +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 . > + > +# 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