From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46269 invoked by alias); 5 May 2017 03:58:10 -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 46233 invoked by uid 89); 5 May 2017 03:58:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= 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 ESMTP; Fri, 05 May 2017 03:57:55 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0470EC04B320; Fri, 5 May 2017 03:57:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 0470EC04B320 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 0470EC04B320 Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A90C36292C; Fri, 5 May 2017 03:57:55 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Simon Marchi , John Baldwin Subject: Re: [PATCH v2 1/2] Introduce "gdb/configure.nat" (and delete "gdb/config/*/*.mh" files) References: <20170425202309.15771-1-sergiodj@redhat.com> <20170503034931.4515-1-sergiodj@redhat.com> <20170503034931.4515-2-sergiodj@redhat.com> <9d941b3b-1b16-3c17-6cae-11940ade7477@redhat.com> Date: Fri, 05 May 2017 03:58:00 -0000 In-Reply-To: <9d941b3b-1b16-3c17-6cae-11940ade7477@redhat.com> (Pedro Alves's message of "Thu, 4 May 2017 17:16:35 +0100") Message-ID: <87pofoymyk.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00133.txt.bz2 Thanks for the review, Pedro. I am assuming that the issues pointed here should be addressed before I push the patch 2/2. I'm sending a v3 of the patch just to make sure everything is addressed properly. On Thursday, May 04 2017, Pedro Alves wrote: > On 05/03/2017 04:49 AM, Sergio Durigan Junior wrote: > >> diff --git a/gdb/Makefile.in b/gdb/Makefile.in >> index b865b7c..e26079a 100644 >> --- a/gdb/Makefile.in >> +++ b/gdb/Makefile.in >> @@ -903,7 +903,16 @@ ALL_TARGET_OBS = \ >> xtensa-tdep.o >> >> # Host-dependent makefile fragment comes in here. > > Stale comment. Should say something about the variables being > defined in configure.nat. Fixed. >> -@host_makefile_frag@ >> +NAT_FILE = @NAT_FILE@ >> +NATDEPFILES = @NATDEPFILES@ >> +NAT_CDEPS = @NAT_CDEPS@ >> +LOADLIBES = @LOADLIBES@ >> +MH_CFLAGS = @MH_CFLAGS@ >> +XM_CLIBS = @XM_CLIBS@ >> +NAT_GENERATED_FILES = @NAT_GENERATED_FILES@ >> +HAVE_NATIVE_GCORE_HOST = @HAVE_NATIVE_GCORE_HOST@ >> + >> +@NAT_EXTRA_FRAGS_FILE@ > > And this last line here is where we source the > fragment, so that comment above should move here > with s/Host/Native-target/ Done. > I think it should be "FRAG", singular. > "makefile fragment file" is the term most often used. OK. > Also, I think "host_makefile_frag" was lowercase because > that variable is not meant to be passed to make or any > tool make invokes. I was naming everything using uppercase because I thought it would be more consistent. But since you asked, I renamed the variable "nat_extra_makefile_frag". >> # End of host-dependent makefile fragment > > Likewise, mind the comments. Fixed. >> > >> -# Copyright (C) 2013-2017 Free Software Foundation, Inc. >> -# > >> diff --git a/gdb/config/i386/i386gnu.mh b/gdb/config/i386/i386gnu-extra.mh >> similarity index 58% >> rename from gdb/config/i386/i386gnu.mh >> rename to gdb/config/i386/i386gnu-extra.mh > > Why the "extra" rename ? If anything, I'd expect i386gnu.mh -> i386gnu.mn? git showed this as a rename, but it's really a new file. i386gnu.mh is gone, like every other previous *.mh file. Instead of using the old name, I decided to add the "-extra" suffix to make it explicit that the file contains only extra definitions, and is not the only thing taken into account for this native target. I initially disagree with your proposal to rename it to i386gnu.mn, so I'm keeping it this way. Please let me know if you really thing the "-extra" suffix shouldn't be there, and I can remove it. >> # Use our own user stubs for the msg rpcs, so we can make them time out, in >> # case the program is fucked, or we guess the wrong signal thread. >> -msg-MIGUFLAGS = -D'MSG_IMPORTS=waittime 1000;' >> +msg-MIGUFLAGS=-D'MSG_IMPORTS=waittime 1000;' >> >> # ick >> -MIGCOM = $(MIG) -cc cat - /dev/null >> +MIGCOM=$(MIG) -cc cat - /dev/null > > Simon recently went through all variables making sure > there's a space around the "=". This goes in the other > direction. You're right. I think this comes from an older version of the patch in which I was trying to define these as shell variables on configure.nat. Sorry about that, fixed in the new version. >> >> # Reply servers need special massaging of the code mig generates, to make >> # them work correctly for error returns in some cases. >> @@ -33,9 +21,4 @@ MIGCOM = $(MIG) -cc cat - /dev/null >> | $(MIGCOM) -sheader /dev/null -server /dev/null -user $*_U.c -header $*_U.h >> >> # MIG stubs are not yet ready for C++ compilation. >> -%_S.o %_U.o : COMPILE.post += -x c >> - >> -NAT_GENERATED_FILES = notify_S.h notify_S.c \ >> - process_reply_S.h process_reply_S.c \ >> - msg_reply_S.h msg_reply_S.c msg_U.h msg_U.c \ >> - exc_request_U.h exc_request_U.c exc_request_S.h exc_request_S.c >> +%_S.o %_U.o : COMPILE.post +=-x c" > > Was that quote char (") at the end added on purpose? > Looks like a typo. It is a typo. Same explanation as above. Sorry. Fixed. > Since most of the MIG related bits are left in the makefile > fragment, I wonder whether it really makes sense to move > the NAT_GENERATED_FILES setting far away from them. Good point, I hadn't thought of that. I moved NAT_GENERATED_FILES to i386gnu-extra.mh and removed it from configure.nat. >> diff --git a/gdb/configure.ac b/gdb/configure.ac >> index 50f6f59..f9d8aac 100644 >> --- a/gdb/configure.ac >> +++ b/gdb/configure.ac >> @@ -2199,29 +2199,22 @@ if test "${host}" != "${target}"; then >> fi >> AC_SUBST(target_subdir) >> >> -frags= >> +# Importing nat definitions. > > s/Importing/Import/ Fixed. >> +NAT_EXTRA_FRAGS_FILE=/dev/null >> if test "${gdb_native}" = "yes"; then >> - host_makefile_frag=${srcdir}/config/${gdb_host_cpu}/${gdb_host}.mh >> - if test ! -f ${host_makefile_frag}; then >> - AC_MSG_ERROR("*** Gdb does not support native target ${host}") >> - fi >> - frags="$frags $host_makefile_frag" >> -else >> - host_makefile_frag=/dev/null >> + . ${srcdir}/configure.nat >> + nativefile=$NAT_FILE >> fi > > There's (at least) a related comment that would be good > to update: > > # If nativefile (NAT_FILE) is not set in config/*/*.m[ht] files, we link > # to an empty version. > > as NAT_FILE now comes from configure.nat, not the .mh files. Updated. >> +# Variables defined here: >> +# >> +# NAT_FILE - The header file with definitions for this native target. >> +# >> +# NATDEPFILES - Source files required for native debugging on this >> +# native target. >> +# >> +# NAT_CDEPS - Dynamic symbols to be exported for libthread_db. >> +# >> +# LOADLIBES - Libraries against which GDB will be linked for this >> +# native target. >> +# >> +# MH_CFLAGS - Additional CFLAGS for this native target. > > At least MH_CFLAGS above is host-specific, not related to the > native target. See for example: > > # doublest.c currently assumes some properties of FP arithmetic > # on the host which require this. > MH_CFLAGS = -mieee > > # When compiled with cc, for debugging, this argument should be passed. > # We have no idea who our current compiler is though, so we skip it. > # MH_CFLAGS = -bnodelcsect > > These are clearly always needed to build gdb on this host, > independently of whether native target is being built. I.e., > when gdb is being built as a cross debugger, with > --target=something-else-not-the-host. Hence the MH_ prefix. Hm, OK, thanks for the explanation. I updated the description to say that the variable is host-specific. > Some of those variables are ripe for a rename / normalization, > but that's something that can always be done after, and will > actually be simpler with your patchset in. Absolutely. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/