[PATCH] Simplify the IPA parts of the gdbserver Makefile

[PATCH] Simplify the IPA parts of the gdbserver Makefile

[Christian Biesinger via gdb-patches]

Instead of adding the Gnulib CFLAGS and partially undoing them, just
don't add them.

Requires a minor change to common-defs.h, and depends on
https://sourceware.org/ml/gdb-patches/2019-11/msg00908.html

gdb/ChangeLog:

2019-11-26  Christian Biesinger  <cbiesinger@google.com>

	* gdbsupport/common-defs.h: Don't use Gnulib when building IPA.

gdb/gdbserver/ChangeLog:

2019-11-26  Christian Biesinger  <cbiesinger@google.com>

	* Makefile.in: Don't use Gnulib headers when building IPA.

Change-Id: I93266a721def7e5cd40b5a2f2ed959c70446a2d8
---
 gdb/gdbserver/Makefile.in    | 16 +++++-----------
 gdb/gdbsupport/common-defs.h |  5 ++++-
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 16012dddcb..aba0eea78f 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -119,8 +119,7 @@ GNULIB_H = $(GNULIB_BUILDDIR)/import/string.h @GNULIB_STDINT_H@
 # e.g.: "target/wait.h".
 #
 INCLUDE_CFLAGS = -I. -I${srcdir} \
-	-I$(srcdir)/../regformats -I$(srcdir)/.. -I$(INCLUDE_DIR) \
-	$(INCGNU)
+	-I$(srcdir)/../regformats -I$(srcdir)/.. -I$(INCLUDE_DIR)
 
 # M{H,T}_CFLAGS, if defined, has host- and target-dependent CFLAGS
 # from the config/ directory.
@@ -143,7 +142,8 @@ CPPFLAGS = @CPPFLAGS@
 INTERNAL_CFLAGS_BASE = ${CXXFLAGS} ${GLOBAL_CFLAGS} \
 	${PROFILE_CFLAGS} ${INCLUDE_CFLAGS} ${CPPFLAGS}
 INTERNAL_WARN_CFLAGS = ${INTERNAL_CFLAGS_BASE} $(WARN_CFLAGS)
-INTERNAL_CFLAGS = ${INTERNAL_WARN_CFLAGS} $(WERROR_CFLAGS) -DGDBSERVER
+NON_GNU_CFLAGS = ${INTERNAL_WARN_CFLAGS} $(WERROR_CFLAGS) -DGDBSERVER
+INTERNAL_CFLAGS = ${NON_GNU_CFLAGS} $(INCGNU)
 
 # LDFLAGS is specifically reserved for setting from the command line
 # when running make.
@@ -440,7 +440,7 @@ IPA_LIB = libinproctrace.so
 $(IPA_LIB): $(sort $(IPA_OBJS)) ${CDEPS}
 	$(SILENCE) rm -f $(IPA_LIB)
 	$(ECHO_CXXLD) $(CC_LD) -shared -fPIC -Wl,--soname=$(IPA_LIB) \
-		-Wl,--no-undefined $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
+		-Wl,--no-undefined $(NON_GNU_CFLAGS) $(INTERNAL_LDFLAGS) \
 		-o $(IPA_LIB) ${IPA_OBJS} -ldl -pthread
 
 # Put the proper machine-specific files first, so M-. on a machine
@@ -551,15 +551,9 @@ regdat_sh = $(srcdir)/../regformats/regdat.sh
 
 UST_CFLAGS = $(ustinc) -DCONFIG_UST_GDB_INTEGRATION
 
-# Undo gnulib replacements for the IPA shared library build.
-# The gnulib headers are still needed, but gnulib is not linked
-# into the IPA lib so replacement apis don't work.
-UNDO_GNULIB_CFLAGS = -Drpl_strerror=strerror
-
 # Note, we only build the IPA if -fvisibility=hidden is supported in
 # the first place.
-IPAGENT_CFLAGS = $(INTERNAL_CFLAGS) $(UST_CFLAGS) \
-	$(UNDO_GNULIB_CFLAGS) \
+IPAGENT_CFLAGS = $(NON_GNU_CFLAGS) $(UST_CFLAGS) \
 	-fPIC -DIN_PROCESS_AGENT \
 	-fvisibility=hidden
 
diff --git a/gdb/gdbsupport/common-defs.h b/gdb/gdbsupport/common-defs.h
index 4fa84ff8e5..d28e996abe 100644
--- a/gdb/gdbsupport/common-defs.h
+++ b/gdb/gdbsupport/common-defs.h
@@ -108,9 +108,12 @@
    MinGW, gnulib might enable __USE_MINGW_ANSI_STDIO, which may or not
    require use of attribute gnu_printf instead of printf.  gnulib
    checks that at configure time.  Since _GL_ATTRIBUTE_FORMAT_PRINTF
-   is compatible with ATTRIBUTE_PRINTF, simply use it.  */
+   is compatible with ATTRIBUTE_PRINTF, simply use it.
+   Since IPA does not use Gnulib, we don't do it there.  */
+#ifndef IN_PROCESS_AGENT
 #undef ATTRIBUTE_PRINTF
 #define ATTRIBUTE_PRINTF _GL_ATTRIBUTE_FORMAT_PRINTF
+#endif
 
 #if GCC_VERSION >= 3004
 #define ATTRIBUTE_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH] Simplify the IPA parts of the gdbserver Makefile

[Pedro Alves]

On 11/26/19 7:10 PM, Christian Biesinger via gdb-patches wrote:
> Instead of adding the Gnulib CFLAGS and partially undoing them, just
> don't add them.
> 

Currently the IPA uses gnulib headers, but not the function
replacements.  What's the end goal you're after?  Why is this
an improvement?

Thanks,
Pedro Alves

Re: [PATCH] Simplify the IPA parts of the gdbserver Makefile

[Christian Biesinger via gdb-patches]

On Tue, Nov 26, 2019 at 2:29 PM Pedro Alves <palves@redhat.com> wrote:
>
> On 11/26/19 7:10 PM, Christian Biesinger via gdb-patches wrote:
> > Instead of adding the Gnulib CFLAGS and partially undoing them, just
> > don't add them.
> >
>
> Currently the IPA uses gnulib headers, but not the function
> replacements.  What's the end goal you're after?  Why is this
> an improvement?

Right. The end goal is
https://sourceware.org/ml/gdb-patches/2019-11/msg00922.html -- I want
to be able to call safe_strerror from more places, which means the
implementation of safe_strerror needs to be able to call glibc's
strerror_r. I suppose I could try adding it to UNDO_GNULIB_CFLAGS,
maybe, but it seemed less confusing to change it like this.

Christian

Re: [PATCH] Simplify the IPA parts of the gdbserver Makefile

[Pedro Alves]

On 11/26/19 8:32 PM, Christian Biesinger wrote:
>> Currently the IPA uses gnulib headers, but not the function
>> replacements.  What's the end goal you're after?  Why is this
>> an improvement?
> 
> Right. The end goal is
> https://sourceware.org/ml/gdb-patches/2019-11/msg00922.html -- I want
> to be able to call safe_strerror from more places, which means the
> implementation of safe_strerror needs to be able to call glibc's
> strerror_r. I suppose I could try adding it to UNDO_GNULIB_CFLAGS,
> maybe, but it seemed less confusing to change it like this.
So currently strerror_r is replaced by gnulib and you get a link
error?

What bothers me is that this moves in the direction of having to
handle portability ourselves, effectively undoing the benefits
of gnulib.  It seems to me to walk in the opposite direction of
the ideal, which would be for the IPA to also use gnulib.
It doesn't use gnulib today, because the IPA is a shared library,
so we'd need to link with a build of gnulib built with -fPIC.

The UNDO_GNULIB_CFLAGS stuff at least gives us normalized headers
between gdb / gdbserver / IPA, which for simple header portability
fixes and defines seems good enough, though not ideal, of course.

Thanks,
Pedro Alves

Re: [PATCH] Simplify the IPA parts of the gdbserver Makefile

[Christian Biesinger via gdb-patches]

On Tue, Nov 26, 2019 at 2:56 PM Pedro Alves <palves@redhat.com> wrote:
>
> On 11/26/19 8:32 PM, Christian Biesinger wrote:
> >> Currently the IPA uses gnulib headers, but not the function
> >> replacements.  What's the end goal you're after?  Why is this
> >> an improvement?
> >
> > Right. The end goal is
> > https://sourceware.org/ml/gdb-patches/2019-11/msg00922.html -- I want
> > to be able to call safe_strerror from more places, which means the
> > implementation of safe_strerror needs to be able to call glibc's
> > strerror_r. I suppose I could try adding it to UNDO_GNULIB_CFLAGS,
> > maybe, but it seemed less confusing to change it like this.
> So currently strerror_r is replaced by gnulib and you get a link
> error?

Yes, exactly.

> What bothers me is that this moves in the direction of having to
> handle portability ourselves, effectively undoing the benefits
> of gnulib.  It seems to me to walk in the opposite direction of
> the ideal, which would be for the IPA to also use gnulib.
> It doesn't use gnulib today, because the IPA is a shared library,
> so we'd need to link with a build of gnulib built with -fPIC.

Oh, is that the only reason? That seems like it should be fixable
reasonably easily, just build gdbserver's gnulib with -fPIC. And if we
move to a single Gnulib build we can just build all of it with -fPIC,
I would think, not much of a downside. I thought there may be codesize
concerns.

> The UNDO_GNULIB_CFLAGS stuff at least gives us normalized headers
> between gdb / gdbserver / IPA, which for simple header portability
> fixes and defines seems good enough, though not ideal, of course.

OK, I'll update that other patch to use that.

Do you think https://sourceware.org/ml/gdb-patches/2019-11/msg00908.html
is still worth having or should I withdraw that too?

Christian

Re: [PATCH] Simplify the IPA parts of the gdbserver Makefile

[Pedro Alves]

On 11/26/19 9:05 PM, Christian Biesinger wrote:
> On Tue, Nov 26, 2019 at 2:56 PM Pedro Alves <palves@redhat.com> wrote:
>>

>> What bothers me is that this moves in the direction of having to
>> handle portability ourselves, effectively undoing the benefits
>> of gnulib.  It seems to me to walk in the opposite direction of
>> the ideal, which would be for the IPA to also use gnulib.
>> It doesn't use gnulib today, because the IPA is a shared library,
>> so we'd need to link with a build of gnulib built with -fPIC.
> 
> Oh, is that the only reason? That seems like it should be fixable
> reasonably easily, just build gdbserver's gnulib with -fPIC. And if we
> move to a single Gnulib build we can just build all of it with -fPIC,
> I would think, not much of a downside. I thought there may be codesize
> concerns.

Yeah, code size concerns went out the window when the IPA started
using std::vector/std::string along with gdbserver...

> 
>> The UNDO_GNULIB_CFLAGS stuff at least gives us normalized headers
>> between gdb / gdbserver / IPA, which for simple header portability
>> fixes and defines seems good enough, though not ideal, of course.
> 
> OK, I'll update that other patch to use that.
> 
> Do you think https://sourceware.org/ml/gdb-patches/2019-11/msg00908.html
> is still worth having or should I withdraw that too?
That seems still worth it for being more direct to the point / precise.

Thanks,
Pedro Alves