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

Sergio Durigan Junior
2017-05-05T03:58:10Z

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