[PATCH] Always include defs.h first.

[PATCH] Always include defs.h first.

[Pedro Alves]

defs.h should always be the first included header in a .c file.  In
turn, this means that a foo.h header should not need to include
defs.h, as the .c file always includes defs.h before including foo.h.

I did

$ grep "\"defs\.h" *.c -B 3 | grep include | grep -v "\"defs"

and

$ grep "\"defs\.h" *.h

and hopefully caught all cases.

Applied.

2012-11-07  Pedro Alves  <palves@redhat.com>

	* arm-tdep.c: Make defs.h be the first include.
	* coff-pe-read.c: Ditto.
	* gnu-nat.c: Ditto.
	* go32-nat.c: Ditto.
	* i386-nat.c: Ditto.
	* ppcnbsd-nat.c: Ditto.
	* ada-varobj.h: Don't include defs.h.
	* i386-darwin-tdep.h: Ditto.
	* i386-nat.h: Ditto.
---
 gdb/ada-varobj.h       |    1 -
 gdb/arm-tdep.c         |    3 ++-
 gdb/coff-pe-read.c     |    3 ++-
 gdb/gnu-nat.c          |    3 ++-
 gdb/go32-nat.c         |    3 ++-
 gdb/i386-darwin-tdep.h |    1 -
 gdb/i386-nat.c         |    2 +-
 gdb/i386-nat.h         |    2 --
 gdb/ppcnbsd-nat.c      |    3 ++-
 9 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/gdb/ada-varobj.h b/gdb/ada-varobj.h
index 2ef1a70..21f4cec 100644
--- a/gdb/ada-varobj.h
+++ b/gdb/ada-varobj.h
@@ -20,7 +20,6 @@
 #ifndef ADA_VAROBJ_H
 #define ADA_VAROBJ_H
 
-#include "defs.h"
 #include "varobj.h"
 
 struct value;
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 05a030a..fbc2479 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -18,9 +18,10 @@
    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 "defs.h"
+
 #include <ctype.h>		/* XXX for isupper ().  */
 
-#include "defs.h"
 #include "frame.h"
 #include "inferior.h"
 #include "gdbcmd.h"
diff --git a/gdb/coff-pe-read.c b/gdb/coff-pe-read.c
index 66c7c82..fd55170 100644
--- a/gdb/coff-pe-read.c
+++ b/gdb/coff-pe-read.c
@@ -21,9 +21,10 @@
 
    Contributed by Raoul M. Gough (RaoulGough@yahoo.co.uk).  */
 
+#include "defs.h"
+
 #include "coff-pe-read.h"
 
-#include "defs.h"
 #include "bfd.h"
 #include "gdbtypes.h"
 
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 2ca01e1..b34935e 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -22,6 +22,8 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "defs.h"
+
 #include <ctype.h>
 #include <errno.h>
 #include <limits.h>
@@ -52,7 +54,6 @@
 
 #include <portinfo.h>
 
-#include "defs.h"
 #include "inferior.h"
 #include "symtab.h"
 #include "value.h"
diff --git a/gdb/go32-nat.c b/gdb/go32-nat.c
index d848017..ef1da91 100644
--- a/gdb/go32-nat.c
+++ b/gdb/go32-nat.c
@@ -82,9 +82,10 @@
    GDB does not use those as of this writing, and will never need
    to.  */
 
+#include "defs.h"
+
 #include <fcntl.h>
 
-#include "defs.h"
 #include "i386-nat.h"
 #include "inferior.h"
 #include "gdbthread.h"
diff --git a/gdb/i386-darwin-tdep.h b/gdb/i386-darwin-tdep.h
index 8be9946..4732b2d 100644
--- a/gdb/i386-darwin-tdep.h
+++ b/gdb/i386-darwin-tdep.h
@@ -19,7 +19,6 @@
 #ifndef __I386_DARWIN_TDEP_H__
 #define __I386_DARWIN_TDEP_H__
 
-#include "defs.h"
 #include "frame.h"
 
 /* Mapping between the general-purpose registers in Darwin x86 thread_state
diff --git a/gdb/i386-nat.c b/gdb/i386-nat.c
index e7d9b4d..559bc29 100644
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -18,8 +18,8 @@
    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 "i386-nat.h"
 #include "defs.h"
+#include "i386-nat.h"
 #include "breakpoint.h"
 #include "command.h"
 #include "gdbcmd.h"
diff --git a/gdb/i386-nat.h b/gdb/i386-nat.h
index c06993c..be563ef 100644
--- a/gdb/i386-nat.h
+++ b/gdb/i386-nat.h
@@ -20,8 +20,6 @@
    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 "defs.h"
-
 #ifndef I386_NAT_H
 #define I386_NAT_H 1
 
diff --git a/gdb/ppcnbsd-nat.c b/gdb/ppcnbsd-nat.c
index 8f73283..2b7837d 100644
--- a/gdb/ppcnbsd-nat.c
+++ b/gdb/ppcnbsd-nat.c
@@ -19,13 +19,14 @@
    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 "defs.h"
+
 #include <sys/types.h>
 #include <sys/ptrace.h>
 #include <machine/reg.h>
 #include <machine/frame.h>
 #include <machine/pcb.h>
 
-#include "defs.h"
 #include "gdbcore.h"
 #include "inferior.h"
 #include "regcache.h"

Re: [PATCH] Always include defs.h first.

[Doug Evans]

On Wed, Nov 7, 2012 at 12:11 PM, Pedro Alves <palves@redhat.com> wrote:
> defs.h should always be the first included header in a .c file.  In
> turn, this means that a foo.h header should not need to include
> defs.h, as the .c file always includes defs.h before including foo.h.

fwiw, I like the idea of header files including what they themselves
need, and not assuming the includer will do it for them, even for
defs.h.
It's something I'd like to see gdb move towards, but I'm not in a hurry. :-)

Re: [PATCH] Always include defs.h first.

[Yao Qi]

On 11/08/2012 04:11 AM, Pedro Alves wrote:
> defs.h should always be the first included header in a .c file.  In

I agree on this point.

> turn, this means that a foo.h header should not need to include
> defs.h, as the .c file always includes defs.h before including foo.h.
>

I am afraid I can't agree.  If foo.h needs defs.h, it should include 
defs.h, rather than replying on foo.h's includer to include defs.h.

> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 05a030a..fbc2479 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -18,9 +18,10 @@
>      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 "defs.h"
> +
>   #include <ctype.h>		/* XXX for isupper ().  */
>
> -#include "defs.h"
>   #include "frame.h"
>   #include "inferior.h"
>   #include "gdbcmd.h"

What is the include order of "defs.h" and system headers, such as 
<ctype.h> and <stdio.h>?  When I learnt C programming some years ago, it 
was said system headers are included first, and then your own headers. 
This rule doesn't apply here?

-- 
Yao

Re: [PATCH] Always include defs.h first.

[Joel Brobecker]

> What is the include order of "defs.h" and system headers, such as
> <ctype.h> and <stdio.h>?  When I learnt C programming some years
> ago, it was said system headers are included first, and then your
> own headers. This rule doesn't apply here?

My understanding is that "defs.h" should always be included first.
It sets things up nicely for the rest of the includes (such as
including the various config.h files, which can have an effect on
how other system includes are expanded). Eg:

    /* Enable extensions on AIX 3, Interix.  */
    #ifndef _ALL_SOURCE
    # undef _ALL_SOURCE
    #endif

For the rest, I don't really know if there is a recommended order,
or not. I don't think we're completely consistent on that. I think
I've seen code that intermingles both.

-- 
Joel

Re: [PATCH] Always include defs.h first.

[Pedro Alves]

On 11/08/2012 01:34 AM, Doug Evans wrote:
> On Wed, Nov 7, 2012 at 12:11 PM, Pedro Alves <palves@redhat.com> wrote:
>> defs.h should always be the first included header in a .c file.  In
>> turn, this means that a foo.h header should not need to include
>> defs.h, as the .c file always includes defs.h before including foo.h.
> 
> fwiw, I like the idea of header files including what they themselves
> need, and not assuming the includer will do it for them, even for
> defs.h.
> It's something I'd like to see gdb move towards, but I'm not in a hurry. :-)

Sorry, I hadn't imagined the change could be disagreeable.  Otherwise, I'd
have waited.

I'm also of the camp that advocates that header files should include what
they themselves need.  OTOH, IMO, defs.h is special.

 - It's defs.h that includes config.h.  Including config.h in headers
   is bad practice.  Comparing to gcc, I'd say defs.h is currently a
   kind of a mix of system.h and coretypes.h, and whatnot.
 - It's needed practically _everywhere_.

Also:

$ ls gdb/*.h | wc -l
232

since not including defs.h in headers is the de facto standard on the code
base, where only 4 of those 232 had "#include defs.h" in them, and another
include was being introduced exactly because the corresponding .c file was in
error, I thought it best to just get rid of those 4 instances making the
codebase a little bit less inconsistent.

-- 
Pedro Alves

Re: [PATCH] Always include defs.h first.

[Pedro Alves]

On 11/08/2012 03:49 AM, Yao Qi wrote:

>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index 05a030a..fbc2479 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -18,9 +18,10 @@
>>      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 "defs.h"
>> +
>>   #include <ctype.h>        /* XXX for isupper ().  */
>>
>> -#include "defs.h"
>>   #include "frame.h"
>>   #include "inferior.h"
>>   #include "gdbcmd.h"
> 
> What is the include order of "defs.h" and system headers, such as <ctype.h> and <stdio.h>?  When I learnt C programming some years ago, it was said system headers are included first, and then your own headers. This rule doesn't apply here?

defs.h includes config.h and config.h must always be included before system headers.

This is probably "authoritatively" described somewhere in autoconf's docs, but
I can't find it now.  But see e.g., <http://inaugust.com/post/68>.

-- 
Pedro Alves

RE: [PATCH] Always include defs.h first.

[Pierre Muller]

  Hi Pedro,

  I think you missed the python subdirectory...
$ grep \"defs\.h\" */*.h
python/py-event.h:#include "defs.h"
python/py-events.h:#include "defs.h"

The C sources also do not all include defs.h ...


Pierre Muller

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : mercredi 7 novembre 2012 21:11
> À : gdb-patches@sourceware.org
> Objet : [PATCH] Always include defs.h first.
> 
> defs.h should always be the first included header in a .c file.  In
> turn, this means that a foo.h header should not need to include
> defs.h, as the .c file always includes defs.h before including foo.h.
> 
> I did
> 
> $ grep "\"defs\.h" *.c -B 3 | grep include | grep -v "\"defs"
> 
> and
> 
> $ grep "\"defs\.h" *.h
> 
> and hopefully caught all cases.
> 
> Applied.
> 
> 2012-11-07  Pedro Alves  <palves@redhat.com>
> 
> 	* arm-tdep.c: Make defs.h be the first include.
> 	* coff-pe-read.c: Ditto.
> 	* gnu-nat.c: Ditto.
> 	* go32-nat.c: Ditto.
> 	* i386-nat.c: Ditto.
> 	* ppcnbsd-nat.c: Ditto.
> 	* ada-varobj.h: Don't include defs.h.
> 	* i386-darwin-tdep.h: Ditto.
> 	* i386-nat.h: Ditto.
> ---
>  gdb/ada-varobj.h       |    1 -
>  gdb/arm-tdep.c         |    3 ++-
>  gdb/coff-pe-read.c     |    3 ++-
>  gdb/gnu-nat.c          |    3 ++-
>  gdb/go32-nat.c         |    3 ++-
>  gdb/i386-darwin-tdep.h |    1 -
>  gdb/i386-nat.c         |    2 +-
>  gdb/i386-nat.h         |    2 --
>  gdb/ppcnbsd-nat.c      |    3 ++-
>  9 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/ada-varobj.h b/gdb/ada-varobj.h
> index 2ef1a70..21f4cec 100644
> --- a/gdb/ada-varobj.h
> +++ b/gdb/ada-varobj.h
> @@ -20,7 +20,6 @@
>  #ifndef ADA_VAROBJ_H
>  #define ADA_VAROBJ_H
> 
> -#include "defs.h"
>  #include "varobj.h"
> 
>  struct value;
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 05a030a..fbc2479 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -18,9 +18,10 @@
>     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 "defs.h"
> +
>  #include <ctype.h>		/* XXX for isupper ().  */
> 
> -#include "defs.h"
>  #include "frame.h"
>  #include "inferior.h"
>  #include "gdbcmd.h"
> diff --git a/gdb/coff-pe-read.c b/gdb/coff-pe-read.c
> index 66c7c82..fd55170 100644
> --- a/gdb/coff-pe-read.c
> +++ b/gdb/coff-pe-read.c
> @@ -21,9 +21,10 @@
> 
>     Contributed by Raoul M. Gough (RaoulGough@yahoo.co.uk).  */
> 
> +#include "defs.h"
> +
>  #include "coff-pe-read.h"
> 
> -#include "defs.h"
>  #include "bfd.h"
>  #include "gdbtypes.h"
> 
> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
> index 2ca01e1..b34935e 100644
> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -22,6 +22,8 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
> 
> +#include "defs.h"
> +
>  #include <ctype.h>
>  #include <errno.h>
>  #include <limits.h>
> @@ -52,7 +54,6 @@
> 
>  #include <portinfo.h>
> 
> -#include "defs.h"
>  #include "inferior.h"
>  #include "symtab.h"
>  #include "value.h"
> diff --git a/gdb/go32-nat.c b/gdb/go32-nat.c
> index d848017..ef1da91 100644
> --- a/gdb/go32-nat.c
> +++ b/gdb/go32-nat.c
> @@ -82,9 +82,10 @@
>     GDB does not use those as of this writing, and will never need
>     to.  */
> 
> +#include "defs.h"
> +
>  #include <fcntl.h>
> 
> -#include "defs.h"
>  #include "i386-nat.h"
>  #include "inferior.h"
>  #include "gdbthread.h"
> diff --git a/gdb/i386-darwin-tdep.h b/gdb/i386-darwin-tdep.h
> index 8be9946..4732b2d 100644
> --- a/gdb/i386-darwin-tdep.h
> +++ b/gdb/i386-darwin-tdep.h
> @@ -19,7 +19,6 @@
>  #ifndef __I386_DARWIN_TDEP_H__
>  #define __I386_DARWIN_TDEP_H__
> 
> -#include "defs.h"
>  #include "frame.h"
> 
>  /* Mapping between the general-purpose registers in Darwin x86 thread_state
> diff --git a/gdb/i386-nat.c b/gdb/i386-nat.c
> index e7d9b4d..559bc29 100644
> --- a/gdb/i386-nat.c
> +++ b/gdb/i386-nat.c
> @@ -18,8 +18,8 @@
>     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 "i386-nat.h"
>  #include "defs.h"
> +#include "i386-nat.h"
>  #include "breakpoint.h"
>  #include "command.h"
>  #include "gdbcmd.h"
> diff --git a/gdb/i386-nat.h b/gdb/i386-nat.h
> index c06993c..be563ef 100644
> --- a/gdb/i386-nat.h
> +++ b/gdb/i386-nat.h
> @@ -20,8 +20,6 @@
>     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 "defs.h"
> -
>  #ifndef I386_NAT_H
>  #define I386_NAT_H 1
> 
> diff --git a/gdb/ppcnbsd-nat.c b/gdb/ppcnbsd-nat.c
> index 8f73283..2b7837d 100644
> --- a/gdb/ppcnbsd-nat.c
> +++ b/gdb/ppcnbsd-nat.c
> @@ -19,13 +19,14 @@
>     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 "defs.h"
> +
>  #include <sys/types.h>
>  #include <sys/ptrace.h>
>  #include <machine/reg.h>
>  #include <machine/frame.h>
>  #include <machine/pcb.h>
> 
> -#include "defs.h"
>  #include "gdbcore.h"
>  #include "inferior.h"
>  #include "regcache.h"

Re: [PATCH] Always include defs.h first.

[Pedro Alves]

On 11/08/2012 05:04 PM, Pierre Muller wrote:
>   Hi Pedro,
> 
>   I think you missed the python subdirectory...
> $ grep \"defs\.h\" */*.h
> python/py-event.h:#include "defs.h"
> python/py-events.h:#include "defs.h"

Indeed I have.

> The C sources also do not all include defs.h ...

Yeah.  They're getting it from those headers.  IMO, this makes it
easy to forget about it, and risk some change adding the inclusion
of a system header before including py-events.h, etc.  But since
we don't have consensus, I'll leave it at that.

-- 
Pedro Alves

Re: [PATCH] Always include defs.h first.

[Doug Evans]

On Thu, Nov 8, 2012 at 8:49 AM, Pedro Alves <palves@redhat.com> wrote:
> On 11/08/2012 01:34 AM, Doug Evans wrote:
>> On Wed, Nov 7, 2012 at 12:11 PM, Pedro Alves <palves@redhat.com> wrote:
>>> defs.h should always be the first included header in a .c file.  In
>>> turn, this means that a foo.h header should not need to include
>>> defs.h, as the .c file always includes defs.h before including foo.h.
>>
>> fwiw, I like the idea of header files including what they themselves
>> need, and not assuming the includer will do it for them, even for
>> defs.h.
>> It's something I'd like to see gdb move towards, but I'm not in a hurry. :-)
>
> Sorry, I hadn't imagined the change could be disagreeable.  Otherwise, I'd
> have waited.
>
> I'm also of the camp that advocates that header files should include what
> they themselves need.  OTOH, IMO, defs.h is special.
>
>  - It's defs.h that includes config.h.  Including config.h in headers
>    is bad practice.  Comparing to gcc, I'd say defs.h is currently a
>    kind of a mix of system.h and coretypes.h, and whatnot.
>  - It's needed practically _everywhere_.
>
> Also:
>
> $ ls gdb/*.h | wc -l
> 232
>
> since not including defs.h in headers is the de facto standard on the code
> base, where only 4 of those 232 had "#include defs.h" in them, and another
> include was being introduced exactly because the corresponding .c file was in
> error, I thought it best to just get rid of those 4 instances making the
> codebase a little bit less inconsistent.

Don't abort on my account - Consistency is Good.

Another thing to consider:
I'd like to move gdb to being more componentized (for lack of a better word).
i.e., made up of application independent pieces.
common/ is a minor step in this direction.
Note that gdbserver doesn't have defs.h and lots of files in common/ do the

#ifdef GDBSERVER
#include "server.h"
#else
#include "defs.h"
#endif

dance.

But again, don't abort on my account, Consistency Is Good, even if
there's also a long term plan to do something different.

Re: [PATCH] Always include defs.h first.

[Joel Brobecker]

[I agree consistency is good]

> Note that gdbserver doesn't have defs.h and lots of files in common/ do the
> 
> #ifdef GDBSERVER
> #include "server.h"
> #else
> #include "defs.h"
> #endif

Perhaps we should rename server.h into defs.h? Or move defs.h to
common?

-- 
Joel

Re: [PATCH] Always include defs.h first.

[Pedro Alves]

On 11/08/2012 06:38 PM, Joel Brobecker wrote:
> [I agree consistency is good]
> 
>> Note that gdbserver doesn't have defs.h and lots of files in common/ do the
>>
>> #ifdef GDBSERVER
>> #include "server.h"
>> #else
>> #include "defs.h"
>> #endif
> 
> Perhaps we should rename server.h into defs.h? Or move defs.h to
> common?

I agree with adding a defs.h to gdbserver.
defs.h depends on bfd and has lots of gdb-specifics, so no
good for gdbserver as is.

-- 
Pedro Alves

Re: [PATCH] Always include defs.h first.

[Pedro Alves]

On 11/08/2012 06:12 PM, Doug Evans wrote:
> 
> Another thing to consider:
> I'd like to move gdb to being more componentized (for lack of a better word).
> i.e., made up of application independent pieces.
> common/ is a minor step in this direction.

Yeah.  I recently wrote the below in an internal discussion.  Allow me to
just paste it here, as it just fits.  This was about the direction
of local/remote parity project once we reach a state where we can consider
reusing gdbserver's backend within gdb, instead of the
linux-nat.c/linux-low.c & friends duplication.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The division depends on how much of gdbserver we'd put into GDB.  One idea is to
split at the target_ops level, where gdb's target_ops vector calls gdbserver's
target_ops methods (bypassing most of "core" gdbserver code, RSP etc.)
But gdbserver's backends make use of e.g., breakpoints, behind gdb's back.  It
may prove to be better to keep core gdb agnostic of this, and import gdbserver's
breakpoints and regcache modules into gdb too (strictly for target side stuff),
instead of adapting the backends use the main gdb's breakpoints list and regcaches.

I never liked the "common" moniker much.  It is a bit artificial and an
artifact.  It tells us that the code is used by more than one something,
but it doesn't tell us really what's inside.  IMO, it's not much different
from saying the gdb/ is the new common and allowing gdb/gdbserver/ to refer
to files under gdb/ .  In the extreme, we could end up with half of gdb
under common/ in a few years...  I was hoping we could so a better split,
say, start by putting the native target code (target_ops&friends) into
its own dir.  Or maybe call it "libbackend/"...

In any case, I think we have two kinds of "common" stuff.  There's the
native target backends (ptrace and friends), but there's also the
host side common stuff.  If you strip both gdb and gdbserver of their
native target backends (e.g, in gdbserver, you end up with "main()", command
line option processing, the event loop, the RSP mashalling, etc.., you'll
still find things that are common between the two programs.  The event
loop is an obvious example.  So it feels to me that we could take the
opportunity to do a better code division.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> Note that gdbserver doesn't have defs.h and lots of files in common/ do the

Well aware.  :-)

-- 
Pedro Alves

Re: [PATCH] Always include defs.h first.

[Doug Evans]

On Thu, Nov 8, 2012 at 10:55 AM, Pedro Alves <palves@redhat.com> wrote:
> Or maybe call it "libbackend/"...

"We need more libraries."

> So it feels to me that we could take the
> opportunity to do a better code division.

Agreed!

Re: [PATCH] Always include defs.h first.

[Joel Brobecker]

> One idea is to split at the target_ops level, where gdb's target_ops
> vector calls gdbserver's target_ops methods (bypassing most of "core"
> gdbserver code, RSP etc.)

"like" :-).

> But gdbserver's backends make use of e.g., breakpoints, behind gdb's
> back.  It may prove to be better to keep core gdb agnostic of this,
> and import gdbserver's breakpoints and regcache modules into gdb too
> (strictly for target side stuff), instead of adapting the backends use
> the main gdb's breakpoints list and regcaches.

This might need a little clarification when we get to it, because
I think we might be forcing stuff in gdbserver on some platforms
that would otherwise not need it. This could be an issue for platforms
where people are trying to provide as small a gdbserver as possible.

> I never liked the "common" moniker much.  It is a bit artificial and an
> artifact.  It tells us that the code is used by more than one something,
> but it doesn't tell us really what's inside.

I very much agree with this. I think it goes exactly in the same
direction as what Doug is saying, regarding modularization. If you
believe the LLVM documentation, that's one of the strong technical
elements in their design.

Going a little further, we could imagine that the remote protocol
being implemented as one or two modules. With a common target_ops,
and maybe a few other little modules that I am forgetting, we could
see gdbserver as being a few lines of code that glue the necessary
modules together.

This could be the beginning of GDB 8.0. It'd actually be so exciting
that I'd go straight to 10 and name it GDB X :-).

-- 
Joel

Re: [PATCH] Always include defs.h first.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> OTOH, IMO, defs.h is special.

Yes, it is.  I'm in favor of this patch.

A different approach globally might be preferable.  But, that is a
separate discussion.  Your patch simply brings the rest of gdb in line
with what I understood to be existing practice.  We've certainly asked
patch submitters to make this change before...

Please put it in.

Tom

Re: [PATCH] Always include defs.h first.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Yeah.  They're getting it from those headers.  IMO, this makes it
Pedro> easy to forget about it, and risk some change adding the inclusion
Pedro> of a system header before including py-events.h, etc.  But since
Pedro> we don't have consensus, I'll leave it at that.

My view is that the uses of defs.h in python/*.h are just review errors.
If I had noticed this when the patches went in, I would have prevented
it.

Furthermore I think it doesn't really make sense to approach this
halfway.  There's no principled reason that only python/*.c should fail
to include defs.h first.  It makes gdb less consistent but not to any
end.

Finally, the defs.h rule is in gdbint.texinfo.

On the basis of this last point, I am committing the appended.
Tested by rebuilding.

Tom

2012-11-08  Tom Tromey  <tromey@redhat.com>

	* python/py-bpevent.c: Include defs.h.
	* python/py-continueevent.c: Include defs.h.
	* python/py-event.c: Include defs.h.
	* python/py-event.h: Don't include defs.h.
	* python/py-events.h: Don't include defs.h.
	* python/py-evts.c: Include defs.h.
	* python/py-exitedevent.c: Include defs.h.
	* python/py-newobjfileevent.c: Include defs.h.
	* python/py-signalevent.c: Include defs.h.
	* python/py-stopevent.c: Include defs.h.
	* python/py-threadevent.c: Include defs.h.

Index: python/py-bpevent.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-bpevent.c,v
retrieving revision 1.4
diff -u -r1.4 py-bpevent.c
--- python/py-bpevent.c	15 Aug 2012 14:22:02 -0000	1.4
+++ python/py-bpevent.c	8 Nov 2012 19:31:13 -0000
@@ -17,6 +17,7 @@
    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 "defs.h"
 #include "py-stopevent.h"
 
 static PyTypeObject breakpoint_event_object_type;
Index: python/py-continueevent.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-continueevent.c,v
retrieving revision 1.3
diff -u -r1.3 py-continueevent.c
--- python/py-continueevent.c	1 Mar 2012 21:06:54 -0000	1.3
+++ python/py-continueevent.c	8 Nov 2012 19:31:13 -0000
@@ -17,6 +17,7 @@
    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 "defs.h"
 #include "py-event.h"
 
 static PyTypeObject continue_event_object_type;
Index: python/py-event.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-event.c,v
retrieving revision 1.4
diff -u -r1.4 py-event.c
--- python/py-event.c	15 Aug 2012 14:22:02 -0000	1.4
+++ python/py-event.c	8 Nov 2012 19:31:13 -0000
@@ -17,6 +17,7 @@
    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 "defs.h"
 #include "py-event.h"
 
 void
Index: python/py-event.h
===================================================================
RCS file: /cvs/src/src/gdb/python/py-event.h,v
retrieving revision 1.5
diff -u -r1.5 py-event.h
--- python/py-event.h	4 Jan 2012 08:17:25 -0000	1.5
+++ python/py-event.h	8 Nov 2012 19:31:13 -0000
@@ -20,7 +20,6 @@
 #ifndef GDB_PY_EVENT_H
 #define GDB_PY_EVENT_H
 
-#include "defs.h"
 #include "py-events.h"
 #include "command.h"
 #include "python-internal.h"
Index: python/py-events.h
===================================================================
RCS file: /cvs/src/src/gdb/python/py-events.h,v
retrieving revision 1.4
diff -u -r1.4 py-events.h
--- python/py-events.h	4 Jan 2012 08:17:25 -0000	1.4
+++ python/py-events.h	8 Nov 2012 19:31:13 -0000
@@ -20,7 +20,6 @@
 #ifndef GDB_PY_EVENTS_H
 #define GDB_PY_EVENTS_H
 
-#include "defs.h"
 #include "command.h"
 #include "python-internal.h"
 #include "inferior.h"
Index: python/py-evts.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-evts.c,v
retrieving revision 1.4
diff -u -r1.4 py-evts.c
--- python/py-evts.c	4 Jan 2012 08:17:25 -0000	1.4
+++ python/py-evts.c	8 Nov 2012 19:31:13 -0000
@@ -17,6 +17,7 @@
    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 "defs.h"
 #include "py-events.h"
 
 /* Initialize python events.  */
Index: python/py-exitedevent.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-exitedevent.c,v
retrieving revision 1.5
diff -u -r1.5 py-exitedevent.c
--- python/py-exitedevent.c	15 Aug 2012 14:22:02 -0000	1.5
+++ python/py-exitedevent.c	8 Nov 2012 19:31:13 -0000
@@ -17,6 +17,7 @@
    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 "defs.h"
 #include "py-event.h"
 
 static PyTypeObject exited_event_object_type;
Index: python/py-newobjfileevent.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-newobjfileevent.c,v
retrieving revision 1.5
diff -u -r1.5 py-newobjfileevent.c
--- python/py-newobjfileevent.c	6 Sep 2012 20:14:13 -0000	1.5
+++ python/py-newobjfileevent.c	8 Nov 2012 19:31:13 -0000
@@ -17,6 +17,7 @@
    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 "defs.h"
 #include "py-event.h"
 
 static PyTypeObject new_objfile_event_object_type;
Index: python/py-signalevent.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-signalevent.c,v
retrieving revision 1.4
diff -u -r1.4 py-signalevent.c
--- python/py-signalevent.c	15 Aug 2012 14:22:02 -0000	1.4
+++ python/py-signalevent.c	8 Nov 2012 19:31:13 -0000
@@ -17,6 +17,7 @@
    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 "defs.h"
 #include "py-stopevent.h"
 
 static PyTypeObject signal_event_object_type;
Index: python/py-stopevent.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-stopevent.c,v
retrieving revision 1.6
diff -u -r1.6 py-stopevent.c
--- python/py-stopevent.c	15 Aug 2012 14:22:02 -0000	1.6
+++ python/py-stopevent.c	8 Nov 2012 19:31:13 -0000
@@ -17,6 +17,7 @@
    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 "defs.h"
 #include "py-stopevent.h"
 
 PyObject *
Index: python/py-threadevent.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-threadevent.c,v
retrieving revision 1.3
diff -u -r1.3 py-threadevent.c
--- python/py-threadevent.c	15 Aug 2012 14:22:02 -0000	1.3
+++ python/py-threadevent.c	8 Nov 2012 19:31:13 -0000
@@ -15,6 +15,7 @@
    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 "defs.h"
 #include "py-event.h"
 
 /* thread events can either be thread specific or process wide.  If gdb is

Re: [PATCH] Always include defs.h first.

[Yao Qi]

On 11/09/2012 12:56 AM, Pedro Alves wrote:
> defs.h includes config.h and config.h must always be included before system headers.
>
> This is probably "authoritatively" described somewhere in autoconf's docs, but
> I can't find it now.  But see e.g.,<http://inaugust.com/post/68>.

Oh, that makes sense to me.  Thanks for the explanations.

-- 
Yao