[RFC,2/2] Move gdb's xmalloc and friends to new file

Message ID 20190530213046.20542-3-tom@tromey.com
State New
Headers show
Series
  • Let's discuss moving gdbserver to top-level
Related show

Commit Message

Tom Tromey May 30, 2019, 9:30 p.m.
When "common" becomes a library, linking will cause a symbol clash,
because "xmalloc" and some related symbols are defined in that
library, libiberty, and readline.

To work around this problem, this patch moves the clashing symbols to
a new file, which is then compiled separately for both gdb and
gdbserver.

gdb/ChangeLog
2019-05-30  Tom Tromey  <tom@tromey.com>

	* common/common-utils.c (xmalloc, xrealloc, xcalloc)
	(xmalloc_failed): Move to alloc.c.
	* alloc.c: New file.
	* Makefile.in (COMMON_SFILES): Add alloc.c.

gdb/gdbserver/ChangeLog
2019-05-30  Tom Tromey  <tom@tromey.com>

	* Makefile.in (SFILES): Add alloc.c.
	(OBS): Add alloc.o.
	(IPA_OBJS): Add alloc-ipa.o.
	(alloc-ipa.o): New target.
	(%.o: ../%.c): New pattern rule.
---
 gdb/ChangeLog             |  7 +++
 gdb/Makefile.in           |  1 +
 gdb/alloc.c               | 95 +++++++++++++++++++++++++++++++++++++++
 gdb/common/common-utils.c | 72 -----------------------------
 gdb/gdbserver/ChangeLog   |  8 ++++
 gdb/gdbserver/Makefile.in | 11 +++++
 6 files changed, 122 insertions(+), 72 deletions(-)
 create mode 100644 gdb/alloc.c

-- 
2.17.2

Comments

Simon Marchi June 3, 2019, 3:03 p.m. | #1
On 2019-05-30 5:30 p.m., Tom Tromey wrote:
> When "common" becomes a library, linking will cause a symbol clash,

> because "xmalloc" and some related symbols are defined in that

> library, libiberty, and readline.

> 

> To work around this problem, this patch moves the clashing symbols to

> a new file, which is then compiled separately for both gdb and

> gdbserver.


Hmm how does this work currently?  We have an xmalloc symbols both in
common/common-utils.o and ../libiberty/libiberty.a, why doesn't it clash?

Simon
Tom Tromey June 3, 2019, 4:33 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> On 2019-05-30 5:30 p.m., Tom Tromey wrote:
>> When "common" becomes a library, linking will cause a symbol clash,

>> because "xmalloc" and some related symbols are defined in that

>> library, libiberty, and readline.

>> 

>> To work around this problem, this patch moves the clashing symbols to

>> a new file, which is then compiled separately for both gdb and

>> gdbserver.


Simon> Hmm how does this work currently?  We have an xmalloc symbols both in
Simon> common/common-utils.o and ../libiberty/libiberty.a, why doesn't it clash?

If a symbol comes from a .o then it overrides the symbols coming from
libraries.  The error only occurs if the symbol is only provided by
multiple libraries.  The former is the case today, because we don't make
a library from common/.

Tom
Pedro Alves June 5, 2019, 9:40 a.m. | #3
On 5/30/19 10:30 PM, Tom Tromey wrote:
> When "common" becomes a library, linking will cause a symbol clash,

> because "xmalloc" and some related symbols are defined in that

> library, libiberty, and readline.

> 

> To work around this problem, this patch moves the clashing symbols to

> a new file, which is then compiled separately for both gdb and

> gdbserver.

> 

> gdb/ChangeLog

> 2019-05-30  Tom Tromey  <tom@tromey.com>

> 

> 	* common/common-utils.c (xmalloc, xrealloc, xcalloc)

> 	(xmalloc_failed): Move to alloc.c.

> 	* alloc.c: New file.

> 	* Makefile.in (COMMON_SFILES): Add alloc.c.

> 

> gdb/gdbserver/ChangeLog

> 2019-05-30  Tom Tromey  <tom@tromey.com>

> 

> 	* Makefile.in (SFILES): Add alloc.c.

> 	(OBS): Add alloc.o.

> 	(IPA_OBJS): Add alloc-ipa.o.

> 	(alloc-ipa.o): New target.

> 	(%.o: ../%.c): New pattern rule.


This will be the first case of gdbserver building a file
from gdb/ .  I suppose we could preserve the gdb/common/
directory for such files.  But I guess moving it out of the
way until gdb/common/ moves to top level helps.

> +++ b/gdb/alloc.c

> @@ -0,0 +1,95 @@

> +/* Shared allocation functions for GDB, the GNU debugger.

> +

> +   Copyright (C) 2019 Free Software Foundation, Inc.


The file is new, but the contents aren't, and it's the contents
that matter wrt to copyright years.

There should be some comment here about why this is in a separate file
instead of living in the common library, to help people that read
the code from the tree without having this commit in context.

> +

> +#include "common/common-defs.h"


There should also be a comment explaining why this includes common-defs.h
instead of defs.h.

> +#include "libiberty.h"

> +#include <cstdlib>


Including <cstdlib> looks strange, given common-defs.h includes stdlib.h.
Why did you need this?

> +#include "common/errors.h"

> +

> +/* The xmalloc() (libiberty.h) family of memory management routines.

> +


Thanks,
Pedro Alves
Tom Tromey June 5, 2019, 10:33 p.m. | #4
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> This will be the first case of gdbserver building a file
Pedro> from gdb/ .  I suppose we could preserve the gdb/common/
Pedro> directory for such files.  But I guess moving it out of the
Pedro> way until gdb/common/ moves to top level helps.

Yeah.  We can shuffle it around again later if we want.

>> +   Copyright (C) 2019 Free Software Foundation, Inc.


Pedro> The file is new, but the contents aren't, and it's the contents
Pedro> that matter wrt to copyright years.

Fixed.

Pedro> There should be some comment here about why this is in a separate file
Pedro> instead of living in the common library, to help people that read
Pedro> the code from the tree without having this commit in context.

Also fixed.

>> +#include "common/common-defs.h"


Pedro> There should also be a comment explaining why this includes common-defs.h
Pedro> instead of defs.h.

Fixed.

>> +#include "libiberty.h"

>> +#include <cstdlib>


Pedro> Including <cstdlib> looks strange, given common-defs.h includes stdlib.h.
Pedro> Why did you need this?

Removed.

I've appended the new file.

I'll probably push these patches sometime soon.

Tom

/* Shared allocation functions for GDB, the GNU debugger.

   Copyright (C) 1986-2019 Free Software Foundation, Inc.

   This file is part of GDB.

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 3 of the License, or
   (at your option) any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

/* This file is unusual.

   Because both libiberty and readline define xmalloc and friends, the
   functions in this file can't appear in a library -- that will cause
   link errors.

   And, because we want to turn the common code into a library, this
   file can't live there.

   So, it lives in gdb and is built separately by gdb and gdbserver.
   Please be aware of this when modifying it.

   This also explains why this file includes common-defs.h and not
   defs.h or server.h -- we'd prefer to avoid depending on the
   GDBSERVER define when possible, and for this file it seemed
   simple to do so.  */

#include "common/common-defs.h"
#include "libiberty.h"
#include "common/errors.h"

/* The xmalloc() (libiberty.h) family of memory management routines.

   These are like the ISO-C malloc() family except that they implement
   consistent semantics and guard against typical memory management
   problems.  */

/* NOTE: These are declared using PTR to ensure consistency with
   "libiberty.h".  xfree() is GDB local.  */

PTR                            /* ARI: PTR */
xmalloc (size_t size)
{
  void *val;

  /* See libiberty/xmalloc.c.  This function need's to match that's
     semantics.  It never returns NULL.  */
  if (size == 0)
    size = 1;

  val = malloc (size);         /* ARI: malloc */
  if (val == NULL)
    malloc_failure (size);

  return val;
}

PTR                              /* ARI: PTR */
xrealloc (PTR ptr, size_t size)          /* ARI: PTR */
{
  void *val;

  /* See libiberty/xmalloc.c.  This function need's to match that's
     semantics.  It never returns NULL.  */
  if (size == 0)
    size = 1;

  if (ptr != NULL)
    val = realloc (ptr, size);	/* ARI: realloc */
  else
    val = malloc (size);	        /* ARI: malloc */
  if (val == NULL)
    malloc_failure (size);

  return val;
}

PTR                            /* ARI: PTR */
xcalloc (size_t number, size_t size)
{
  void *mem;

  /* See libiberty/xmalloc.c.  This function need's to match that's
     semantics.  It never returns NULL.  */
  if (number == 0 || size == 0)
    {
      number = 1;
      size = 1;
    }

  mem = calloc (number, size);      /* ARI: xcalloc */
  if (mem == NULL)
    malloc_failure (number * size);

  return mem;
}

void
xmalloc_failed (size_t size)
{
  malloc_failure (size);
}

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 0f495783600..15ec7a61b1c 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -924,6 +924,7 @@  COMMON_SFILES = \
 	ada-varobj.c \
 	addrmap.c \
 	agent.c \
+	alloc.c \
 	annotate.c \
 	arch-utils.c \
 	auto-load.c \
diff --git a/gdb/alloc.c b/gdb/alloc.c
new file mode 100644
index 00000000000..e6c9c215435
--- /dev/null
+++ b/gdb/alloc.c
@@ -0,0 +1,95 @@ 
+/* Shared allocation functions for GDB, the GNU debugger.
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "common/common-defs.h"
+#include "libiberty.h"
+#include <cstdlib>
+#include "common/errors.h"
+
+/* The xmalloc() (libiberty.h) family of memory management routines.
+
+   These are like the ISO-C malloc() family except that they implement
+   consistent semantics and guard against typical memory management
+   problems.  */
+
+/* NOTE: These are declared using PTR to ensure consistency with
+   "libiberty.h".  xfree() is GDB local.  */
+
+PTR                            /* ARI: PTR */
+xmalloc (size_t size)
+{
+  void *val;
+
+  /* See libiberty/xmalloc.c.  This function need's to match that's
+     semantics.  It never returns NULL.  */
+  if (size == 0)
+    size = 1;
+
+  val = malloc (size);         /* ARI: malloc */
+  if (val == NULL)
+    malloc_failure (size);
+
+  return val;
+}
+
+PTR                              /* ARI: PTR */
+xrealloc (PTR ptr, size_t size)          /* ARI: PTR */
+{
+  void *val;
+
+  /* See libiberty/xmalloc.c.  This function need's to match that's
+     semantics.  It never returns NULL.  */
+  if (size == 0)
+    size = 1;
+
+  if (ptr != NULL)
+    val = realloc (ptr, size);	/* ARI: realloc */
+  else
+    val = malloc (size);	        /* ARI: malloc */
+  if (val == NULL)
+    malloc_failure (size);
+
+  return val;
+}
+
+PTR                            /* ARI: PTR */
+xcalloc (size_t number, size_t size)
+{
+  void *mem;
+
+  /* See libiberty/xmalloc.c.  This function need's to match that's
+     semantics.  It never returns NULL.  */
+  if (number == 0 || size == 0)
+    {
+      number = 1;
+      size = 1;
+    }
+
+  mem = calloc (number, size);      /* ARI: xcalloc */
+  if (mem == NULL)
+    malloc_failure (number * size);
+
+  return mem;
+}
+
+void
+xmalloc_failed (size_t size)
+{
+  malloc_failure (size);
+}
diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 74ca93810c7..dd839a0d4d1 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -22,84 +22,12 @@ 
 #include "host-defs.h"
 #include <ctype.h>
 
-/* The xmalloc() (libiberty.h) family of memory management routines.
-
-   These are like the ISO-C malloc() family except that they implement
-   consistent semantics and guard against typical memory management
-   problems.  */
-
-/* NOTE: These are declared using PTR to ensure consistency with
-   "libiberty.h".  xfree() is GDB local.  */
-
-PTR                            /* ARI: PTR */
-xmalloc (size_t size)
-{
-  void *val;
-
-  /* See libiberty/xmalloc.c.  This function need's to match that's
-     semantics.  It never returns NULL.  */
-  if (size == 0)
-    size = 1;
-
-  val = malloc (size);         /* ARI: malloc */
-  if (val == NULL)
-    malloc_failure (size);
-
-  return val;
-}
-
-PTR                              /* ARI: PTR */
-xrealloc (PTR ptr, size_t size)          /* ARI: PTR */
-{
-  void *val;
-
-  /* See libiberty/xmalloc.c.  This function need's to match that's
-     semantics.  It never returns NULL.  */
-  if (size == 0)
-    size = 1;
-
-  if (ptr != NULL)
-    val = realloc (ptr, size);	/* ARI: realloc */
-  else
-    val = malloc (size);	        /* ARI: malloc */
-  if (val == NULL)
-    malloc_failure (size);
-
-  return val;
-}
-
-PTR                            /* ARI: PTR */           
-xcalloc (size_t number, size_t size)
-{
-  void *mem;
-
-  /* See libiberty/xmalloc.c.  This function need's to match that's
-     semantics.  It never returns NULL.  */
-  if (number == 0 || size == 0)
-    {
-      number = 1;
-      size = 1;
-    }
-
-  mem = calloc (number, size);      /* ARI: xcalloc */
-  if (mem == NULL)
-    malloc_failure (number * size);
-
-  return mem;
-}
-
 void *
 xzalloc (size_t size)
 {
   return xcalloc (1, size);
 }
 
-void
-xmalloc_failed (size_t size)
-{
-  malloc_failure (size);
-}
-
 /* Like asprintf/vasprintf but get an internal_error if the call
    fails. */
 
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index f5fc55034ee..a0793e657c5 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -197,6 +197,7 @@  SFILES = \
 	$(srcdir)/arch/arm-get-next-pcs.c \
 	$(srcdir)/arch/arm-linux.c \
 	$(srcdir)/arch/ppc-linux-common.c \
+	$(srcdir)/../alloc.c \
 	$(srcdir)/common/btrace-common.c \
 	$(srcdir)/common/buffer.c \
 	$(srcdir)/common/cleanups.c \
@@ -238,6 +239,7 @@  SOURCES = $(SFILES)
 TAGFILES = $(SOURCES) ${HFILES} ${ALLPARAM} ${POSSLIBS}
 
 OBS = \
+	alloc.o \
 	ax.o \
 	common/agent.o \
 	common/btrace-common.o \
@@ -413,6 +415,7 @@  gdbreplay$(EXEEXT): $(sort $(GDBREPLAY_OBS)) $(LIBGNU) $(LIBIBERTY)
 		$(LIBIBERTY)
 
 IPA_OBJS = \
+	alloc-ipa.o \
 	ax-ipa.o \
 	common/common-utils-ipa.o \
 	common/errors-ipa.o \
@@ -568,6 +571,10 @@  ax.o: ax.c
 	$(COMPILE) $(WARN_CFLAGS_NO_FORMAT) $<
 	$(POSTCOMPILE)
 
+alloc-ipa.o: ../alloc.c
+	$(IPAGENT_COMPILE) $(WARN_CFLAGS_NO_FORMAT) $<
+	$(POSTCOMPILE)
+
 # Rules for objects that go in the in-process agent.
 
 arch/%-ipa.o: ../arch/%.c
@@ -623,6 +630,10 @@  common/%.o: ../common/%.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
 
+%.o: ../%.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
+
 # Rules for register format descriptions.  Suffix destination files with
 # -generated to identify and clean them easily.