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

[Guinevere Larsen <guinevere@redhat.com>]

On 8/5/25 1:03 PM, Simon Marchi wrote:
> On 7/7/25 4:16 PM, Guinevere Larsen wrote:
>> GDB has support for many binary file formats, some which might be very
>> unlikely to be found in some situations (such as the XCOFF format in
>> an x86 system). 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 can also 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, called --enable-binary-file-formats.
>> The default behavior when the switch is omitted is to compile all file
>> formats, keeping the original behavior of the script. At the time of
>> this commit, the valid options for this option are: dbx, coff (which
>> includes coff-pe), xcoff, mips, elf, macho and all. All is treated
>> especially, activating all supported readers.
>>
>> A few targets may require specific binary file format support, as they
>> directly call functions defined by the file reader. Specifically,
>> windows targets require coff support, and rs6000 aix and lynx178 targets
>> require xcoff support. Considering that those formats are the main - or
>> only - one available in those targets, I believe it makes sense to
>> re-enable those readers. If that happens, the script will emit the
>> following warning:
>>
>>    FOO is required to support one or more requested targets. Adding it
>>
>> Users aren't able to force the disabling of those formats, since GDB
>> will not compile without those readers. Ideally we'd like to be able
>> to disable even those formats, in case a user wants to build GDB only
>> to examine remote files for example, but the current infrastructure
>> for the file format readers doesn't allow us to do it.
>>
>> Mach-O and elf support are also dependent on BFD support being compiled
>> in.  In case one of those was requested and BFD does not support them,
>> the following error is emitted:
>>
>>      FOO was requested, but BFD does not support it.
>>
>> Finally, this configure switch is also printed by the "show
>> configuration" command in GDB.
>>
>> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>> ---
>>
>> This is rebased on current master, there was a bit of a rebase conflict,
>> but nothing complicated.
> I think the git commit subject needs to be updated to reflect the new
> option name.
Fixed
>
>> diff --git a/gdb/README b/gdb/README
>> index eca4181c751..d45e6d37c4a 100644
>> --- a/gdb/README
>> +++ b/gdb/README
>> @@ -417,6 +417,30 @@ more obscure GDB `configure' options are not listed here.
>>        There is no convenient way to generate a list of all available
>>        targets.
>>   
>> +`--enable-binary-file-formats=FORMAT,FORMAT,...'
>> +`--enable-binary-file-formats=all'
>> +    Configure GDB to only be be able to read selected file formats.
>> +    The special value "all" causes all file formats to be compiled
>> +    in, and is the the default behavior of the option.  This option
>> +    is meant for advanced users who are sure of what they expect,
>> +    if you are unsure which options you will need on your debugging
>> +    sessions, we recommend that you not use this feature.  The
>> +    accepted options are:
> In the list below, a few instances of "two spaces after period".
For consistency, I ended up changing all periods to commas, so this 
ended up being a non-issue
>
>> +      * coff: Main format on windows systems. This is required to
>> +        compile with windows target support;
> windows -> Windows
>
>> +      * dbx (also known as a.out): is a legacy file format, this
>> +        is recommended if you know you will be dealing with this
>> +        file format;
> Missing "This" at the beginning, to be like the other entries.

For consistency, I actually changed this to just say

"Legacy file format, this is (...)"

Since the other options will all read "Main format on (...)".

>
>> +      * elf: Main format of linux systems. This is heavily
>> +        recommended when compiling with linux support;
> linux -> Linux
>
> For consistency: in all entries except this one, the "This is heavily"
> is just a phrase following a comma.  I would change this one to match.
> In general if you can make the form consistent across items, it would be
> nice.
>
>> +      * macho: Main format on MacOS systems, this is heavily
>> +        recommended when compiling for those targets;
> MacOS -> macOS
>
>> @@ -191,6 +191,15 @@ AS_HELP_STRING([--enable-targets=TARGETS], [alternative target configurations]),
>>   	;;
>>   esac])
>>   
>> +AC_ARG_ENABLE(binary_file_formats,
>> +	      AS_HELP_STRING([--enable-binary-file-formats=FILE_FORMATS],
> Question mostly for Eli, should the metavar use an underscore or dash
> ("FILE-FORMATS")?
I saw Eli's response, I'll change this to FORMATS.
>
>> +			     [enable support for selected file formats (default 'all')]),
> Since the list of valide values is finite and not too long, it might be
> nice to list the possible values right there in the help message.

done. This now reads as:

enable support for selected file formats (default 'all')
available formats: coff, dbx, elf, macho, mips, xcoff

>
>> +[case "${enableval}" in
>> + yes | "") AC_MSG_ERROR(enable-formats option must specify file formats or 'all')
> enable-formats -> enable-binary-file-formats
>
> I think you are also missing a space before "yes".
done both
>
>> @@ -239,11 +248,20 @@ fi
>>   TARGET_OBS=
>>   all_targets=
>>   HAVE_NATIVE_GCORE_TARGET=
>> +# File formats that will be enabled based on the selected
>> +# target(s).  These are chosen because they are required to
>> +# compile one or more of the selected targets.
>> +target_formats=
>> +# If all targets were requested, this is all formats that should
>> +# accompany them.  These are just the ones required for compilation
>> +# to succeed, not the formats suggested based on targets.
>> +all_target_formats="coff xcoff"
> Please add empty lines before each comment above.
done
>
>> @@ -1952,11 +1970,17 @@ fi
>>   # Note that WIN32APILIBS is set by GDB_AC_COMMON.
>>   WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>>   
>> +# Object files to be used when building with support for all file formats.
>> +# This should not have elf or macho, as support for those formats depends
>> +# on BFD enabling them as well.
>> +all_binary_file_srcs="\$(dbx_SRCS) \$(mips_SRCS) \$(coff_SRCS) \$(xcoff_SRCS)"
>> +
>> +support_elf=no
>>   # Add ELF support to GDB, but only if BFD includes ELF support.
>>   GDB_AC_CHECK_BFD([for ELF support in BFD], gdb_cv_var_elf,
>>                    [bfd_get_elf_phdr_upper_bound (NULL)], elf-bfd.h)
>>   if test "$gdb_cv_var_elf" = yes; then
>> -  CONFIG_OBS="$CONFIG_OBS elfread.o stap-probe.o dtrace-probe.o \
>> +  CONFIG_OBS="$CONFIG_OBS stap-probe.o dtrace-probe.o \
>>   		gcore-elf.o elf-none-tdep.o"
> Does it make sense (or is it useful) to compile these other files (other
> than elfread.o) if ELF support is not enabled?  Could we just list them
> all in elf_SRCS so that they are not built if the user doesn't want ELF
> support?  If you don't have an ELF reader.

I don't know. I took the path of least changes to implementing this, 
made worse by me not having a convenient way to test a non-ELF target.

I'll try to find a way to test if those are necessary and update the 
patch if they aren't

> If I understand correctly, "support_elf" means that BFD supports ELF,
> not that GDB will support ELF, right?  If so, could you rename it to
> "bfd_supports_elf"?
yes, you're correct. I'll update the name
> I also wouldn't mind if you renamed the conf var gdb_cv_var_elf to
> something like gdb_cv_var_bfd_elf.
Sure, might as well.
>>     AC_DEFINE(HAVE_ELF, 1,
>>   	    [Define if ELF support should be included.])
>> @@ -1964,15 +1988,67 @@ if test "$gdb_cv_var_elf" = yes; then
>>     if test "$plugins" = "yes"; then
>>       AC_SEARCH_LIBS(dlopen, dl)
>>     fi
>> +  support_elf=yes
>> +  all_binary_file_srcs="$all_binary_file_srcs \$(elf_SRCS)"
>>   fi
>>   
>>   # Add macho support to GDB, but only if BFD includes it.
>> +support_macho=no
> Likewise for support_macho.
done
>
>>   GDB_AC_CHECK_BFD([for Mach-O support in BFD], gdb_cv_var_macho,
>>                    [bfd_mach_o_lookup_command (NULL, 0, NULL)], mach-o.h)
>>   if test "$gdb_cv_var_macho" = yes; then
>> -  CONFIG_OBS="$CONFIG_OBS machoread.o"
>> +  support_macho=yes
>> +  all_binary_file_srcs="$all_binary_file_srcs \$(macho_SRCS)"
>> +fi
>> +
>> +FORMAT_SRCS=
>> +
>> +if test "$enable_binary_file_formats" != "all"; then
>> +    # 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
>> +	case ,$enable_binary_file_formats, in
>> +	*,$req,*)
>> +	    # Do nothing.
>> +	    ;;
> Does this work if $req is first or last in the list?
yes, because the case checks for ",$enable_binary_file_formats,", so 
even if there is only one option the commas will be there.
>
>> +	*)
>> +	    AC_MSG_WARN("$req is required to support one or more requested targets.  Adding it")
>> +	    enable_binary_file_formats="${enable_binary_file_formats},$req"
>> +	    ;;
>> +	esac
>> +    done
>> +
>> +    AC_DEFINE_UNQUOTED(SUPPORTED_FORMATS, "$enable_binary_file_formats",
>> +		       Which file formats were requested at configure time. )
> I think that all variables related to this should be renamed to include
> "binary file".  For instance, SUPPORTED_FORMATS ->
> SUPPORTED_BINARY_FILE_FORMATS.  There are a few others in the configure
> script or Makefile.
>
> Also, "Which file formats" -> "Which binary file formats".
Alright, fixed.
>
>>   fi
>>   
>> +enable_binary_file_formats=$(echo $enable_binary_file_formats | sed 's/,/ /g')
>> +
>> +for format in $enable_binary_file_formats
>> +do
> Can you write a comment above that loop to indicate what it does?

sure, added a small comment like so:

     Go through all requested and required binary file formats to compile
     GDB, and double check that we can compile them.

>> +    if test "$format" = "elf"; then
>> +	if test "$support_elf" != "yes"; then
>> +	    AC_MSG_ERROR("elf support was requested, but BFD does not support it.");
>> +	fi
>> +    elif test "$format" = "macho"; then
>> +	if test "$support_macho" != "yes"; then
>> +	    AC_MSG_ERROR("Mach-O support was requested, but BFD does not support it.");
>> +	fi
>> +    fi
> Can you write the above as
>
>      if test "$format" = "elf" && "$support_elf" != "yes"; then
> 	AC_MSG_ERROR("elf support was requested, but BFD does not support it.");
>      fi
>
>      if test "$format" = "macho" && "$support_macho" != "yes"; then
> 	AC_MSG_ERROR("Mach-O support was requested, but BFD does not support it.");
>      fi
>
> ?  I find it a bit easier to read (less boilerplate).
Sure, no problem. I don't really know or enjoy bash, so I go with 
whatever is easiest or I already know, so I wasn't sure this was viable.
>
> Simon
>

-- 
Cheers,
Guinevere Larsen
She/it