Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Simon Marchi <simon.marchi@efficios.com>
To: gdb-patches@sourceware.org
Cc: Pedro Alves <pedro@palves.net>, Simon Marchi <simon.marchi@efficios.com>
Subject: [PATCH v2 3/4] gdb, gdbserver, gdbsupport: include early header files with `-include`
Date: Fri, 22 Mar 2024 22:14:54 -0400	[thread overview]
Message-ID: <20240323021648.154109-4-simon.marchi@efficios.com> (raw)
In-Reply-To: <20240323021648.154109-1-simon.marchi@efficios.com>

The motivation for this change is for analysis tools and IDEs to be
better at analyzing header files on their own.

There are some definitions and includes we want to occur at the very
beginning of all translation units.  The way we currently do that is by
requiring all source files (.c and .cc files) to include one of defs.h
(for gdb), server.h (for gdbserver) of common-defs.h (for gdbsupport and
shared source files).  These special header files define and include
everything that needs to be included at the very beginning.  Other
header files are written in a way that assume that these special
"prologue" header files have already been included.

My problem with that is that my editor (clangd-based) provides a very
bad experience when editing header files.  Since clangd doesn't know
that one of defs.h/server.h/common-defs.h was included already, a lot of
things are flagged as errors.  For instance, CORE_ADDR is not known.
It's possible to edit the files in this state, but a lot of the power of
the editor is unavailable.

My proposal to help with this is to include those things we always want
to be there using the compilers' `-include` option.  Tom Tromey said
that the current approach might exist because not all compilers used to
have an option like this.  But I believe that it's safe to assume they
do today.

With this change, clangd picks up the -include option from the compile
command, and is able to analyze the header file correctly, as it sees
all that stuff included or defined but that -include option.  That works
because when editing a header file, clangd tries to get the compilation
flags from a source file that includes said header file.

This change is a bit self-serving, because it addresses one of my
frustrations when editing header files, but it might help others too.
I'd be curious to know if others encounter the same kinds of problems
when editing header files.  Also, even if the change is not necessary by
any means, I think the solution of using -include for stuff we always
want to be there is more elegant than the current solution.

Even with this -include flag, many header files currently don't include
what they use, but rather depend on files included before them.  This
will still cause errors when editing them, but it should be easily
fixable by adding the appropriate include.  There's no rush to do so, as
long as the code still compiles, it's just a convenience thing.

The changes are:

 - Add the appropriate `-include` option to the various Makefiles.

 - There is one problem for gdbserver's Makefile: we do not want to
   include server.h when building `gdbreplay.o`.  So we can't simply put
   the `-include` in `INTERNAL_CFLAGS`.  I added the `-include` option
   to the `COMPILE` and `IPAGENT_COMPILE` variables, and added a special
   rule to compile `gdbreplay.o`.

 - Remove the `-include` option from the `check-headers` rule, since it
   is already included in `INTERNAL_CFLAGS`.

Change-Id: If3e345d00a9fc42336322f1d8286687d22134340
---
 gdb/Makefile.in        |  3 ++-
 gdbserver/Makefile.in  | 11 +++++++++--
 gdbsupport/Makefile.am |  1 +
 gdbsupport/Makefile.in |  1 +
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index a5139ea75803..a9f641c06593 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -607,6 +607,7 @@ GDB_CFLAGS = \
 	-I. \
 	-I$(srcdir) \
 	-I$(srcdir)/config \
+	-include $(srcdir)/defs.h \
 	-DLOCALEDIR="\"$(localedir)\"" \
 	$(DEFS)
 
@@ -2050,7 +2051,7 @@ check-headers:
 	@echo Checking headers.
 	for i in $(CHECK_HEADERS) ; do \
 		$(CXX) $(CXX_DIALECT) -x c++-header -c -fsyntax-only \
-		$(INTERNAL_CFLAGS) $(CXXFLAGS) -include defs.h $(srcdir)/$$i ; \
+		$(INTERNAL_CFLAGS) $(CXXFLAGS) $(srcdir)/$$i ; \
 	done
 .PHONY: check-headers
 
diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
index 45073abca493..4d29491a8721 100644
--- a/gdbserver/Makefile.in
+++ b/gdbserver/Makefile.in
@@ -69,9 +69,11 @@ COMPILE.pre = $(CXX) $(CXX_DIALECT)
 COMPILE.post = -c -o $@
 POSTCOMPILE = @true
 
+INCLUDE_SERVER_H = -include $(srcdir)/server.h
+
 # CXXFLAGS is at the very end on purpose, so that user-supplied flags can
 # override internal flags.
-COMPILE = $(ECHO_CXX) $(COMPILE.pre) $(INTERNAL_CFLAGS) $(CXXFLAGS) $(COMPILE.post)
+COMPILE = $(ECHO_CXX) $(COMPILE.pre) $(INTERNAL_CFLAGS) $(INCLUDE_SERVER_H) $(CXXFLAGS) $(COMPILE.post)
 
 # It is also possible that you will need to add -I/usr/include/sys to the
 # CFLAGS section if your system doesn't have fcntl.h in /usr/include (which
@@ -509,7 +511,7 @@ IPAGENT_CFLAGS = \
 
 # CXXFLAGS is at the very end on purpose, so that user-supplied flags can
 # override internal flags.
-IPAGENT_COMPILE = $(ECHO_CXX) $(COMPILE.pre) $(IPAGENT_CFLAGS) $(CXXFLAGS) $(COMPILE.post)
+IPAGENT_COMPILE = $(ECHO_CXX) $(COMPILE.pre) $(IPAGENT_CFLAGS) $(INCLUDE_SERVER_H) $(CXXFLAGS) $(COMPILE.post)
 
 # Rules for special cases.
 
@@ -589,6 +591,11 @@ target/%.o: ../gdb/target/%.c
 %-generated.cc: ../gdb/regformats/rs6000/%.dat $(regdat_sh)
 	$(ECHO_REGDAT) $(SHELL) $(regdat_sh) $< $@
 
+# Rule for gdbreplay.o.  This is the same as COMPILE, without INCLUDE_SERVER_H.
+
+gdbreplay.o: gdbreplay.cc
+	$(ECHO_CXX) $(COMPILE.pre) $(INTERNAL_CFLAGS) $(CXXFLAGS) $(COMPILE.post) $<
+
 #
 # Dependency tracking.
 #
diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
index 7c360aa413ef..2b0f987125c1 100644
--- a/gdbsupport/Makefile.am
+++ b/gdbsupport/Makefile.am
@@ -35,6 +35,7 @@ AM_CPPFLAGS = \
 	$(INCINTL) \
 	-I../bfd \
 	-I$(srcdir)/../bfd \
+	-include $(srcdir)/common-defs.h \
 	@LARGEFILE_CPPFLAGS@
 
 override CXX += $(CXX_DIALECT)
diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
index ab35b9148a50..ee709112aaed 100644
--- a/gdbsupport/Makefile.in
+++ b/gdbsupport/Makefile.in
@@ -403,6 +403,7 @@ AM_CPPFLAGS = \
 	$(INCINTL) \
 	-I../bfd \
 	-I$(srcdir)/../bfd \
+	-include $(srcdir)/common-defs.h \
 	@LARGEFILE_CPPFLAGS@
 
 AM_CXXFLAGS = $(WARN_CFLAGS) $(WERROR_CFLAGS)
-- 
2.44.0


  parent reply	other threads:[~2024-03-23  2:18 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-23  2:14 [PATCH v2 0/4] Include early include " Simon Marchi
2024-03-23  2:14 ` [PATCH v2 1/4] gdb, gdbserver, gdbsupport: reformat some Makefile variables, one entry per line Simon Marchi
2024-03-23  2:14 ` [PATCH v2 2/4] {gdb, gdbserver}/Makefile.in: remove unnecessary intermediary variables Simon Marchi
2024-03-23  2:14 ` Simon Marchi [this message]
2024-03-26 11:57   ` [PATCH v2 3/4] gdb, gdbserver, gdbsupport: include early header files with `-include` Pedro Alves
2024-03-26 14:39     ` Simon Marchi
2024-03-26 15:31       ` Pedro Alves
2024-03-26 15:49         ` Simon Marchi
2024-03-23  2:14 ` [PATCH v2 4/4] gdb, gdbserver, gdbsupport: remove includes of early headers Simon Marchi
2024-03-26 11:59   ` Pedro Alves
2024-03-26 14:58     ` Simon Marchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240323021648.154109-4-simon.marchi@efficios.com \
    --to=simon.marchi@efficios.com \
    --cc=gdb-patches@sourceware.org \
    --cc=pedro@palves.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox