From patchwork Mon Mar 18 20:01:38 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 87330 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4A7B63858425 for ; Mon, 18 Mar 2024 20:03:40 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 13FF73858C5F for ; Mon, 18 Mar 2024 20:03:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 13FF73858C5F Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=efficios.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=efficios.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 13FF73858C5F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710792190; cv=none; b=QafLvBcBq2Yw24WaaI6ecyMGEo2Mt4HG+uYvkxuCi38oY7vmTEoBkSCJNs2KDAeeEOaCp9Vk470CZxTW1ynWCTgtX5bG58eBtdBcHd7mhoDPPccm/h3aGw7UX5z8elDpQni4wL6pNeyFK4UcAWntWQ7yxtD8zGBVqtUeiioGOJs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710792190; c=relaxed/simple; bh=B5EWvAGwufa7LgCi/R7Uyc7Eb9YNTx4+eztdGw3sLvU=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=UqfimT88LscmENDsG/gU0VuKmLw271y03xnt9duttE/Wqk8z6z6zbohwGx1HZQO24CoUWwkfFPm9QwPis3H6Nlu5T9jxQdSBX7SiWfA0ilNK22Uc5Dp3CjhDUT9XCO6xX5L3sTZCszeF0BKJNuJf4nUsaOeJUNjd+aAl4zAU/jk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from localhost.localdomain (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B50881E08C; Mon, 18 Mar 2024 16:02:58 -0400 (EDT) From: Simon Marchi To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 1/3] gdb, gdbserver, gdbsupport: reformat some Makefile variables, one entry per line Date: Mon, 18 Mar 2024 16:01:38 -0400 Message-ID: <20240318200257.131199-1-simon.marchi@efficios.com> X-Mailer: git-send-email 2.44.0 MIME-Version: 1.0 X-Spam-Status: No, score=-1173.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_SOFTFAIL, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Reformat some variables that the subsequent patches are going to touch. I think it makes them easier to read, and it also makes diffs clearer. Change-Id: I82f63ba0e6d0fe268eb1f1ad5ab22c3cd016ab02 --- gdb/Makefile.in | 8 ++++++-- gdbserver/Makefile.in | 12 +++++++++--- gdbsupport/Makefile.am | 15 +++++++++++---- 3 files changed, 26 insertions(+), 9 deletions(-) base-commit: 0273b5967e21404d0442273b189b0e275cfafa74 diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 331620375aed..95709ae395a2 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -601,8 +601,12 @@ CONFIG_DEP_SUBDIR = $(addsuffix /$(DEPDIR),$(CONFIG_SRC_SUBDIR)) # your system doesn't have fcntl.h in /usr/include (which is where it # should be according to Posix). DEFS = @DEFS@ -GDB_CFLAGS = -I. -I$(srcdir) -I$(srcdir)/config \ - -DLOCALEDIR="\"$(localedir)\"" $(DEFS) +GDB_CFLAGS = \ + -I. \ + -I$(srcdir) \ + -I$(srcdir)/config \ + -DLOCALEDIR="\"$(localedir)\"" \ + $(DEFS) # MH_CFLAGS, if defined, has host-dependent CFLAGS from the config directory. GLOBAL_CFLAGS = $(MH_CFLAGS) diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in index c7120895a26d..c17a0a522df2 100644 --- a/gdbserver/Makefile.in +++ b/gdbserver/Makefile.in @@ -125,9 +125,15 @@ INCSUPPORT = -I$(srcdir)/.. -I.. # in those directories should be included with the subdirectory. # e.g.: "target/wait.h". # -INCLUDE_CFLAGS = -I. -I${srcdir} \ - -I$(srcdir)/../gdb/regformats -I$(srcdir)/.. -I$(INCLUDE_DIR) \ - -I$(srcdir)/../gdb $(INCGNU) $(INCSUPPORT) \ +INCLUDE_CFLAGS = \ + -I. \ + -I${srcdir} \ + -I$(srcdir)/../gdb/regformats \ + -I$(srcdir)/.. \ + -I$(INCLUDE_DIR) \ + -I$(srcdir)/../gdb \ + $(INCGNU) \ + $(INCSUPPORT) \ $(INTL_CFLAGS) # M{H,T}_CFLAGS, if defined, has host- and target-dependent CFLAGS diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am index 88414b4c927a..7c360aa413ef 100644 --- a/gdbsupport/Makefile.am +++ b/gdbsupport/Makefile.am @@ -25,10 +25,17 @@ ACLOCAL_AMFLAGS = -I . -I ../config # ZW_GNU_GETTEXT_SISTER_DIR, but doesn't have any translations (currently). SUBDIRS = -AM_CPPFLAGS = -I$(srcdir)/../include -I$(srcdir)/../gdb \ - -I../gnulib/import -I$(srcdir)/../gnulib/import \ - -I.. -I$(srcdir)/.. $(INCINTL) -I../bfd -I$(srcdir)/../bfd \ - @LARGEFILE_CPPFLAGS@ +AM_CPPFLAGS = \ + -I$(srcdir)/../include \ + -I$(srcdir)/../gdb \ + -I../gnulib/import \ + -I$(srcdir)/../gnulib/import \ + -I.. \ + -I$(srcdir)/.. \ + $(INCINTL) \ + -I../bfd \ + -I$(srcdir)/../bfd \ + @LARGEFILE_CPPFLAGS@ override CXX += $(CXX_DIALECT) From patchwork Mon Mar 18 20:01:39 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 87331 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3798B3858430 for ; Mon, 18 Mar 2024 20:03:43 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 2F73B3858C78 for ; Mon, 18 Mar 2024 20:03:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2F73B3858C78 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=efficios.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=efficios.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 2F73B3858C78 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710792190; cv=none; b=gblNPRpC27vZxlMat3D57wS9K3GC/z/L/u1ha3c9qvCNDrL/HrhWisuoSC6zZND890SZpwxUgwAPMzETO7FH88kWQQ7J1m9j382MlXFnTKJUz4xHb2lcB4B7TS8RaL2Jv7KhvaCPLXIsYsVTKZnsOisdPdaihH8PhMCe77t9ybc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710792190; c=relaxed/simple; bh=wQwoo3louirv62dO39qykiixF9rrcvu4uoC+SFtmXMs=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=l1xMhhpN14Ix7g/RwYIFu8Ku8IcUhC+Jc5U/xrAygxWKqcNTlmYCVBKtZvawRIAOkvpJExs4g/gom0qfHUI8dIcpHv6D+sjH6Lu1HK1S1o73Quoc+mxRtJzyX5hrvnWxmRqqOaQNooCGXbMwONZY3VUkzsrc4b0e99Ngwyo1go8= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from localhost.localdomain (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D9F161E0AC; Mon, 18 Mar 2024 16:02:59 -0400 (EDT) From: Simon Marchi To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 2/3] gdb, gdbserver, gdbsupport: include config.h files with -include Date: Mon, 18 Mar 2024 16:01:39 -0400 Message-ID: <20240318200257.131199-2-simon.marchi@efficios.com> X-Mailer: git-send-email 2.44.0 In-Reply-To: <20240318200257.131199-1-simon.marchi@efficios.com> References: <20240318200257.131199-1-simon.marchi@efficios.com> MIME-Version: 1.0 X-Spam-Status: No, score=-1173.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_PASS, SPF_SOFTFAIL, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org 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 source 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, 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 copiles, it's just a convenience thing. This patch does the small step of moving the inclusion of the various config.h files to that new method. The changes are: - Add three files meant to be included with -include: gdb/gdb.inc.h, gdbserver/gdbserver.inc.h and gdbsupport/gdbsupport.inc.h. - Move the inclusion of the various config.h files there - Modify the compilation flags of all three subdirectories to add the appropriate -include option. Change-Id: If3e345d00a9fc42336322f1d8286687d22134340 --- gdb/Makefile.in | 1 + gdb/defs.h | 7 ------- gdb/gdb.inc.h | 22 ++++++++++++++++++++++ gdbserver/Makefile.in | 3 ++- gdbserver/gdbserver.inc.h | 22 ++++++++++++++++++++++ gdbserver/server.h | 8 -------- gdbsupport/Makefile.am | 1 + gdbsupport/Makefile.in | 16 ++++++++++++---- gdbsupport/common-defs.h | 10 ---------- gdbsupport/gdbsupport.inc.h | 35 +++++++++++++++++++++++++++++++++++ 10 files changed, 95 insertions(+), 30 deletions(-) create mode 100644 gdb/gdb.inc.h create mode 100644 gdbserver/gdbserver.inc.h create mode 100644 gdbsupport/gdbsupport.inc.h diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 95709ae395a2..3b144fa3034c 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -605,6 +605,7 @@ GDB_CFLAGS = \ -I. \ -I$(srcdir) \ -I$(srcdir)/config \ + -include $(srcdir)/gdb.inc.h \ -DLOCALEDIR="\"$(localedir)\"" \ $(DEFS) diff --git a/gdb/defs.h b/gdb/defs.h index cf471bf5d662..8a9ff3aba0f7 100644 --- a/gdb/defs.h +++ b/gdb/defs.h @@ -25,13 +25,6 @@ #include "gdbsupport/common-defs.h" -#undef PACKAGE -#undef PACKAGE_NAME -#undef PACKAGE_VERSION -#undef PACKAGE_STRING -#undef PACKAGE_TARNAME - -#include #include "bfd.h" #include diff --git a/gdb/gdb.inc.h b/gdb/gdb.inc.h new file mode 100644 index 000000000000..7be4c8eea939 --- /dev/null +++ b/gdb/gdb.inc.h @@ -0,0 +1,22 @@ +/* Copyright (C) 2024 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 . */ + +/* This file in included automatically via `-include` at the beginning of each + source file compiled in gdb/. */ + +#include "gdbsupport/gdbsupport.inc.h" +#include "config.h" diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in index c17a0a522df2..c92b881650a4 100644 --- a/gdbserver/Makefile.in +++ b/gdbserver/Makefile.in @@ -134,7 +134,8 @@ INCLUDE_CFLAGS = \ -I$(srcdir)/../gdb \ $(INCGNU) \ $(INCSUPPORT) \ - $(INTL_CFLAGS) + $(INTL_CFLAGS) \ + -include $(srcdir)/gdbserver.inc.h # M{H,T}_CFLAGS, if defined, has host- and target-dependent CFLAGS # from the config/ directory. diff --git a/gdbserver/gdbserver.inc.h b/gdbserver/gdbserver.inc.h new file mode 100644 index 000000000000..22ec0dd94318 --- /dev/null +++ b/gdbserver/gdbserver.inc.h @@ -0,0 +1,22 @@ +/* Copyright (C) 2024 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 . */ + +/* This file in included automatically via `-include` at the beginning of each + source file compiled in gdbserver/. */ + +#include "gdbsupport/gdbsupport.inc.h" +#include "config.h" diff --git a/gdbserver/server.h b/gdbserver/server.h index 0074818c6df0..ee27d2b3f5c2 100644 --- a/gdbserver/server.h +++ b/gdbserver/server.h @@ -21,14 +21,6 @@ #include "gdbsupport/common-defs.h" -#undef PACKAGE -#undef PACKAGE_NAME -#undef PACKAGE_VERSION -#undef PACKAGE_STRING -#undef PACKAGE_TARNAME - -#include - static_assert (sizeof (CORE_ADDR) >= sizeof (void *)); #include "gdbsupport/version.h" diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am index 7c360aa413ef..db1eee88059a 100644 --- a/gdbsupport/Makefile.am +++ b/gdbsupport/Makefile.am @@ -35,6 +35,7 @@ AM_CPPFLAGS = \ $(INCINTL) \ -I../bfd \ -I$(srcdir)/../bfd \ + -include $(srcdir)/gdbsupport.inc.h \ @LARGEFILE_CPPFLAGS@ override CXX += $(CXX_DIALECT) diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in index 6f8dacc157f9..9b1063f31ab3 100644 --- a/gdbsupport/Makefile.in +++ b/gdbsupport/Makefile.in @@ -393,10 +393,18 @@ ACLOCAL_AMFLAGS = -I . -I ../config # from Automake, as gdbsupport uses AM_GNU_GETTEXT through # ZW_GNU_GETTEXT_SISTER_DIR, but doesn't have any translations (currently). SUBDIRS = -AM_CPPFLAGS = -I$(srcdir)/../include -I$(srcdir)/../gdb \ - -I../gnulib/import -I$(srcdir)/../gnulib/import \ - -I.. -I$(srcdir)/.. $(INCINTL) -I../bfd -I$(srcdir)/../bfd \ - @LARGEFILE_CPPFLAGS@ +AM_CPPFLAGS = \ + -I$(srcdir)/../include \ + -I$(srcdir)/../gdb \ + -I../gnulib/import \ + -I$(srcdir)/../gnulib/import \ + -I.. \ + -I$(srcdir)/.. \ + $(INCINTL) \ + -I../bfd \ + -I$(srcdir)/../bfd \ + -include $(srcdir)/gdbsupport.inc.h \ + @LARGEFILE_CPPFLAGS@ AM_CXXFLAGS = $(WARN_CFLAGS) $(WERROR_CFLAGS) noinst_LIBRARIES = libgdbsupport.a diff --git a/gdbsupport/common-defs.h b/gdbsupport/common-defs.h index 6120719480b8..e7eb814f3251 100644 --- a/gdbsupport/common-defs.h +++ b/gdbsupport/common-defs.h @@ -20,16 +20,6 @@ #ifndef COMMON_COMMON_DEFS_H #define COMMON_COMMON_DEFS_H -#include - -#undef PACKAGE_NAME -#undef PACKAGE -#undef PACKAGE_VERSION -#undef PACKAGE_STRING -#undef PACKAGE_TARNAME - -#include "gnulib/config.h" - /* From: https://www.gnu.org/software/gnulib/manual/html_node/stdint_002eh.html diff --git a/gdbsupport/gdbsupport.inc.h b/gdbsupport/gdbsupport.inc.h new file mode 100644 index 000000000000..ab0999579528 --- /dev/null +++ b/gdbsupport/gdbsupport.inc.h @@ -0,0 +1,35 @@ +/* Copyright (C) 2024 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 . */ + +/* This file in included automatically via `-include` at the beginning of each + source file compiled in gdbsupport/. */ + +#include "gdbsupport/config.h" + +#undef PACKAGE +#undef PACKAGE_NAME +#undef PACKAGE_STRING +#undef PACKAGE_TARNAME +#undef PACKAGE_VERSION + +#include "gnulib/config.h" + +#undef PACKAGE +#undef PACKAGE_NAME +#undef PACKAGE_STRING +#undef PACKAGE_TARNAME +#undef PACKAGE_VERSION From patchwork Mon Mar 18 20:01:40 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 87332 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7DE243858C5F for ; Mon, 18 Mar 2024 20:03:52 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 8C5863858C35 for ; Mon, 18 Mar 2024 20:03:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8C5863858C35 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=efficios.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=efficios.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8C5863858C35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710792190; cv=none; b=pTIA96gghzt+uNW8yiG2Qddcr1c8aA0Apj1Lw54V/wJb8Ec+GG6cIIyLIL4Zytkc4DOB0U9MR96Laf8RS8JPpboP00pK0ySWY83v7KsxWMtuLgF5ESs8Q4lWBxNO32E55C7qvKmHVZjQTJUm6MA00BFLZVv/DScrGuUMNLPbWRk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710792190; c=relaxed/simple; bh=0zQMwjMD/JPPUW7ESbPQvdUEP+2rivUuqT5OF/tnYNs=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=n/UneFhf4JxYbubv4Gh1nE26a+zrNGPF+tO/5p4QeSpOUn8sx7myP6SZlbW3RZgX0dXRGzDu3h67AznLCcov0ASSYDujVK9O2yupcSKejP2ngiQ4RL0PI1+XMFetP/wUo6HhMcq/xVbwIUd8VJkHXfM141H7r2C7w5nZ9nW720Q= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from localhost.localdomain (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D9E4F1E0BB; Mon, 18 Mar 2024 16:03:00 -0400 (EDT) From: Simon Marchi To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 3/3] gdbsupport: move more things to gdbsupport.inc.h Date: Mon, 18 Mar 2024 16:01:40 -0400 Message-ID: <20240318200257.131199-3-simon.marchi@efficios.com> X-Mailer: git-send-email 2.44.0 In-Reply-To: <20240318200257.131199-1-simon.marchi@efficios.com> References: <20240318200257.131199-1-simon.marchi@efficios.com> MIME-Version: 1.0 X-Spam-Status: No, score=-1173.5 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_PASS, SPF_SOFTFAIL, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Move some more of the things that should be seen everywhere by all compilation units from common-defs.h to gdbsupport.inc.h. After this change, to the best of my knowledge, common-defs.h, server.h and defs.h are no longer special, they don't need to be included first anymore. A lot of source files don't need what is directly defined in common-defs.h / server.h / defs.h , but rather use things in header files included transitively. There's no rush to do so, but for these files, we'll be able to remove the inclusion of common-defs.h / server.h / defs.h and make them include what they actually use. Ultimately, I think we could just get rid of these header files, since they pretty much just server as big bags of includes. Moving to more fine-graned includes means less unnecessary includes, meaning less unnecessary recompilation when some header file is modified. The things moved to gdbsupport.inc.h are: - __STDC_CONSTANT_MACROS, __STDC_LIMIT_MACROS, __STDC_FORMAT_MACROS: according to the comment, they must be defined before including some standard headers. Are these defines still useful though? - _FORTIFY_SOURCE: must come before any standard library include. - _WIN32_WINNT: must come before including the Windows API headers. - ATTRIBUTE_PRINTF, ATTRIBUTE_NONNULL: we override what ansidecl.h gives us. So better define it in gdbsupport.inc.h, so that there is no chance of some source file including ansidecl.h without having the overrides. - HAVE_USEFUL_SBRK: this needs to be seen everywhere, in case one does `#ifdef HAVE_USEFUL_SBRK`. - Inclusion of poison.h, so that all code everywhere, uses our overrides of xfree & co. This requires including liberty.h in poison.h, since it overrides some macros from it. - Since poison.h uses xfree and xfree uses some traits defined in poison.h, but gnulib also defines its own (non-templated) version of xfree, I had some strange error due to some non-compatible redefinitions. All in all, it becomes simpler to move xfree to poison.h and get rid of gdb-xfree.h, so do that. Change-Id: I21ad69e5f38f9fd7a3943d9f96d3782739d4b6df --- gdbsupport/common-defs.h | 147 ----------------------------------- gdbsupport/common-utils.cc | 1 - gdbsupport/gdb-xfree.h | 41 ---------- gdbsupport/gdb_unique_ptr.h | 1 - gdbsupport/gdbsupport.inc.h | 149 ++++++++++++++++++++++++++++++++++++ gdbsupport/poison.h | 20 ++++- 6 files changed, 167 insertions(+), 192 deletions(-) delete mode 100644 gdbsupport/gdb-xfree.h diff --git a/gdbsupport/common-defs.h b/gdbsupport/common-defs.h index e7eb814f3251..401e3c4ef099 100644 --- a/gdbsupport/common-defs.h +++ b/gdbsupport/common-defs.h @@ -20,60 +20,6 @@ #ifndef COMMON_COMMON_DEFS_H #define COMMON_COMMON_DEFS_H -/* From: - https://www.gnu.org/software/gnulib/manual/html_node/stdint_002eh.html - - "On some hosts that predate C++11, when using C++ one must define - __STDC_CONSTANT_MACROS to make visible the definitions of constant - macros such as INTMAX_C, and one must define __STDC_LIMIT_MACROS to - make visible the definitions of limit macros such as INTMAX_MAX.". - - And: - https://www.gnu.org/software/gnulib/manual/html_node/inttypes_002eh.html - - "On some hosts that predate C++11, when using C++ one must define - __STDC_FORMAT_MACROS to make visible the declarations of format - macros such as PRIdMAX." - - Must do this before including any system header, since other system - headers may include stdint.h/inttypes.h. */ -#define __STDC_CONSTANT_MACROS 1 -#define __STDC_LIMIT_MACROS 1 -#define __STDC_FORMAT_MACROS 1 - -/* Some distros enable _FORTIFY_SOURCE by default, which on occasion - has caused build failures with -Wunused-result when a patch is - developed on a distro that does not enable _FORTIFY_SOURCE. We - enable it here in order to try to catch these problems earlier; - plus this seems like a reasonable safety measure. The check for - optimization is required because _FORTIFY_SOURCE only works when - optimization is enabled. If _FORTIFY_SOURCE is already defined, - then we don't do anything. Also, on MinGW, fortify requires - linking to -lssp, and to avoid the hassle of checking for - that and linking to it statically, we just don't define - _FORTIFY_SOURCE there. */ - -#if (!defined _FORTIFY_SOURCE && defined __OPTIMIZE__ && __OPTIMIZE__ > 0 \ - && !defined(__MINGW32__)) -#define _FORTIFY_SOURCE 2 -#endif - -/* We don't support Windows versions before XP, so we define - _WIN32_WINNT correspondingly to ensure the Windows API headers - expose the required symbols. - - NOTE: this must be kept in sync with common.m4. */ -#if defined (__MINGW32__) || defined (__CYGWIN__) -# ifdef _WIN32_WINNT -# if _WIN32_WINNT < 0x0501 -# undef _WIN32_WINNT -# define _WIN32_WINNT 0x0501 -# endif -# else -# define _WIN32_WINNT 0x0501 -# endif -#endif /* __MINGW32__ || __CYGWIN__ */ - #include #include @@ -93,89 +39,6 @@ #include #endif -#include "ansidecl.h" -/* This is defined by ansidecl.h, but we prefer gnulib's version. On - 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_STANDARD - is compatible with ATTRIBUTE_PRINTF, simply use it. */ -#undef ATTRIBUTE_PRINTF -#define ATTRIBUTE_PRINTF _GL_ATTRIBUTE_FORMAT_PRINTF_STANDARD - -/* This is defined by ansidecl.h, but we disable the attribute. - - Say a developer starts out with: - ... - extern void foo (void *ptr) __attribute__((nonnull (1))); - void foo (void *ptr) {} - ... - with the idea in mind to catch: - ... - foo (nullptr); - ... - at compile time with -Werror=nonnull, and then adds: - ... - void foo (void *ptr) { - + gdb_assert (ptr != nullptr); - } - ... - to catch: - ... - foo (variable_with_nullptr_value); - ... - at runtime as well. - - Said developer then verifies that the assert works (using -O0), and commits - the code. - - Some other developer then checks out the code and accidentally writes some - variant of: - ... - foo (variable_with_nullptr_value); - ... - and builds with -O2, and ... the assert doesn't trigger, because it's - optimized away by gcc. - - There's no suppported recipe to prevent the assertion from being optimized - away (other than: build with -O0, or remove the nonnull attribute). Note - that -fno-delete-null-pointer-checks does not help. A patch was submitted - to improve gcc documentation to point this out more clearly ( - https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ). The - patch also mentions a possible workaround that obfuscates the pointer - using: - ... - void foo (void *ptr) { - + asm ("" : "+r"(ptr)); - gdb_assert (ptr != nullptr); - } - ... - but that still requires the developer to manually add this in all cases - where that's necessary. - - A warning was added to detect the situation: -Wnonnull-compare, which does - help in detecting those cases, but each new gcc release may indicate a new - batch of locations that needs fixing, which means we've added a maintenance - burden. - - We could try to deal with the problem more proactively by introducing a - gdb_assert variant like: - ... - void gdb_assert_non_null (void *ptr) { - asm ("" : "+r"(ptr)); - gdb_assert (ptr != nullptr); - } - void foo (void *ptr) { - gdb_assert_nonnull (ptr); - } - ... - and make it a coding style to use it everywhere, but again, maintenance - burden. - - With all these things considered, for now we go with the solution with the - least maintenance burden: disable the attribute, such that we reliably deal - with it everywhere. */ -#undef ATTRIBUTE_NONNULL -#define ATTRIBUTE_NONNULL(m) #define ATTRIBUTE_UNUSED_RESULT __attribute__ ((__warn_unused_result__)) #define ATTRIBUTE_USED __attribute__ ((__used__)) @@ -193,18 +56,8 @@ #include "common-debug.h" #include "cleanups.h" #include "common-exceptions.h" -#include "gdbsupport/poison.h" /* Pull in gdb::unique_xmalloc_ptr. */ #include "gdbsupport/gdb_unique_ptr.h" -/* sbrk on macOS is not useful for our purposes, since sbrk(0) always - returns the same value. brk/sbrk on macOS is just an emulation - that always returns a pointer to a 4MB section reserved for - that. */ - -#if defined (HAVE_SBRK) && !__APPLE__ -#define HAVE_USEFUL_SBRK 1 -#endif - #endif /* COMMON_COMMON_DEFS_H */ diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc index 99b9cb8609bb..35a4c66f4546 100644 --- a/gdbsupport/common-utils.cc +++ b/gdbsupport/common-utils.cc @@ -21,7 +21,6 @@ #include "common-utils.h" #include "host-defs.h" #include "gdbsupport/gdb-safe-ctype.h" -#include "gdbsupport/gdb-xfree.h" void * xzalloc (size_t size) diff --git a/gdbsupport/gdb-xfree.h b/gdbsupport/gdb-xfree.h deleted file mode 100644 index 09c75395b62f..000000000000 --- a/gdbsupport/gdb-xfree.h +++ /dev/null @@ -1,41 +0,0 @@ -/* Copyright (C) 1986-2024 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 . */ - -#ifndef GDBSUPPORT_GDB_XFREE_H -#define GDBSUPPORT_GDB_XFREE_H - -#include "gdbsupport/poison.h" - -/* GDB uses this instead of 'free', it detects when it is called on an - invalid type. */ - -template -static void -xfree (T *ptr) -{ - static_assert (IsFreeable::value, "Trying to use xfree with a non-POD \ -data type. Use operator delete instead."); - - if (ptr != NULL) -#ifdef GNULIB_NAMESPACE - GNULIB_NAMESPACE::free (ptr); /* ARI: free */ -#else - free (ptr); /* ARI: free */ -#endif -} - -#endif /* GDBSUPPORT_GDB_XFREE_H */ diff --git a/gdbsupport/gdb_unique_ptr.h b/gdbsupport/gdb_unique_ptr.h index 19b1581dab5c..3c2ec4098bed 100644 --- a/gdbsupport/gdb_unique_ptr.h +++ b/gdbsupport/gdb_unique_ptr.h @@ -22,7 +22,6 @@ #include #include -#include "gdbsupport/gdb-xfree.h" namespace gdb { diff --git a/gdbsupport/gdbsupport.inc.h b/gdbsupport/gdbsupport.inc.h index ab0999579528..bca15460e87b 100644 --- a/gdbsupport/gdbsupport.inc.h +++ b/gdbsupport/gdbsupport.inc.h @@ -33,3 +33,152 @@ #undef PACKAGE_STRING #undef PACKAGE_TARNAME #undef PACKAGE_VERSION + +/* From: + https://www.gnu.org/software/gnulib/manual/html_node/stdint_002eh.html + + "On some hosts that predate C++11, when using C++ one must define + __STDC_CONSTANT_MACROS to make visible the definitions of constant + macros such as INTMAX_C, and one must define __STDC_LIMIT_MACROS to + make visible the definitions of limit macros such as INTMAX_MAX.". + + And: + https://www.gnu.org/software/gnulib/manual/html_node/inttypes_002eh.html + + "On some hosts that predate C++11, when using C++ one must define + __STDC_FORMAT_MACROS to make visible the declarations of format + macros such as PRIdMAX." + + Must do this before including any system header, since other system + headers may include stdint.h/inttypes.h. */ +#define __STDC_CONSTANT_MACROS 1 +#define __STDC_LIMIT_MACROS 1 +#define __STDC_FORMAT_MACROS 1 + +/* Some distros enable _FORTIFY_SOURCE by default, which on occasion + has caused build failures with -Wunused-result when a patch is + developed on a distro that does not enable _FORTIFY_SOURCE. We + enable it here in order to try to catch these problems earlier; + plus this seems like a reasonable safety measure. The check for + optimization is required because _FORTIFY_SOURCE only works when + optimization is enabled. If _FORTIFY_SOURCE is already defined, + then we don't do anything. Also, on MinGW, fortify requires + linking to -lssp, and to avoid the hassle of checking for + that and linking to it statically, we just don't define + _FORTIFY_SOURCE there. */ + +#if (!defined _FORTIFY_SOURCE && defined __OPTIMIZE__ && __OPTIMIZE__ > 0 \ + && !defined(__MINGW32__)) +#define _FORTIFY_SOURCE 2 +#endif + +/* We don't support Windows versions before XP, so we define + _WIN32_WINNT correspondingly to ensure the Windows API headers + expose the required symbols. + + NOTE: this must be kept in sync with common.m4. */ +#if defined (__MINGW32__) || defined (__CYGWIN__) +# ifdef _WIN32_WINNT +# if _WIN32_WINNT < 0x0501 +# undef _WIN32_WINNT +# define _WIN32_WINNT 0x0501 +# endif +# else +# define _WIN32_WINNT 0x0501 +# endif +#endif /* __MINGW32__ || __CYGWIN__ */ + +#include "ansidecl.h" +/* This is defined by ansidecl.h, but we prefer gnulib's version. On + 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_STANDARD + is compatible with ATTRIBUTE_PRINTF, simply use it. */ +#undef ATTRIBUTE_PRINTF +#define ATTRIBUTE_PRINTF _GL_ATTRIBUTE_FORMAT_PRINTF_STANDARD + +/* This is defined by ansidecl.h, but we disable the attribute. + + Say a developer starts out with: + ... + extern void foo (void *ptr) __attribute__((nonnull (1))); + void foo (void *ptr) {} + ... + with the idea in mind to catch: + ... + foo (nullptr); + ... + at compile time with -Werror=nonnull, and then adds: + ... + void foo (void *ptr) { + + gdb_assert (ptr != nullptr); + } + ... + to catch: + ... + foo (variable_with_nullptr_value); + ... + at runtime as well. + + Said developer then verifies that the assert works (using -O0), and commits + the code. + + Some other developer then checks out the code and accidentally writes some + variant of: + ... + foo (variable_with_nullptr_value); + ... + and builds with -O2, and ... the assert doesn't trigger, because it's + optimized away by gcc. + + There's no suppported recipe to prevent the assertion from being optimized + away (other than: build with -O0, or remove the nonnull attribute). Note + that -fno-delete-null-pointer-checks does not help. A patch was submitted + to improve gcc documentation to point this out more clearly ( + https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ). The + patch also mentions a possible workaround that obfuscates the pointer + using: + ... + void foo (void *ptr) { + + asm ("" : "+r"(ptr)); + gdb_assert (ptr != nullptr); + } + ... + but that still requires the developer to manually add this in all cases + where that's necessary. + + A warning was added to detect the situation: -Wnonnull-compare, which does + help in detecting those cases, but each new gcc release may indicate a new + batch of locations that needs fixing, which means we've added a maintenance + burden. + + We could try to deal with the problem more proactively by introducing a + gdb_assert variant like: + ... + void gdb_assert_non_null (void *ptr) { + asm ("" : "+r"(ptr)); + gdb_assert (ptr != nullptr); + } + void foo (void *ptr) { + gdb_assert_nonnull (ptr); + } + ... + and make it a coding style to use it everywhere, but again, maintenance + burden. + + With all these things considered, for now we go with the solution with the + least maintenance burden: disable the attribute, such that we reliably deal + with it everywhere. */ +#undef ATTRIBUTE_NONNULL +#define ATTRIBUTE_NONNULL(m) + +#include "gdbsupport/poison.h" + +/* sbrk on macOS is not useful for our purposes, since sbrk(0) always + returns the same value. brk/sbrk on macOS is just an emulation + that always returns a pointer to a 4MB section reserved for + that. */ + +#if defined (HAVE_SBRK) && !__APPLE__ +#define HAVE_USEFUL_SBRK 1 +#endif diff --git a/gdbsupport/poison.h b/gdbsupport/poison.h index 7b4f8e8a178d..cb1f474b5c5b 100644 --- a/gdbsupport/poison.h +++ b/gdbsupport/poison.h @@ -20,6 +20,7 @@ #ifndef COMMON_POISON_H #define COMMON_POISON_H +#include "libiberty.h" #include "traits.h" #include "obstack.h" @@ -90,8 +91,23 @@ using IsMallocable = std::is_trivially_constructible; template using IsFreeable = gdb::Or, std::is_void>; -template >>> -void free (T *ptr) = delete; +/* GDB uses this instead of 'free', it detects when it is called on an + invalid type. */ + +template +static void +xfree (T *ptr) +{ + static_assert (IsFreeable::value, "Trying to use xfree with a non-POD \ +data type. Use operator delete instead."); + + if (ptr != NULL) +#ifdef GNULIB_NAMESPACE + GNULIB_NAMESPACE::free (ptr); /* ARI: free */ +#else + free (ptr); /* ARI: free */ +#endif +} template static T *