Change in binutils-gdb[master]: Create xml-builtin.h to declare xml_builtins

Change in binutils-gdb[master]: Create xml-builtin.h to declare xml_builtins

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25
......................................................................


Patch Set 1:

Hi Christian,

I'm using this change as a test for email notifications formatting.


-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I806ef0851c43ead90b545a11794e41f5e5178436
Gerrit-Change-Number: 25
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Mon, 14 Oct 2019 23:10:40 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in binutils-gdb[master]: Create xml-builtin.h to declare xml_builtins

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25
......................................................................


Patch Set 1:

(1 comment)

Thanks for the patch.

I'm not totally sure that the new .h belongs in gdb.  On the one hand, I think we want to try to keep gdb and gdbserver separate to some extent, and only share via gdbsupport.  On the other hand, I broke that rule already with alloc.c.

I did find a nit though.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25/1//COMMIT_MSG@22 
PS1, Line 22: 	* features/feature_to_c.sh: Add an include for xml-buoltin.h
Typo, should be "xml-builtin.h"



-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

[review] Create xml-builtin.h to declare xml_builtins

[Christian Biesinger (Code Review)]

Christian Biesinger has uploaded a new patch set version (#2).

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25
......................................................................

Create xml-builtin.h to declare xml_builtins

xml-builtin.c only has character arrays and no dependencies, so this
creates a simple header file for that purpose so that gdbserver
can include that instead of re-declaring xml_builtin.

Despite the name, feature_to_c.sh is already specific to xml_builtins
(it hardcodes the variable name), so making it always output the
include for xml-builtin.h seems fine.

gdb/ChangeLog:

2019-10-09  Christian Biesinger  <cbiesinger@google.com>

	* Makefile.in: Add xml-builtin.h.
	* features/feature_to_c.sh: Add an include for xml-builtin.h
	to ensure that the compiler checks that the types match.
	* xml-builtin.h: New file.
	* xml-support.c (fetch_xml_builtin): Add missing const.
	* xml-support.h: Remove declaration of xml_builtins.

gdb/gdbserver/ChangeLog:

2019-10-09  Christian Biesinger  <cbiesinger@google.com>

	* server.c: Include xml-builtin.h.
	(get_xml_features): Don't declare xml_builtins here.

Change-Id: I806ef0851c43ead90b545a11794e41f5e5178436
---
M gdb/Makefile.in
M gdb/features/feature_to_c.sh
M gdb/gdbserver/server.c
A gdb/xml-builtin.h
M gdb/xml-support.c
M gdb/xml-support.h
6 files changed, 36 insertions(+), 7 deletions(-)



diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 36650ad..04a5cdb 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1426,6 +1426,7 @@
 	x86-linux-nat.h \
 	x86-nat.h \
 	xcoffread.h \
+	xml-builtin.h \
 	xml-support.h \
 	xml-syscall.h \
 	xml-tdesc.h \
diff --git a/gdb/features/feature_to_c.sh b/gdb/features/feature_to_c.sh
index 2e7e0c7..ae83774 100755
--- a/gdb/features/feature_to_c.sh
+++ b/gdb/features/feature_to_c.sh
@@ -32,6 +32,8 @@
   exit 1
 fi
 
+echo '#include "xml-builtin.h"' >> $output
+
 for input; do
   arrayname=xml_feature_`echo $input | sed 's,.*/,,; s/[-.]/_/g'`
 
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 0bfff04..25a2be8 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -41,6 +41,9 @@
 #include "gdbsupport/environ.h"
 #include "filenames.h"
 #include "gdbsupport/pathstuff.h"
+#ifdef USE_XML
+#include "xml-builtin.h"
+#endif
 
 #include "gdbsupport/selftest.h"
 #include "gdbsupport/scope-exit.h"
@@ -920,7 +923,6 @@
 
 #ifdef USE_XML
   {
-    extern const char *const xml_builtin[][2];
     int i;
 
     /* Look for the annex.  */
diff --git a/gdb/xml-builtin.h b/gdb/xml-builtin.h
new file mode 100644
index 0000000..972417d
--- /dev/null
+++ b/gdb/xml-builtin.h
@@ -0,0 +1,28 @@
+/* Header file for builtin XML files.
+
+   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/>.  */
+
+#ifndef XML_BUILTIN_H
+#define XML_BUILTIN_H
+
+/* The text of compiled-in XML documents, from xml-builtin.c
+   (generated).  */
+
+extern const char *const xml_builtin[][2];
+
+#endif /* XML_BUILTIN_H */
diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index eaf99ef..915be76 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 #include "gdbcmd.h"
+#include "xml-builtin.h"
 #include "xml-support.h"
 #include "gdbsupport/filestuff.h"
 #include "safe-ctype.h"
@@ -919,7 +920,7 @@
 const char *
 fetch_xml_builtin (const char *filename)
 {
-  const char *(*p)[2];
+  const char *const (*p)[2];
 
   for (p = xml_builtin; (*p)[0]; p++)
     if (strcmp ((*p)[0], filename) == 0)
diff --git a/gdb/xml-support.h b/gdb/xml-support.h
index 7ceb938..aaa829d 100644
--- a/gdb/xml-support.h
+++ b/gdb/xml-support.h
@@ -45,11 +45,6 @@
 				  gdb_byte *readbuf, const gdb_byte *writebuf,
 				  ULONGEST offset, LONGEST len);
 
-/* The text of compiled-in XML documents, from xml-builtin.c
-   (generated).  */
-
-extern const char *xml_builtin[][2];
-
 /* Support for XInclude.  */
 
 /* Callback to fetch a new XML file, based on the provided HREF.  */

[review] Create xml-builtin.h to declare xml_builtins

[Christian Biesinger (Code Review)]

Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25
......................................................................


Patch Set 2:

> Patch Set 1:
> 
> (1 comment)
> 
> Thanks for the patch.
> 
> I'm not totally sure that the new .h belongs in gdb.  On the one hand, I think we want to try to keep gdb and gdbserver separate to some extent, and only share via gdbsupport.  On the other hand, I broke that rule already with alloc.c.

Well xml-builtin.c is created in gdb/ and compiled by both... Do you want me to move both to gdbsupport?

> I did find a nit though.

Thanks, fixed.

[review] Create xml-builtin.h to declare xml_builtins

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25
......................................................................


Patch Set 2: Code-Review+2

> Well xml-builtin.c is created in gdb/ and compiled by both... Do you want me to move both to gdbsupport?

I don't see that in my build.  I looked for xml-builtin.o in gdbserver,
and I dug through gdbserver/Makefile.in a bit.

I do see xml-builtin-generated in the Makefile, but that seems to be
a second copy, built locally.  It's not made in my build for whatever reason.

Anyway, no, you don't have to change anything.  Thanks for doing this.

[review] Create xml-builtin.h to declare xml_builtins

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has uploaded a new patch set version (#3) to the change originally created by Christian Biesinger.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25
......................................................................

Create xml-builtin.h to declare xml_builtins

xml-builtin.c only has character arrays and no dependencies, so this
creates a simple header file for that purpose so that gdbserver
can include that instead of re-declaring xml_builtin.

Despite the name, feature_to_c.sh is already specific to xml_builtins
(it hardcodes the variable name), so making it always output the
include for xml-builtin.h seems fine.

gdb/ChangeLog:

2019-10-16  Christian Biesinger  <cbiesinger@google.com>

	* Makefile.in: Add xml-builtin.h.
	* features/feature_to_c.sh: Add an include for xml-builtin.h
	to ensure that the compiler checks that the types match.
	* xml-builtin.h: New file.
	* xml-support.c (fetch_xml_builtin): Add missing const.
	* xml-support.h: Remove declaration of xml_builtins.

gdb/gdbserver/ChangeLog:

2019-10-16  Christian Biesinger  <cbiesinger@google.com>

	* server.c: Include xml-builtin.h.
	(get_xml_features): Don't declare xml_builtins here.

Change-Id: I806ef0851c43ead90b545a11794e41f5e5178436
---
M gdb/ChangeLog
M gdb/Makefile.in
M gdb/features/feature_to_c.sh
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/server.c
A gdb/xml-builtin.h
M gdb/xml-support.c
M gdb/xml-support.h
8 files changed, 50 insertions(+), 7 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8748257..e79f449 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2019-10-16  Christian Biesinger  <cbiesinger@google.com>
+
+	* Makefile.in: Add xml-builtin.h.
+	* features/feature_to_c.sh: Add an include for xml-builtin.h
+	to ensure that the compiler checks that the types match.
+	* xml-builtin.h: New file.
+	* xml-support.c (fetch_xml_builtin): Add missing const.
+	* xml-support.h: Remove declaration of xml_builtins.
+
 2019-10-16  Tom de Vries  <tdevries@suse.de>
 
 	PR tdep/25096
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index af4b14d..8fec099 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1425,6 +1425,7 @@
 	x86-linux-nat.h \
 	x86-nat.h \
 	xcoffread.h \
+	xml-builtin.h \
 	xml-support.h \
 	xml-syscall.h \
 	xml-tdesc.h \
diff --git a/gdb/features/feature_to_c.sh b/gdb/features/feature_to_c.sh
index 2e7e0c7..ae83774 100755
--- a/gdb/features/feature_to_c.sh
+++ b/gdb/features/feature_to_c.sh
@@ -32,6 +32,8 @@
   exit 1
 fi
 
+echo '#include "xml-builtin.h"' >> $output
+
 for input; do
   arrayname=xml_feature_`echo $input | sed 's,.*/,,; s/[-.]/_/g'`
 
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 801a66c..b2213cf 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@
+2019-10-16  Christian Biesinger  <cbiesinger@google.com>
+
+	* server.c: Include xml-builtin.h.
+	(get_xml_features): Don't declare xml_builtins here.
+
 2019-10-15  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* Makefile.in: Remove references to vec-ipa.o.
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 0bfff04..25a2be8 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -41,6 +41,9 @@
 #include "gdbsupport/environ.h"
 #include "filenames.h"
 #include "gdbsupport/pathstuff.h"
+#ifdef USE_XML
+#include "xml-builtin.h"
+#endif
 
 #include "gdbsupport/selftest.h"
 #include "gdbsupport/scope-exit.h"
@@ -920,7 +923,6 @@
 
 #ifdef USE_XML
   {
-    extern const char *const xml_builtin[][2];
     int i;
 
     /* Look for the annex.  */
diff --git a/gdb/xml-builtin.h b/gdb/xml-builtin.h
new file mode 100644
index 0000000..972417d
--- /dev/null
+++ b/gdb/xml-builtin.h
@@ -0,0 +1,28 @@
+/* Header file for builtin XML files.
+
+   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/>.  */
+
+#ifndef XML_BUILTIN_H
+#define XML_BUILTIN_H
+
+/* The text of compiled-in XML documents, from xml-builtin.c
+   (generated).  */
+
+extern const char *const xml_builtin[][2];
+
+#endif /* XML_BUILTIN_H */
diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index eaf99ef..915be76 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 #include "gdbcmd.h"
+#include "xml-builtin.h"
 #include "xml-support.h"
 #include "gdbsupport/filestuff.h"
 #include "safe-ctype.h"
@@ -919,7 +920,7 @@
 const char *
 fetch_xml_builtin (const char *filename)
 {
-  const char *(*p)[2];
+  const char *const (*p)[2];
 
   for (p = xml_builtin; (*p)[0]; p++)
     if (strcmp ((*p)[0], filename) == 0)
diff --git a/gdb/xml-support.h b/gdb/xml-support.h
index 5012767..59b4467 100644
--- a/gdb/xml-support.h
+++ b/gdb/xml-support.h
@@ -44,11 +44,6 @@
 				  gdb_byte *readbuf, const gdb_byte *writebuf,
 				  ULONGEST offset, LONGEST len);
 
-/* The text of compiled-in XML documents, from xml-builtin.c
-   (generated).  */
-
-extern const char *xml_builtin[][2];
-
 /* Support for XInclude.  */
 
 /* Callback to fetch a new XML file, based on the provided HREF.  */

[review] Create xml-builtin.h to declare xml_builtins

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25
......................................................................

Create xml-builtin.h to declare xml_builtins

xml-builtin.c only has character arrays and no dependencies, so this
creates a simple header file for that purpose so that gdbserver
can include that instead of re-declaring xml_builtin.

Despite the name, feature_to_c.sh is already specific to xml_builtins
(it hardcodes the variable name), so making it always output the
include for xml-builtin.h seems fine.

gdb/ChangeLog:

2019-10-16  Christian Biesinger  <cbiesinger@google.com>

	* Makefile.in: Add xml-builtin.h.
	* features/feature_to_c.sh: Add an include for xml-builtin.h
	to ensure that the compiler checks that the types match.
	* xml-builtin.h: New file.
	* xml-support.c (fetch_xml_builtin): Add missing const.
	* xml-support.h: Remove declaration of xml_builtins.

gdb/gdbserver/ChangeLog:

2019-10-16  Christian Biesinger  <cbiesinger@google.com>

	* server.c: Include xml-builtin.h.
	(get_xml_features): Don't declare xml_builtins here.

Change-Id: I806ef0851c43ead90b545a11794e41f5e5178436
---
M gdb/ChangeLog
M gdb/Makefile.in
M gdb/features/feature_to_c.sh
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/server.c
A gdb/xml-builtin.h
M gdb/xml-support.c
M gdb/xml-support.h
8 files changed, 50 insertions(+), 7 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8748257..e79f449 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2019-10-16  Christian Biesinger  <cbiesinger@google.com>
+
+	* Makefile.in: Add xml-builtin.h.
+	* features/feature_to_c.sh: Add an include for xml-builtin.h
+	to ensure that the compiler checks that the types match.
+	* xml-builtin.h: New file.
+	* xml-support.c (fetch_xml_builtin): Add missing const.
+	* xml-support.h: Remove declaration of xml_builtins.
+
 2019-10-16  Tom de Vries  <tdevries@suse.de>
 
 	PR tdep/25096
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index af4b14d..8fec099 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1425,6 +1425,7 @@
 	x86-linux-nat.h \
 	x86-nat.h \
 	xcoffread.h \
+	xml-builtin.h \
 	xml-support.h \
 	xml-syscall.h \
 	xml-tdesc.h \
diff --git a/gdb/features/feature_to_c.sh b/gdb/features/feature_to_c.sh
index 2e7e0c7..ae83774 100755
--- a/gdb/features/feature_to_c.sh
+++ b/gdb/features/feature_to_c.sh
@@ -32,6 +32,8 @@
   exit 1
 fi
 
+echo '#include "xml-builtin.h"' >> $output
+
 for input; do
   arrayname=xml_feature_`echo $input | sed 's,.*/,,; s/[-.]/_/g'`
 
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 801a66c..b2213cf 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@
+2019-10-16  Christian Biesinger  <cbiesinger@google.com>
+
+	* server.c: Include xml-builtin.h.
+	(get_xml_features): Don't declare xml_builtins here.
+
 2019-10-15  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* Makefile.in: Remove references to vec-ipa.o.
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 0bfff04..25a2be8 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -41,6 +41,9 @@
 #include "gdbsupport/environ.h"
 #include "filenames.h"
 #include "gdbsupport/pathstuff.h"
+#ifdef USE_XML
+#include "xml-builtin.h"
+#endif
 
 #include "gdbsupport/selftest.h"
 #include "gdbsupport/scope-exit.h"
@@ -920,7 +923,6 @@
 
 #ifdef USE_XML
   {
-    extern const char *const xml_builtin[][2];
     int i;
 
     /* Look for the annex.  */
diff --git a/gdb/xml-builtin.h b/gdb/xml-builtin.h
new file mode 100644
index 0000000..972417d
--- /dev/null
+++ b/gdb/xml-builtin.h
@@ -0,0 +1,28 @@
+/* Header file for builtin XML files.
+
+   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/>.  */
+
+#ifndef XML_BUILTIN_H
+#define XML_BUILTIN_H
+
+/* The text of compiled-in XML documents, from xml-builtin.c
+   (generated).  */
+
+extern const char *const xml_builtin[][2];
+
+#endif /* XML_BUILTIN_H */
diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index eaf99ef..915be76 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 #include "gdbcmd.h"
+#include "xml-builtin.h"
 #include "xml-support.h"
 #include "gdbsupport/filestuff.h"
 #include "safe-ctype.h"
@@ -919,7 +920,7 @@
 const char *
 fetch_xml_builtin (const char *filename)
 {
-  const char *(*p)[2];
+  const char *const (*p)[2];
 
   for (p = xml_builtin; (*p)[0]; p++)
     if (strcmp ((*p)[0], filename) == 0)
diff --git a/gdb/xml-support.h b/gdb/xml-support.h
index 5012767..59b4467 100644
--- a/gdb/xml-support.h
+++ b/gdb/xml-support.h
@@ -44,11 +44,6 @@
 				  gdb_byte *readbuf, const gdb_byte *writebuf,
 				  ULONGEST offset, LONGEST len);
 
-/* The text of compiled-in XML documents, from xml-builtin.c
-   (generated).  */
-
-extern const char *xml_builtin[][2];
-
 /* Support for XInclude.  */
 
 /* Callback to fetch a new XML file, based on the provided HREF.  */