From patchwork Sun Apr 19 22:41:03 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Schwinge X-Patchwork-Id: 6324 Received: (qmail 28861 invoked by alias); 19 Apr 2015 22:41:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 28848 invoked by uid 89); 19 Apr 2015 22:41:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 19 Apr 2015 22:41:28 +0000 Received: from svr-orw-fem-02x.mgc.mentorg.com ([147.34.96.206] helo=SVR-ORW-FEM-02.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1Yjxu0-0005US-4S from Thomas_Schwinge@mentor.com ; Sun, 19 Apr 2015 15:41:24 -0700 Received: from tftp-cs (147.34.91.1) by svr-orw-fem-02.mgc.mentorg.com (147.34.96.168) with Microsoft SMTP Server id 14.3.224.2; Sun, 19 Apr 2015 15:41:23 -0700 Received: by tftp-cs (Postfix, from userid 49978) id DFE5BC220E; Sun, 19 Apr 2015 15:41:22 -0700 (PDT) From: Thomas Schwinge To: David Michael , Justus Winter <4winter@informatik.uni-hamburg.de>, CC: , Samuel Thibault Subject: Re: [committed mig] Do not generate code dereferencing type-punned pointers In-Reply-To: References: <1424016590-17786-1-git-send-email-4winter@informatik.uni-hamburg.de> <20150302160349.22493.59537@thinkbox.jade-hamburg.de> User-Agent: Notmuch/0.9-101-g81dad07 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) Date: Mon, 20 Apr 2015 00:41:03 +0200 Message-ID: <87mw233fnk.fsf@kepler.schwinge.homeip.net> MIME-Version: 1.0 Hi! David and Justus, thanks for initially looking into this! On Mon, 2 Mar 2015 12:51:41 -0500, David Michael wrote: > On Mon, Mar 2, 2015 at 11:03 AM, Justus Winter > <4winter@informatik.uni-hamburg.de> wrote: > > Hi David :) > > > > thanks for cleaning up after me ;) (again and again...) > > > > Quoting David Michael (2015-02-18 05:39:46) > >> On Sun, Feb 15, 2015 at 11:09 AM, Justus Winter > >> <4winter@informatik.uni-hamburg.de> wrote: > >> > * utils.c (WriteFieldDeclPrim): Generate a union with an additional > >> > pointer field for variable-length arrays. > >> > >> This makes GDB's awk script go haywire because it doesn't know how to > >> deal with unions. The following is a workaround to get it building > >> again, but I'm not sure of its correctness. Can someone more > >> knowledgeable than me check on this? > > > > Not that I'm claiming to know why gdb does what it does, but Samuel > > asked me to look into this. > > Thanks for looking into it. > > > For some reason (proxying?), gdb uses mig to create server-side stubs > > for the *reply*-half of some protocols. It then uses an awk-script to > > insert the following code into the server-stubs: > > > > if (In0P->Head.msgh_size == sizeof (Reply) > > && ! (In0P->Head.msgh_bits & MACH_MSGH_BITS_COMPLEX) > > && ! BAD_TYPECHECK(&In0P->return_codeType, &return_codeCheck) > > && In0P->return_code != 0) > > /* Error return, only the error code argument is passed. */ > > { > > kern_return_t (*sfun)(mach_port_t, kern_return_t, int, data_t, mach_msg_type_number_t, data_t, mach_msg_type_number_t) = S_proc_getprocinfo_reply; > > OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t))sfun) (In0P->Head.msgh_request_port, In0P->return_code); > > return; > > } > > > > This is inserted *before* any type checks. The awk script has this to > > say: > > > > # The first args type checking statement; we need to insert our chunk of > > # code that bypasses all the type checks if this is an error return, after > > # which we're done until we get to the next function. Handily, the size > > # of mig's Reply structure is also the size of the alternate Request > > # structure that we want to check for. > > [...] > > # Force the function into a type that only takes the first two args, via > > # the temp variable SFUN (is there another way to correctly do this cast?). > > # This is possibly bogus, but easier than supplying bogus values for all > > # the other args (we can't just pass 0 for them, as they might not be scalar). > > > > I don't see why it bothers with SFUN in the first place. It could just do > > > > OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t)) S_proc_getprocinfo_reply) (In0P->Head.msgh_request_port, In0P->return_code); > > That was my first thought as well, but the comment "is there another > way to correctly do this cast?" made me think I was missing some > subtlety. Thomas took care of updating this last time mig was > modified; maybe he is familiar with its design choices? Nope, sorry. :-| > > right? If so, the main reason for parsing the struct is gone, and we > > could simplify the script a lot and thus making it less likely to > > break in the future. Getting rid of it entirely would be nice though. > > > > A word about the patch. It seems to cheat by assuming `data_t' as the > > type as soon at it sees a union. That shouldn't matter since the type > > is only used for the stupid cast. But the better way would be to just > > get rid of the parsing in the first place, or the whole script when > > we're at it. > > Yes, I'd be in favor of anything that makes the build process less > dependent on the exact mig output. I don't know enough about what gdb > is doing to remove this script entirely, but I'll see if it still > works when using a direct cast instead of all the argument parsing. A good suggestions, thanks. I now pushed the following two patches to GDB master: commit d214e5e79e38b18bc3786b3e8ba0e55fdbba294b Author: Thomas Schwinge Date: Mon Apr 20 00:22:32 2015 +0200 [GDB] Hurd: Simplify the reply_mig_hack.awk script. gdb/ * reply_mig_hack.awk: Don't bother to declare an intermediate function pointer variable. ... allowing us to simplify the parsing a little bit. And, instead of "warning: initialization from incompatible pointer type", we now get "warning: function called through a non-compatible type". Oh well. --- gdb/ChangeLog | 5 +++++ gdb/reply_mig_hack.awk | 25 ++++++++++++++++--------- 2 files changed, 21 insertions(+), 9 deletions(-) Grüße, Thomas diff --git gdb/ChangeLog gdb/ChangeLog index 69e5c48..093686f 100644 --- gdb/ChangeLog +++ gdb/ChangeLog @@ -1,3 +1,8 @@ +2015-04-20 Thomas Schwinge + + * reply_mig_hack.awk: Don't bother to declare an intermediate + function pointer variable. + 2015-04-17 Doug Evans * solib-svr4.c (svr4_exec_displacement): Rename outer "displacement" diff --git gdb/reply_mig_hack.awk gdb/reply_mig_hack.awk index 2c2a30a..7eab504 100644 --- gdb/reply_mig_hack.awk +++ gdb/reply_mig_hack.awk @@ -60,7 +60,7 @@ parse_phase == 3 && /}/ { print; next; } -parse_phase == 3 { +parse_phase == 3 && num_args == 0 { # The type field for an argument. arg_type_code_name[num_args] = $2; sub (/;$/, "", arg_type_code_name[num_args]) # Get rid of the semi-colon @@ -68,11 +68,22 @@ parse_phase == 3 { print; next; } +parse_phase == 3 && num_args == 1 { + # We've got more than one argument (but we don't care what it is). + num_args++; + print; next; +} + +parse_phase == 3 { + # We've know everything we need; now just wait for the end of the Request + # struct. + print; next; +} + parse_phase == 4 { # The value field for an argument. arg_name[num_args] = $2; sub (/;$/, "", arg_name[num_args]) # Get rid of the semi-colon - arg_type[num_args] = $1; num_args++; parse_phase = 3; print; next; @@ -109,15 +120,11 @@ parse_phase == 5 && /^#if[ \t]TypeCheck/ { print "\t && In0P->" arg_name[0] " != 0)"; print "\t /* Error return, only the error code argument is passed. */"; print "\t {"; - # Force the function into a type that only takes the first two args, via - # the temp variable SFUN (is there another way to correctly do this cast?). + # Force the function user_function_name into a type that only takes the first + # two arguments. # This is possibly bogus, but easier than supplying bogus values for all # the other args (we can't just pass 0 for them, as they might not be scalar). - printf ("\t kern_return_t (*sfun)(mach_port_t"); - for (i = 0; i < num_args; i++) - printf (", %s", arg_type[i]); - printf (") = %s;\n", user_function_name); - print "\t OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t))sfun) (In0P->Head.msgh_request_port, In0P->" arg_name[0] ");"; + print "\t OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t)) " user_function_name ") (In0P->Head.msgh_request_port, In0P->" arg_name[0] ");"; print "\t return;"; print "\t }"; print ""; commit 110f91128cf3e047eb1e04d346c27d71cc33fb9c Author: Thomas Schwinge Date: Mon Apr 20 00:31:54 2015 +0200 [GDB] Hurd: Robustify the reply_mig_hack.awk script. ..., so that it also works with the GNU MIG 1.5 just released. gdb/ * reply_mig_hack.awk: Robustify parsing. --- gdb/ChangeLog | 2 ++ gdb/reply_mig_hack.awk | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git gdb/ChangeLog gdb/ChangeLog index 093686f..4bca7d6 100644 --- gdb/ChangeLog +++ gdb/ChangeLog @@ -1,5 +1,7 @@ 2015-04-20 Thomas Schwinge + * reply_mig_hack.awk: Robustify parsing. + * reply_mig_hack.awk: Don't bother to declare an intermediate function pointer variable. diff --git gdb/reply_mig_hack.awk gdb/reply_mig_hack.awk index 7eab504..b731115 100644 --- gdb/reply_mig_hack.awk +++ gdb/reply_mig_hack.awk @@ -49,7 +49,7 @@ parse_phase == 2 { print; next; } -parse_phase == 3 && /}/ { +parse_phase == 3 && /} Request/ { # The args structure is over. if (num_args > 1) parse_phase = 5; @@ -62,6 +62,9 @@ parse_phase == 3 && /}/ { parse_phase == 3 && num_args == 0 { # The type field for an argument. + # This won't be accurate in case of unions being used in the Request struct, + # but that doesn't matter, as we'll only be looking at arg_type_code_name[0], + # which will not be a union type. arg_type_code_name[num_args] = $2; sub (/;$/, "", arg_type_code_name[num_args]) # Get rid of the semi-colon parse_phase = 4; @@ -82,6 +85,9 @@ parse_phase == 3 { parse_phase == 4 { # The value field for an argument. + # This won't be accurate in case of unions being used in the Request struct, + # but that doesn't matter, as we'll only be looking at arg_name[0], which + # will not be a union type. arg_name[num_args] = $2; sub (/;$/, "", arg_name[num_args]) # Get rid of the semi-colon num_args++;