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

[Pedro Alves <palves@redhat.com>]

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.

> -@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/

I think it should be "FRAG", singular.
"makefile fragment file" is the term most often used.

Also, I think "host_makefile_frag" was lowercase because
that variable is not meant to be passed to make or any
tool make invokes.

>  # End of host-dependent makefile fragment

Likewise, mind the comments.

>  

> -#  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?

>  # 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.

>  
>  # 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.

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.

> 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/

> +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.


> +# 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.

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.

Thanks,
Pedro Alves