[PATCH] gdb, configure: Add disable-formats option for configure

[Eli Zaretskii]

> From: Guinevere Larsen <guinevere@redhat.com>
> Cc: Guinevere Larsen <guinevere@redhat.com>
> 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.

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?

> 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.

> +  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.

> +  --enable-formats=FILE_FORMATS
> +                          enable support for selected file formats(default
                                                                    ^^
Space missing there.

> +# 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.

> +	  # 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).

> +    # 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.

> +    # 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.

> --- 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.

> +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".

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?

Maybe I'm confused, but if I am, it means we lack some important
information in the manual which would clarify this.

Thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>