From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 87964 invoked by alias); 18 Dec 2015 12:25:57 -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 87938 invoked by uid 89); 18 Dec 2015 12:25:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=sk:siginfo, collateral, converts, Common X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 18 Dec 2015 12:25:55 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id CB4A38F4F8; Fri, 18 Dec 2015 12:25:53 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tBICPqQ9012923; Fri, 18 Dec 2015 07:25:52 -0500 Message-ID: <5673FB50.1040507@redhat.com> Date: Fri, 18 Dec 2015 12:25:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Walfred Tedeschi , brobecker@adacore.com CC: gdb-patches@sourceware.org Subject: Re: [PATCH V2 1/5] Merges gdb and gdbserver implementation for siginfo. References: <1450371416-24270-1-git-send-email-walfred.tedeschi@intel.com> In-Reply-To: <1450371416-24270-1-git-send-email-walfred.tedeschi@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-12/txt/msg00358.txt.bz2 On 12/17/2015 04:56 PM, Walfred Tedeschi wrote: > The compatible siginfo handling from amd64-linux-nat.c and > gdbserver/linux-x86-low were extracted it into a new file > nat/amd64-linux-siginfo.c. > > > 2015-12-15 Walfred Tedeschi > > * nat/amd64-linux-siginfo.c: New file. > * nat/amd64-linux-siginfo.h: New file. > * Makefile.in (HFILES_NO_SRCDIR): Add new header to > HFILES_NO_SRCDIR. Add native object files rule for > amd64-linux-siginfo.o "HFILES_NO_SRCDIR" is already the specified context. Specify the rules as context: * Makefile.in (HFILES_NO_SRCDIR): Add nat/amd64-linux-siginfo.h. Add native object files rule for (amd64-linux-siginfo.o:): New rule. > * config/i386/linux64.mh (NATDEPFILES): Add amd64-linux-siginfo.o. > * amd64-linux-nat.c (compat_siginfo_from_siginfo) Mention the header inclusion: * amd64-linux-nat.c: Include "nat/amd64-linux-siginfo.h". (compat_siginfo_from_siginfo) ... > > gdbserver > > * configure.srv (srv_tgtobj): Add amd64-linux-siginfo.o. Should be: * configure.srv (x86_64-*-linux*): Add amd64-linux-siginfo.o to srv_tgtobj. I think you also need to add this here in the gdb_cv_i386_is_x86_64 case: i[34567]86-*-linux*) srv_regobj="$srv_i386_linux_regobj" srv_xmlfiles="$srv_i386_linux_xmlfiles" if test "$gdb_cv_i386_is_x86_64" = yes ; then srv_regobj="$srv_regobj $srv_amd64_linux_regobj" srv_xmlfiles="${srv_xmlfiles} $srv_amd64_linux_xmlfiles" fi > * linux-x86-low.c (compat_siginfo_from_siginfo) Mention the include: * linux-x86-low.c [__x86_64__]: Include "nat/amd64-linux-siginfo.h". ... > (siginfo_from_compat_siginfo, compat_x32_siginfo_from_siginfo) > (siginfo_from_compat_x32_siginfo and collateral structures): Move > to nat/amd64-linux-siginfo.c. > * Makefile.in (x86_64-*-linux*): Add amd64-linux-siginfo.o. The last entry doesn't make sense. Seems like it was talking about the configure.src change. You want: * Makefile.in (amd64-linux-siginfo.o:): New rule. > @@ -737,34 +335,15 @@ amd64_linux_siginfo_fixup (siginfo_t *native, gdb_byte *inf, int direction) > /* Is the inferior 32-bit? If so, then do fixup the siginfo > object. */ > if (gdbarch_bfd_arch_info (gdbarch)->bits_per_word == 32) > - { > - gdb_assert (sizeof (siginfo_t) == sizeof (compat_siginfo_t)); > - > - if (direction == 0) > - compat_siginfo_from_siginfo ((struct compat_siginfo *) inf, native); > - else > - siginfo_from_compat_siginfo (native, (struct compat_siginfo *) inf); > - > - return 1; > - } > + return amd64_linux_siginfo_fixup_common (native, inf , direction, > + FIXUP_32); Spurious space after "inf". Looks like that ended up in all amd64_linux_siginfo_fixup_common calls. > +++ b/gdb/nat/amd64-linux-siginfo.c > +#include > +#include "common-defs.h" > +#include "amd64-linux-siginfo.h" > + > +/* When GDB is built as a 64-bit application on linux, the > + PTRACE_GETSIGINFO data is always presented in 64-bit layout. Since > + debugging a 32-bit inferior with a 64-bit GDB should look the same > + as debugging it with a 32-bit GDB, we do the 32-bit <-> 64-bit > + conversion in-place ourselves. With the presence of possible different > + fields for host and target we have to guarantee that we use the This sentence is incomplete. > + Also, the first step is to make a copy from the original bits to the > + internal structure which is the super set. */ > + ??? > +} > + > +/* Converts the compatible siginfo into system siginfo. */ > +static void > +siginfo_from_compat_siginfo (siginfo_t *to, compat_siginfo_t *from) There should be an empty line between intro comment and function. Here and elsewhere. > + > +/* Convert a native/host siginfo object, into/from the siginfo in the > + layout of the inferiors' architecture. Returns true if any > + conversion was done; false otherwise. If DIRECTION is 1, then copy > + from INF to NATIVE. If DIRECTION is 0, then copy from NATIVE to INF. */ This should be already documented in the header. Here say: /* See whatever.h. */ > + > +int > +amd64_linux_siginfo_fixup_common (siginfo_t *native, gdb_byte *inf, > + int direction, > + enum amd64_siginfo_fixup_mode mode) > +{ > + > + if (mode == FIXUP_32) Spurious empty line. > +#ifndef AMD64_LINUX_SIGINFO_H > +#define AMD64_LINUX_SIGINFO_H 1 > + > + > +/* When GDB is built as a 64-bit application on linux, the > + PTRACE_GETSIGINFO data is always presented in 64-bit layout. Since > + debugging a 32-bit inferior with a 64-bit GDB should look the same > + as debugging it with a 32-bit GDB, we do the 32-bit <-> 64-bit > + conversion in-place ourselves. With the presence of possible different > + fields for host and target we have to guarantee that we use the > + Also, the first step is to make a copy from the original bits to the > + internal structure which is the super set. */ Duplicate comment. There should only be one copy, and it should probably be in the header file. > + > +/* ENUM to determine the kind of siginfo fixup to be performed. */ No need to say it's an enum: /* The kind of siginfo fixup to be performed. */ > + > +enum amd64_siginfo_fixup_mode > +{ > + FIXUP_32 = 1, > + FIXUP_X32 = 2 Please document each mode. > +}; > + > +/* Common code for performing the fixup of the siginfo. */ > + > +int > +amd64_linux_siginfo_fixup_common (siginfo_t *native, gdb_byte *inf, Function name only goes at column 0 in definitions, never in declarations. > + int direction, > + enum amd64_siginfo_fixup_mode mode); > + > +#endif Thanks, Pedro Alves