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 *