Re: Move gdbsupport to the top level

[Pedro Alves <palves at redhat dot com>]

Hi,

On 7/11/19 10:02 PM, Tom Tromey wrote:
> This patch moves the gdbsupport directory to the top level.  This is
> the next step in the ongoing project to move gdbserver to the top
> level.
> 
> The bulk of this patch was created by "git mv gdb/gdbsupport gdbsupport".
> 
> This patch then adds a build system to gdbsupport and wires it into
> the top level.  Then it changes gdb to use the top-level build.
> 
> gdbserver, on the other hand, is not yet changed.  It still does its
> own build of gdbsupport.

(I'm looking at the branch, so I'm not sure I'm commenting on the patch
as it was posted.)

This all builds fine for me when I build normally, but,
this breaks building gdbserver standalone, without configuring 
from the top level.  I.e.:

 $ mkdir build-gdbserver
 $ cd build-gdbserver
 $ .../src/gdb/gdbserver/configure
 ...
 make: *** No rule to make target '../../gdbsupport/libgdbsupport.a', needed by 'gdbserver'.  Stop.
 make: *** Waiting for unfinished jobs....
 ...


> 
> I wasn't able to send this through the buildbot.  I did test it on
> x86-64 Fedora 29.  If you want to try it, it is on the branch
> submit/move-gdbsupport-to-top in my github.
> 
> 
> This patch was also too big to send the usual way, so I have compressed
> it and added it as an attachment.  Sorry about that.

The patch is much smaller and easier to read without the
the generated files.  It's what I did here locally to try to
make sense of what I was seeing.

> -  CONFIG_OBS="$CONFIG_OBS \$(SUBDIR_UNITTESTS_OBS) gdbsupport/selftest.o selftest-arch.o"
> -  CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_UNITTESTS_SRCS) gdbsupport/selftest.c selftest-arch.c"
> +  CONFIG_OBS="$CONFIG_OBS \$(SUBDIR_UNITTESTS_OBS)  selftest-arch.o"
> +  CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_UNITTESTS_SRCS) selftest-arch.c"

Spurious double space.

So I guess the biggest change here is how/where config.h is included.

>From the branch:

> --- c/gdb/nat/linux-btrace.c
> +++ w/gdb/nat/linux-btrace.c
> @@ -20,6 +20,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include "gdbsupport/common-defs.h"
> +#include <config.h>

I noticed that only a few nat/ files needed to include config.h,
which I found surprising/confusing at first.

What are the new rules here?  Add <config.h> on as-needed basis,
or should we have some nat.h file that is included by
all nat/ files, and same for arch/ ?  The former seems a bit
error prone, given that you could move code around and not realize
that an #ifdef is disabling something because you missed config.h.

Alternatively, I guess we could move the required bits from
gdb&gdbserver's configury to gdbsupport's, so that config.h
wasn't ever necessary in shared code.  Not sure whether that
would be a bit of an abstraction violation.

Thanks,
Pedro Alves