Re: [PATCH v2 1/2] Introduce "gdb/configure.nat" (and delete "gdb/config/*/*.mh" files)

[Sergio Durigan Junior <sergiodj@redhat.com>]

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/