From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id yDd4GSkt/GVAQxAAWB0awg (envelope-from ) for ; Thu, 21 Mar 2024 08:50:49 -0400 Received: by simark.ca (Postfix, from userid 112) id 62C961E0C0; Thu, 21 Mar 2024 08:50:49 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (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 ESMTPS id 5166F1E030 for ; Thu, 21 Mar 2024 08:50:47 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F2CB33858D28 for ; Thu, 21 Mar 2024 12:50:46 +0000 (GMT) Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) by sourceware.org (Postfix) with ESMTPS id 8E1DB3858D28 for ; Thu, 21 Mar 2024 12:50:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8E1DB3858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8E1DB3858D28 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.167.43 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711025427; cv=none; b=S3METDR2n50+chIf3B5uTzlkeHNbJ6KKXcvuH9Re0I1tLqB6fEfQgt3Di0qbz0JGzA8O8v+CLDUX03qmIKpZU3gS130CBXCeP621k+C/RUcmxWHq65oplcMwQWqkXczk3cqO5IxpNg1k9YMt0SIB6p3mCT0c4YSGdl5BR370fTI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711025427; c=relaxed/simple; bh=agq0GoGVINHIgVPSd8bhDVmEihnZOXxOdthhM0LxncM=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=Rk1Mtv9/d+rgC/1VWKe4ay6JzOJnmKop8bB0lB+xMNO2tvgB4ehEhNkYu0mo0QoZQz87RhrgTgKhGdWXRtOHnQx7qURpR0K7xMwS9+i1wG6LLfR+DATMxKwRpI3WUvkdno7Xn72ys0zDHI4U9ThPccK/9ovy3CD9Yur3bzofxgo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lf1-f43.google.com with SMTP id 2adb3069b0e04-512e39226efso805521e87.0 for ; Thu, 21 Mar 2024 05:50:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711025424; x=1711630224; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=CE4xI4AoLvTXJxNtZLxnCuSl3ROUZavIXri8gDxIp5s=; b=eXh7/niD/k3MrT/DAcnVYBXmVYEDTv0vBfYh3AmLMvkrqIW0EF6LVLwnyW5Y06hPJU qyhABZ4wgf6NmApA3Id+KbeuOyxfMrta0p3Z6QJWkg/0Fi1SzaQIWtt034AWgJ4wFFa7 G3Jb2smlB8JE2aB/6kXuDBvBoynTkM9lmWhllmGpRyj92UsAFN6Z7HdzXuDxM/0HGPxV U9tZErxDG0JkatyyUlDXXxLv5RjoFWdnaFT4o93wlHg4s7cY1/tOMdbloVEp1fQHNxnY 2CCZAA6AYekhgOk5tA3U8LdIy0g7+bT7hFua2QN/1lBYCMX/RiVUfnwNO31Hf10XkYP5 Ww7A== X-Forwarded-Encrypted: i=1; AJvYcCXf0bCZdgRPu8cbcd3t4fyBsX4VIodE0QBxl7mC09+MaeVSa3KnP6i9Wnifii6PYz5x7lBOUSB6WKpXbSjn+7RrHDEXiV4BJSV5GA== X-Gm-Message-State: AOJu0YxJ3WHIwWaCTrzhjHpRYGNTvJAyAGCioj6KDwcSTWCIjvZIwA/N xbpzhdla1bjs9miNdz3ZC14piXDp5B9eni1h8VB5dyFT8IExU3zje6ErD99r X-Google-Smtp-Source: AGHT+IGMzyygThLHPuDAo4v25K+IsjPU1rH3RW5my4V/AYHVARRFdaATX6+g7UQUoeGDepVa0PTvVA== X-Received: by 2002:ac2:4dbb:0:b0:513:ca62:3dce with SMTP id h27-20020ac24dbb000000b00513ca623dcemr15218193lfe.13.1711025423845; Thu, 21 Mar 2024 05:50:23 -0700 (PDT) Received: from ?IPV6:2001:8a0:f918:ab00:5ea7:1bb:7941:5784? ([2001:8a0:f918:ab00:5ea7:1bb:7941:5784]) by smtp.gmail.com with ESMTPSA id fc18-20020a05600c525200b00414105c4cd9sm5491485wmb.21.2024.03.21.05.50.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 21 Mar 2024 05:50:23 -0700 (PDT) Message-ID: Date: Thu, 21 Mar 2024 12:50:19 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/3] gdb, gdbserver, gdbsupport: include config.h files with -include Content-Language: en-US To: Simon Marchi , Simon Marchi , gdb-patches@sourceware.org References: <20240318200257.131199-1-simon.marchi@efficios.com> <20240318200257.131199-2-simon.marchi@efficios.com> <2da78531-8a3a-4ac9-a87c-f4962d573fce@palves.net> <9a146cea-3adf-4365-8eb7-60c65d00dcf4@simark.ca> From: Pedro Alves In-Reply-To: <9a146cea-3adf-4365-8eb7-60c65d00dcf4@simark.ca> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no 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+public-inbox=simark.ca@sourceware.org On 2024-03-21 02:11, Simon Marchi wrote: > On 3/20/24 16:32, Pedro Alves wrote: >> On 2024-03-18 20:01, Simon Marchi wrote: >> Note there is a make rule to check whether gdb headers are standalone. "make check-headers". >> Unfortunately, nobody ever runs that ( me included, after adding it a decade ago :-P ). >> Ideally, we'd fix all it flags and run it (or something like it) once in a while in a CI. >> (And same for gdbserver/gdbsupport, of course.) > > Ah! I probably saw that in the past but forgot about it. It might need > to be changed too, depending on what follows. > >>> 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. >> >> I'm surprised by the actual patch. Why isn't this including defs.h/common-defs.h? There are >> surely things defined in those files that need to be defined in headers too. Why create >> this divergence? I'd think that we would include defs.h/common-defs.h in headers too, and >> then work on moving things out of defs.h/common-defs.h over time. > > I am not sure I understand what you mean. If a given header file uses > something defined in defs.h, then it should include defs.h. Otherwise > it doesn't need to. Maybe if you give a concrete example I will get > your point better. > I think you're looking at this all backwards, TBH. Currently, defs.h (and common-defs.h/server.h...) is a special header, and we need to include it first. We all agree that defs.h includes too much, and that several things in it should be moved to other headers, and included only where they're needed. If we imagine we've already done that, then defs.h is left with only the things that need to be included by all source files, and all headers. That's the end goals that seems super obvious to me. Yet, your patch takes a different route, and creates yet another set of special headers, and moves some things there. It then leaves us in a partial transition state, where it's very likely that some headers needed more things that are included by defs.h, but they aren't included in the new magic header, nor in the said headers that needed the things. I.e., it likely regresses "make check-headers" even further. Some headers use bfd_vma for example. Are they all including libbfd.h explicitly or transitively? I don't know. Currently they get it from defs.h. Some headers make use of "enum block_enum", which is defined in defs.h today, so when you edit such headers you'll will not have that enum defined. Likewise probably other enums in defs.h. We use gdb::array_view in a lot of headers, etc. I really see no reason for the new headers, and switching to a different set of magic headers, and moving things around from defs.h to the new headers. It makes me a bit nervous even, as order of some things in defs.h (and friends) is important. E.g., the GCC_GENERATED_STDINT_H thing. What I'm saying is that the patch that I was expecting to see, was one that simply does one thing -- makes gdb use "-include" to force-include defs.h everywhere (and common-defs.h/server.h, you get the point). It's then just one single, atomic, change. We go from having to put defs.h at the top of source files, to not having to do that because we use "-include defs.h". That's it. Nothing more. So defs.h is still the same special header, just the mechanism by which we include it changes. The code that ends up compiled is _exactly_ the same. Documentation explaining that that is the special header doesn't have to change. And your use case, editing the header, will end up with headers including _exactly_ what they include today when any source file is compiled, not some subset of defs.h. Any oddity with editing the file is an oddity normal compilation would have too. And then, over time, we move things out of defs.h. E.g., move to including array-view.h in headers that need it. Move the definitions of enum block_enum, enum user_selected_what_flag, etc. to some other headers that make more sense. Move the inline functions and function prototypes to other headers. Etc. But we take it from the top down. Ultimately still end up with defs.h, but a smaller defs.h. And at each step of the way, editing a header file always sees the exact same set of pre-included files/symbols as when the same header is compiled normally. Pedro Alves