From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25023 invoked by alias); 23 Nov 2014 05:22:27 -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 25008 invoked by uid 89); 23 Nov 2014 05:22:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Sun, 23 Nov 2014 05:22:17 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id EB60211679A; Sun, 23 Nov 2014 00:22:15 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id qI6gLpdr4Xeu; Sun, 23 Nov 2014 00:22:15 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 28FEB116797; Sun, 23 Nov 2014 00:22:14 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 32B4C40F79; Sun, 23 Nov 2014 09:22:14 +0400 (RET) Date: Sun, 23 Nov 2014 05:22:00 -0000 From: Joel Brobecker To: Walfred Tedeschi Cc: vladimir@codesourcery.com, gdb-patches@sourceware.org Subject: Re: [PATCH] Add support for bound table in the Intel MPX context. Message-ID: <20141123052214.GD5774@adacore.com> References: <1412062583-24571-1-git-send-email-walfred.tedeschi@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1412062583-24571-1-git-send-email-walfred.tedeschi@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-11/txt/msg00551.txt.bz2 > In order to investigate the contents of the MPX boundary table two new commands > are added to GDB. "mpx-info-bounds" and "mpx-set-bounds" are used to display > and set values on the MPX bound table. > > 2014-08-12 Mircea Gherzan > Walfred Tedeschi > > * i386-tdep.c (MPX_BASE_MASK, MPX_BD_MASK, MPX_BT_MASK, MPX_BD_MASK_32, > MPX_BT_MASK_32): New macros. > (i386_mpx_set_bounds): New function that implements > the "mpx set-bounds" command. > (i386_mpx_enabled) Helper function to test MPX availability. > (i386_mpx_bd_base) Helper function to calculate the base directory > address. (i386_mpx_get_bt_entry) Helper function to access a bound > table entry. (i386_mpx_print_bounds) Effectively display bound > information. (_initialize_i386_tdep): Add new commands > to commands list. > * NEWS: List new commands for MPX support. > > testsuite: > > * gdb.arch/i386-mpx-map.c: New file. > * gdb.arch/i386-mpx-map.exp: New File. > > doc: > * gdb.texinfo: Add documentation about "mpx-info-bounds" > and "mpx-set-bounds" Sorry again for the late review. In the future, please give us a couple of weeks to review, and then feel free to ping us weekly afterwards. For the new commands, since they are mpx-specific, I would prefer if they were prefixed, rather than in the global "domain". I don't know how the other maintainers feel about that, but I'd rather keep non-prefixed commands to commands that are indeed general. I would like to propose... show mpx bound set mpx bound lbound ubound ... but the fact that setting the mpx bounds takes 2 arguments, this is somewhat of a departure from the typical "set" command. So I sent an RFC to discuss this separately: https://www.sourceware.org/ml/gdb-patches/2014-11/msg00550.html Please do not change your code until we have a resolution of the above. Comments on the code below... > +/* Show the MPX bounds for the given array/pointer. */ > + > +#define MPX_BASE_MASK (~(ULONGEST)0xfff) Space after "(ULONGEST)". > + > +static unsigned long > +i386_mpx_bd_base (void) All new functions should have an introductory comment. We decided to make no exceptions, even for obvious functions. However, for functions that implement a given interface, a simple one-line comment saying so suffices. Eg: /* Implement the "mpx-set-bound" command. */ Or: /* Implement the "frame_align" gdbarch method. */ Or: /* Implement the "to_open" target_ops method. */ The idea in the last couple of examples is that the expected behavior of that routine is expected to match what the gdbarch/target_ops vector documents. So, no need to duplicate it (which makes also changing the interface less labour intensive, as we only need to adjust the doco at a single location). Can you add those to all new functions you're defining here? > + struct regcache *rcache; > + struct gdbarch_tdep *tdep; > + ULONGEST ret; > + enum register_status stat; > + struct gdb_exception except; > + > + rcache = get_current_regcache (); > + tdep = gdbarch_tdep (get_regcache_arch (rcache)); > + > + stat = regcache_raw_read_unsigned (rcache, tdep->bndcfgu_regnum, &ret); > + > + if (stat != REG_VALID) > + error (_("BNDCFGU register invalid, read status %d."), stat); > + > + return ret & MPX_BASE_MASK; > +} > + > +/* Check if the current target is MPX enabled. */ > + > +int > +i386_mpx_enabled (void) Can you explain why this function is made global rather than static? > +/* Pure pointer math for 64 bit address translation. */ > +static CORE_ADDR > +i386_mpx_get_bt_entry (CORE_ADDR ptr, CORE_ADDR bd_base) Another GDB Coding Style rule (sorry...): Can you always have an empty line between a function's documentation and the function's start? Also, can you adjust the comment to actually say what it does, rather than talk about how it does it? > +{ > + ULONGEST offset1; > + ULONGEST offset2; > + ULONGEST mpx_bd_mask, bd_ptr_r_shift, bd_ptr_l_shift; > + ULONGEST bt_mask, bt_select_r_shift, bt_select_l_shift; > + CORE_ADDR bd_entry_addr; > + CORE_ADDR bt_addr; > + CORE_ADDR bd_entry; > + > + struct gdbarch *gdbarch = get_current_arch (); > + int ret = 0; Any reason for the empty space above? As it turns out, it tricked me into thinking that the gdbarch assignment wasn't also a variable declaration, and so I thought that "ret" was declared in the middle of code, which is currently not allowed. > + ret = target_read_memory (bd_entry_addr, (gdb_byte *) &bd_entry, > + sizeof (bd_entry)); That looks suspicious - I think you're making the assumption that host and target have the same endianness. To read a pointer from inferior memory, you have read_memory_typed_address which should be doing what you are looking for. > + > + if (ret) > + error (_("Cannot read bounds directory entry at 0x%lx."), > + (long) bd_entry_addr); > + > + if ((bd_entry & 0x1) == 0) > + error (_("Invalid bounds directory entry at 0x%lx."), > + (long) bd_entry_addr); If you use read_memory_typed_address, then the first error message should disappear. But if not, then use %s combined with paddress in both error messages to format CORE_ADDR values. > +static void > +i386_mpx_info_bounds (char *args, int from_tty) > +{ > + CORE_ADDR bd_base = 0; > + CORE_ADDR addr; > + CORE_ADDR bt_entry_addr = 0; > + CORE_ADDR bt_entry[4]; > + int ret; > + > + if (!i386_mpx_enabled ()) > + error (_("MPX not supported on this target.")); > + > + if (!args) > + error (_("Address of pointer variable expected.")); For pointer comparisons, the GDB project uses explicit comparisons. So can you replace "!args" by "args != NULL"? > + addr = parse_and_eval_address (args); > + > + bd_base = i386_mpx_bd_base (); > + bt_entry_addr = i386_mpx_get_bt_entry (addr, bd_base); > + > + ret = target_read_memory (bt_entry_addr, (gdb_byte *) bt_entry, > + sizeof (bt_entry)); Same as above - I think there is an endianness issue, here. > + if (ret) > + error (_("Cannot read bounds table entry at %lx."), bt_entry_addr); If you decide to implement the read above such that you keep the error message, then use paddress to print the address. > +static void > +i386_mpx_set_bounds (char *args, int from_tty) > +{ > + CORE_ADDR bd_base = 0; > + CORE_ADDR addr, lower, upper; > + CORE_ADDR bt_entry_addr = 0; > + CORE_ADDR bt_entry[4]; > + int ret; > + char *addr_str, *lower_str, *upper_str, *tmp; > + > + if (!i386_mpx_enabled ()) > + error ("MPX not supported on this target."); Missing i18n marker. > + > + if (!args) > + error ("Address of pointer variable expected."); args != NULL again. Missing i18n marker. There are other instances of these below, but it's unclear if that code is going to stay. So I will allow myself to skip mentioning those, but could you please make sure you fix them all? > + addr_str = strtok (args, " "); Please use gdb_buildargv to parse the arguments instead of doing it yourself via strtok. Besides, those function modify its argument, which means your function has an unexpected side-effect, and it would also stop working the day we make "args" parameter a const. I suspect this is also going to simplify a bit the verification of the number of arguments. > + ret = target_read_memory (bt_entry_addr, (gdb_byte *) bt_entry, > + sizeof (bt_entry)); > + if (ret) > + error ("Cannot read bounds table entry at 0x%lx", (long) bt_entry_addr); Same as before regarding reading an address from inferior memory. > + bt_entry[0] = (uint64_t) lower; > + bt_entry[1] = ~(uint64_t) upper; > + > + ret = target_write_memory (bt_entry_addr, (gdb_byte *) bt_entry, > + sizeof (bt_entry)); Same thing here. You'll need to convert the address to target endianness first, and then write that. > + if (ret) > + error ("Cannot write bounds table entry at 0x%lx", (long) bt_entry_addr); > + else > + i386_mpx_print_bounds (bt_entry); Personally, I would not re-print the bounds. If the user really wants to confirm his operation, I'd let him call the command to show it again. > diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h > index e0950a3..42da850 100644 > --- a/gdb/i386-tdep.h > +++ b/gdb/i386-tdep.h > @@ -434,4 +434,7 @@ extern int i386_stap_is_single_operand (struct gdbarch *gdbarch, > extern int i386_stap_parse_special_token (struct gdbarch *gdbarch, > struct stap_parse_info *p); > > +int > +i386_mpx_enabled (void); This is related to my question asking whether you really need this function to be non-static. If the answer is yes, for some reason, The function's name should not be be on the first column of the next line (this is reserved for function implementations, in order to facilitate locating them via grep). So: int i386_mpx_enabled (void); > diff --git a/gdb/testsuite/gdb.arch/i386-mpx-map.c b/gdb/testsuite/gdb.arch/i386-mpx-map.c > new file mode 100644 > index 0000000..441131a > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/i386-mpx-map.c [...] > +#include Is stdio.h really needed here? Using headers such as this one make it much harder to run this test on bare-metal targets. > +#include > + > +#include "x86-cpuid.h" > + > +#ifndef NOINLINE > +#define NOINLINE __attribute__ ((noinline)) > +#endif > + > +#define SIZE 5 > + > +typedef int T; > + > +unsigned int have_mpx (void) NOINLINE; > + > +unsigned int NOINLINE > +have_mpx (void) > +{ > + unsigned int eax, ebx, ecx, edx; > + > + if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx)) > + return 0; > + > + if ((ecx & bit_OSXSAVE) == bit_OSXSAVE) > + { > + if (__get_cpuid_max (0, NULL) < 7) > + return 0; > + > + __cpuid_count (7, 0, eax, ebx, ecx, edx); > + > + if ((ebx & bit_MPX) == bit_MPX) > + return 1; > + else > + return 0; > + } > + return 0; > +} > + > +void > +foo (T * p) No space between "*" and "p", please. > +{ > + T *x; Empty line after the end of the local variable declarations. > +#if defined __GNUC__ && !defined __INTEL_COMPILER > + __bnd_store_ptr_bounds (p, &p); > +#endif > + x = p + SIZE - 1; > +#if defined __GNUC__ && !defined __INTEL_COMPILER > + __bnd_store_ptr_bounds (x, &x); > +#endif > + return; /* after-assign */ > +} > + > +int > +main (void) > +{ > + if (have_mpx ()) > + { > + T *a = NULL; Same here, please (empty line). > + a = calloc (SIZE, sizeof (T)); /* after-decl */ > +#if defined __GNUC__ && !defined __INTEL_COMPILER > + __bnd_store_ptr_bounds (a, &a); > +#endif > + foo (a); /* after-alloc */ > + free (a); > + } > + return 0; > +} > diff --git a/gdb/testsuite/gdb.arch/i386-mpx-map.exp b/gdb/testsuite/gdb.arch/i386-mpx-map.exp > new file mode 100644 > index 0000000..539e1dd > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/i386-mpx-map.exp > @@ -0,0 +1,80 @@ > +# Copyright 2014 Free Software Foundation, Inc. > +# > +# Contributed by Intel Corp. , > +# > +# > +# 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 . > + > +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } { > + verbose "Skipping x86 MPX tests." > + return > +} > + > +standard_testfile > + > +set comp_flags "-fmpx -I${srcdir}/../nat/" > + > +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \ > + [list debug nowarnings additional_flags=${comp_flags}]] } { > + return -1 > +} > + > +if ![runto_main] { > + untested "could not run to main" > + return -1 > +} > + > +set supports_mpx 0 > +set test "probe MPX support" > +send_gdb "print have_mpx ()\r" > + > +gdb_test_multiple "print have_mpx()" $test { > + -re ".. = 1\r\n$gdb_prompt $" { > + pass $test > + set supports_mpx 1 > + } > + -re ".. = 0\r\n$gdb_prompt $" { > + pass $test > + } > +} I think there is a problem here, and you're probably sending the "print have_mpx()" command twice. Please remove the call to "send_gdb" which we try to avoid as much as possible", and see if the test still works. The part that really surprises me is that you'd think that this could cause synchronization between sent commands and expected output in the rest of the testcase to be off by one command. How did it still work? > +if { !$supports_mpx } { > + unsupported "processor does not support MPX" > + return > +} > + > +gdb_breakpoint [ gdb_get_line_number "after-decl" ] > +gdb_breakpoint [ gdb_get_line_number "after-alloc" ] > +gdb_breakpoint [ gdb_get_line_number "after-assign" ] > + > +gdb_test "mpx-info-bounds 0x0" "Invalid bounds directory entry at 0x\[0-9a-fA-F\]+." "NULL address of the pointer" You can use $hex for matching addreses. I'll let you adjust the rest of this file without explicitly pointing out the locations where you can do so... > +gdb_continue_to_breakpoint "after-decl" ".*after-decl.*" > +#gdb_test "mpx-info-bounds a" "Invalid bounds directory entry at 0x\[0-9a-fA-F\]+." "pointer instead of pointer address" > +# Fix size Please do not introduce commented-out code, at least not without explicitly explaining why. > +gdb_test "mpx-info-bounds &a" "Null bounds on map: pointer value = 0x0+." "address of a NULL pointer" You might want to escape the last dot, if you want to make sure that value is 0. > +gdb_test "mpx-info-bounds &x" "\\\{lbound = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\}: pointer value = 0x\[0-9a-fA-F\]+, size = -1, metadata = 0x0+." "pointer after assignment" Same here for the last dot. -- Joel