From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id N94OCCWl9WbrNDgAWB0awg (envelope-from ) for ; Thu, 26 Sep 2024 14:17:09 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Y3edruTG; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id F29C91E353; Thu, 26 Sep 2024 14:17:08 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-10.1 required=5.0 tests=ARC_SIGNED,ARC_VALID, BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED, RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE,URIBL_BLOCKED, URIBL_DBL_BLOCKED_OPENDNS autolearn=ham autolearn_force=no version=4.0.0 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 CAA751E05C for ; Thu, 26 Sep 2024 14:17:06 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 43A65385841C for ; Thu, 26 Sep 2024 18:17:06 +0000 (GMT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 647F33858D33 for ; Thu, 26 Sep 2024 18:16:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 647F33858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 647F33858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727374604; cv=none; b=UXc6t8LI484XDd1eqnBQkUn9CuOW7AP9+wbd0mTIjwkz4HYtHARk8cncGgazl3B4w/dYDd2sXgsVnCYtk+fkjuM3lVCVdvc+k4Fx+WRVQYhmmc5dQr+uWPKyKDKKki9+MCHDvQooSt/CcMrC20vIdZW+fRWMdxU+GYEC0vz7ll0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727374604; c=relaxed/simple; bh=xVCyonjPWdshemGsbRTputrRV6+z4C9u/+1lHueGU1o=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=bV/C16h0SCVrilBh7PE+eOV23aDUAHbGHl3jn9buC/8HznPPzA5wTnt8nyUh9XMLXlKyyjfLv602T08gDZ5/PD9viKtvDI5n7sGN5ks+7XHDN6dUNjIjb3omdgZ8h+mDcxEQ504yJS7+iyOgMpZvGFblmvqsZFCaIgD7qN/gNrQ= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1727374602; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=OaPYK71vYrzKlMroGVQHYWbIa6sQ0dhUf6lDjIJ5GFg=; b=Y3edruTGjOPabeP9cEysrGimIGe9ngxzbK6CSfSH7qSCnNYLkT+j/sUCy/jbb35AxzC1Q3 ALTc85BkcfODb1wqCDWoi7nL8M1ziRXH+73+jdpeCnroqyKel0HorBOnJMNg9m6b2Twe/9 Ib1RAZfrPpiWh8qlEZULJs0b5n00Vkw= Received: from mail-oi1-f199.google.com (mail-oi1-f199.google.com [209.85.167.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-504-87YD_hfnMP2t3MegYHRkDg-1; Thu, 26 Sep 2024 14:16:41 -0400 X-MC-Unique: 87YD_hfnMP2t3MegYHRkDg-1 Received: by mail-oi1-f199.google.com with SMTP id 5614622812f47-3dee94f0dcdso1078937b6e.3 for ; Thu, 26 Sep 2024 11:16:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727374599; x=1727979399; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=OaPYK71vYrzKlMroGVQHYWbIa6sQ0dhUf6lDjIJ5GFg=; b=TRQz/dA6HqYJQ6pOBciHVghSnGoh6PDFJLUugtv5dTGdd4PFYnj9xqSlODSMd0JEeg pAGvGms23RII2XO2Ro1uMY1zquCQ+yhi5S+ZUUv2pBNxJ0bz4zwe74n1lZwv8Za4X+WZ Hg4iukl29oRSUGUebL1XZDE3UZwa4a/2DMMwPEjSm8VSh3R6t3mb00/nJ2oF6rPL5ARI OlPHA/Rz2AyXAPYXA3O9HbsB9kB9O2NNrTcssZVW2L5sjiSJbaNmvVKc/wsrwkIgvrPI DqG4g8DPPDI6ShhxVUJ6zzn9iMFfbvlaTdNjTAN1lrAHNPX22+PIpRtrxBWUQljY02vh UW4Q== X-Gm-Message-State: AOJu0YxdTSiSK1pszKSuq0seHLhJgwpYbCfa0vAo6PEOHAl82FD8odPy 91DDBFFAUXgDKj85mBbn7e60eRgzV2f3B9ag12LHrEg4TQmRJvWFldnr2uSh5gVxKc4qwKcPDA8 /lUxhQ0s/MryUC4QRmQyACcuRy5GmFdi3FI2ehHTioZt4HV4sSm7QyEgvAL4wS6c4HH0= X-Received: by 2002:a05:6358:481e:b0:1b5:f973:1e9d with SMTP id e5c5f4694b2df-1becbd0a205mr73268255d.19.1727374599158; Thu, 26 Sep 2024 11:16:39 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHOkTQUi+74hIZyT9aGXJuc9uFpOnMBiYzkiQo6DIFt+bBGOX4rLHvJWi8mvyOTXs0doPnIbA== X-Received: by 2002:a05:6358:481e:b0:1b5:f973:1e9d with SMTP id e5c5f4694b2df-1becbd0a205mr73266355d.19.1727374598813; Thu, 26 Sep 2024 11:16:38 -0700 (PDT) Received: from ?IPV6:2804:14d:8084:92c5::1000? ([2804:14d:8084:92c5::1000]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-7e6db2c8af2sm206361a12.48.2024.09.26.11.16.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Sep 2024 11:16:38 -0700 (PDT) Message-ID: Date: Thu, 26 Sep 2024 15:16:34 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] gdb, configure: Add disable-formats option for configure To: Eli Zaretskii Cc: gdb-patches@sourceware.org References: <20240925175340.1850969-1-guinevere@redhat.com> <86msjvars2.fsf@gnu.org> From: Guinevere Larsen In-Reply-To: <86msjvars2.fsf@gnu.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 9/26/24 2:49 AM, Eli Zaretskii wrote: >> From: Guinevere Larsen >> Cc: Guinevere Larsen >> Date: Wed, 25 Sep 2024 14:53:40 -0300 >> >> GDB has support for many file formats, some which might be very unlikely >> to be found in some situations (such as the COFF format in linux). This >> commit introduces the option for a user to choose which formats GDB will >> support at build configuration time. >> >> This is especially useful to avoid possible security concerns with >> readers that aren't expected to be used at all, as they are one of >> the simplest vectors for an attacker to try and hit GDB with. This >> change also can reduce the size of the final binary, if that is a >> concern. >> >> This commit adds a switch to the configure script allowing a user to >> only enable selected file formats. The default behavior when the switch >> is omitted is to compile all file formats, keeping the original behavior >> of the script. > I always thought that the reason we compile into GDB all the available > readers is to enable remote debugging via gdbserver. If I'm right, > then using this option you propose will produce GDB that is unable to > remote-debug targets which use the omitted formats. This fact should > at least be prominently explained in the documentation, because users > might not realize they shoot themselves in the foot. If I understood what Simon said in IRC correctly, gdb would still be able to do basic debugging, like setting breakpoints in memory addresses, stopping/continuing threads, poking memory, even if we don't have knowledge of the file format. Also, if we haven't compiled in, for example, windows-tdep, GDB already has severe issues connecting to gdbserver, like not being able to get registers correctly, or mishandling system calls, so I think not knowing the file format is the smaller issue in this case. > > I might be confused about this aspect, though; see below. > >> When enabling selected readers, the configure script will also look at >> the selected targets and may choose to add some readers that are the >> default - or only - format available for the target. If that happens, >> the script will emit the following warning: >> >> FOO is required to support one or more targets requested. Adding it >> >> Users aren't able to force the disabling of those formats, since tdep >> files may directly call functions from the required readers. > I think you only consider targets for native and cross-debugging here; > see above. > >> +# All files that relate to GDB's ability to read debug information. >> +# Used with --enable-formats=all. >> +ALL_FORMAT_OBS = \ >> + coff-pe-read.o \ >> + coffread.o \ >> + dbxread.o \ >> + mipsread.o \ >> + xcoffread.o > What about elfread.o, mdebugread, and machoread.o? elfread and machoread were indeed forgotten, but they require some special handling. They can only be added if support is also being compiled on BFD. I'll add a comment here explaining and fix the handling later. mdebugread is not a file format reader, it is a debug format reader. It will be handled in the future, along with dwarf and stabs and such. > >> diff --git a/gdb/NEWS b/gdb/NEWS >> index cfc9cb05f77..8d127558a1d 100644 >> --- a/gdb/NEWS >> +++ b/gdb/NEWS >> @@ -92,6 +92,17 @@ vFile:stat >> vFile:fstat but takes a filename rather than an open file >> descriptor. >> >> +* Configure changes >> + >> +enable-formats=[FORMAT,]... >> +enable-formats=all >> + A user can now decide to only compile support for certain file formats. > I think "file format" is too generic a term to be a good name for this > option. I think something like "objfile format" or maybe "binary file > format", while being longer, are more focused, and thus better. I'll go with objfile format, then. IMO, binary file format is too long. > >> + The available formats at this point are: dbx, coff, xcoff, elf, mach-o >> + and mips. Some targets require specific file formats to be available, >> + and in such cases, the configure script will warn the user and add >> + support anyway. By default, all formats will be compiled in, to >> + continue the behavior from before adding the switch. > This part is otherwise okay English-wise, but see my basic concern > above regarding remote debugging. > >> --- a/gdb/README >> +++ b/gdb/README >> @@ -403,6 +403,11 @@ more obscure GDB `configure' options are not listed here. >> specified list of targets. The special value `all' configures >> GDB for debugging programs running on any target it supports. >> >> +`--enable-formats=FORMAT,FORMAT,...' >> +`--enable-formats=all` > ^^^^^^^^^^^^^^^^^^^^^^ > Please don't quote `like this`: it's a markdown-style quoting we don't > use in our documentation. All other options in the README file are formatted like this. I think it would be odd if only this option didn't this markdown-style, even if it is unique to that file. > >> + --enable-formats=FILE_FORMATS >> + enable support for selected file formats(default > ^^ > Space missing there. Fixed > >> +# File formats that will be enabled based on the selected >> +# target(s). These are chosen because most, if not all, executables for > ^^ > GNU standards use US English convention of leaving 2 spaces between > sentences. Fixed > >> + # Decide which file formats are absolutely required based on >> + # the requested targets. Warn later that they are added, in >> + # case the user manually requested them, or requested all. >> + # It's fine to add xcoff multiple times since the loop that >> + # adds it to FORMAT_OBS will ensure that it is only added once. >> + echo $i >> + case "${i}" in >> + *"windows-tdep.o" ) target_formats="${target_formats} coff";; >> + "linux-tdep.o" ) target_formats="${target_formats} elf";; >> + "rs6000-aix-tdep.o" ) target_formats="${target_formats} xcoff";; >> + "rs6000-lynx178-tdep.o" ) target_formats="${target_formats} xcoff";; >> + "mips-tdep.o" ) target_formats="${target_formats} mips";; >> + esac > This list seems to imply that only non-ELF targets should be in it > (but then why linux-tdep.o is in the list?), because otherwise the > list is way too short; there are a lot more *-tdep.o files that are > built for various platforms, just look what "ls *-tdep.c" produces. > If indeed this mentions only non-ELF targets, that should be mentioned > in the comment. Also, this misses at least i386-go32-tdep.o (which > needs coff). The list is meant to be "if these tdep files are included, compilation will fail". I just tested locally and i386-go32-tdep.o compiles just fine without coffread.c. I guess I should describe the list (and reasons to add file format) more in terms of compilation failing than in terms of what formats are available for a target. I will also do more testing, to make sure I don't accidentally forget to add something and have the compilation fail. > >> + # Formats that are required by some requested target(s). >> + # Warn users that they are added, so no one is surprised. >> + for req in $target_formats; do >> + if ! echo "$enable_formats" | grep -wq "$req"; then >> + echo "$req is required to support one or more targets requested. Adding it" > ^^ > Two spaces are needed there. fixed > >> + # Despite the naming convention implying coff-pe to be a separate >> + # reader, it is in fact just a helper for coffread; >> + coff) FORMAT_OBS="$FORMAT_OBS coffread.o coff-pe-read.o" ;; > But the DJGPP (a.k.a. "go32") native target needs only coff, it > doesn't need coff-pe. Right, but I don't think it makes sense to have a separate option for coff-pe when it requires coff to function. I think it would end up making stuff pretty confusing to have only one format that has a dependency, when all formats are not listed, and so there's no way to annotate that one in specific. > >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -41177,6 +41177,13 @@ Configure @value{GDBN} for cross-debugging programs running on the >> specified list of targets. The special value @samp{all} configures >> @value{GDBN} for debugging programs running on any target it supports. >> >> +@item --enable-formats=@r{[}@var{format}@r{]}@dots{} >> +@itemx --enable-formats=all > See above about the too-generic name of the option. > >> +Configure @value{GDBN} to support certain binary file formats. If a > ^^^^^^^^^^^^^^^^^^^ > Here you use the correct, much more focused, terminology, but IMO the > configure option should also use it. > >> +format is the main (or only) file format for a given target, the > What is a "given target" in this context? This should be explained, > because it is not clear from the surrounding context. And see below > regarding the general confusion this "targets" business causes. I'll try to think of a better description in the v2. > >> +configure script may add support to it anyway, and warn the user. >> +If not given, all file formats that @value{GDBN} supports are compiled. > ^^^^^^^^ > I'd say "compiled in". Fixed > > And I'm a bit confused by this whole issue and its relation to "GDB > targets". The documentation of --target and --enable-targets options > to the configure script says: > > '--target=TARGET' > Configure GDB for cross-debugging programs running on the specified > TARGET. Without this option, GDB is configured to debug programs > that run on the same machine (HOST) as GDB itself. > [...] > '--enable-targets=[TARGET]...' > '--enable-targets=all' > Configure GDB for cross-debugging programs running on the specified > list of targets. The special value 'all' configures GDB for > debugging programs running on any target it supports. > > First, this doesn't say what is the default if --enable-targets is not > specified; I think we should add that. More importantly, it says > "cross-debugging", not "remote debugging", and my reading of > configure.ac matches that: this option affects the value of > TARGET_OBS, which is the list of target-specific files needed for > debugging by GDB itself, without the help of gdbserver. So using the > value of --enable-targets= for the purpose of deciding which readers > will be compiled into GDB seems to be wrong, because when a target is > debugged remotely via gdbserver, it is my understanding that the > reader of the target's binary file format is needed in GDB itself, no? Yes, the client needs to understand the file format, but GDB already does this partial format support only considering which targets are requested. xcoff is only compiled in if rs6000-aix-tdep.o is compiled in. Also, we only compile Mach-O and ELF support if BFD is has support for those. My patch just makes it more controllable by the user, and loudly warns of new files being added. And as I said at the beginning, GDB already can't properly work with gdbservers if the target where the server is running is not compiled in, I don't think this is making things worse in any significant way. With all this said, I think instead of using enabled formats, I should iterate through TARGET_OBS to define, so that if --target is specified, or nothing was given, we'll still find the correct file formats. > > Maybe I'm confused, but if I am, it means we lack some important > information in the manual which would clarify this. Maybe more clarity would be helpful, but I don't think these issues have to do with my patch itself, so I think this should be further improvement for documentation rather than having to fix it in this patch. > > Thanks. > > Reviewed-By: Eli Zaretskii > The  review tag is meant to be used if you are OK with the patch, maybe with a few tweaks (like the obvious fixes you pointed out to me of missing spaces and stuff). It's meant to be used to say "review is done from this email on" even if conversation about future improvements continues happening. Your large scale "target" questions make me think that this wasn't a straightforward thing to fix, so I think this should be held until you're happy with the conversation around the definition of target and so on -- Cheers, Guinevere Larsen She/Her/Hers