* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-25 17:53 [PATCH] gdb, configure: Add disable-formats option for configure Guinevere Larsen
@ 2024-09-26 5:49 ` Eli Zaretskii
2024-09-26 18:16 ` Guinevere Larsen
2024-09-26 19:18 ` Tom Tromey
` (2 subsequent siblings)
3 siblings, 1 reply; 30+ messages in thread
From: Eli Zaretskii @ 2024-09-26 5:49 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: gdb-patches
> 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>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-26 5:49 ` Eli Zaretskii
@ 2024-09-26 18:16 ` Guinevere Larsen
2024-09-26 18:35 ` Eli Zaretskii
0 siblings, 1 reply; 30+ messages in thread
From: Guinevere Larsen @ 2024-09-26 18:16 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
On 9/26/24 2:49 AM, Eli Zaretskii wrote:
>> 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.
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 <eliz@gnu.org>
>
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-26 18:16 ` Guinevere Larsen
@ 2024-09-26 18:35 ` Eli Zaretskii
2024-09-26 21:03 ` Guinevere Larsen
0 siblings, 1 reply; 30+ messages in thread
From: Eli Zaretskii @ 2024-09-26 18:35 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: gdb-patches
> Date: Thu, 26 Sep 2024 15:16:34 -0300
> Cc: gdb-patches@sourceware.org
> From: Guinevere Larsen <guinevere@redhat.com>
>
> >> +`--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.
No, they use quoting `like this', not `like this`.
> >> + *"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 thought this is about having in the built GDB support for the
necessary binary file format, not about compilation failures. What
good is a GDB if it cannot access the binary file format used by the
target?
> >> + # 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.
So what's the solution? forcing coff-pe to be compiled by the DJGPP
port?
> > '--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.
My point was that the manual doesn't clarify these issues. It left me
confused.
> > 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.
You may be right, but without clarifying this whole issue I cannot
decide whether your additions about this are okay or not. So I think
we have no choice but to clarify those other aspects together with
this review, even if just in principle.
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> >
> 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).
Which is what happened here. My main reservations are not about the
documentation, but about the larger picture, where I don't have a
decisive word anyway.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-26 18:35 ` Eli Zaretskii
@ 2024-09-26 21:03 ` Guinevere Larsen
2024-09-27 6:05 ` Eli Zaretskii
0 siblings, 1 reply; 30+ messages in thread
From: Guinevere Larsen @ 2024-09-26 21:03 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
On 9/26/24 3:35 PM, Eli Zaretskii wrote:
>> Date: Thu, 26 Sep 2024 15:16:34 -0300
>> Cc: gdb-patches@sourceware.org
>> From: Guinevere Larsen <guinevere@redhat.com>
>>
>>>> +`--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.
> No, they use quoting `like this', not `like this`.
Oh sorry, I missed that, I'll fix the formatting. And I guess we have to
fix --enable-tagets=all at some point.
>
>>>> + *"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 thought this is about having in the built GDB support for the
> necessary binary file format, not about compilation failures.
I did describe it from the point of view of functionality, but the
technical reason for it was compilation problems. I will change the
explanation on the next version to be more straight forward.
> What
> good is a GDB if it cannot access the binary file format used by the
> target?
The GDB may have been built primarily for remote debugging a different
target... And even locally, like I said, you can still debug as an
unstructured collection of bytes, setting breaks on addresses, pausing
and continuing, so it's not like it is useless. Plus, this will only
affect users that actively decide to not compile support, if they ignore
the flag everything will work as it used to.
When chatting on IRC, Simon mentioned that ideally we'd be able to not
compile any formats, and give the user full control. If they wish to
shoot their own foot off, who are we to know better. Most users will
probably just accept whatever their distribution gives them, and I would
guess most people building their own wouldn't mess with this flag, I
think this is pretty low impact.
>
>>>> + # 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.
> So what's the solution? forcing coff-pe to be compiled by the DJGPP
> port?
That would be my suggestion, yes. If you have a good suggestion for a
way to have coff-pe be a switch, that is obviously dependent on coff and
still obviously an objfile format, I'm all ears!
Also, worthy of note, this is still an improvement for a security
conscious DJGPP user, since currently we compile coff-pe in along with
every other file format reader, so they'll still get a bit of
unnecessary cruft, but not nearly as much as before this change.
>
>>> '--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.
> My point was that the manual doesn't clarify these issues. It left me
> confused.
>
>>> 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.
> You may be right, but without clarifying this whole issue I cannot
> decide whether your additions about this are okay or not. So I think
> we have no choice but to clarify those other aspects together with
> this review, even if just in principle.
I hope I clarified enough in this email to allow you to judge my changes
on their own, and a later unrelated patch can fill in the missing
information, since the changes really aren't related to enable-targets
and it was my mistake to parse them there instead of just seeing which
tdep files are compiled in and need a file format.
>
>>> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>>>
>> 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).
> Which is what happened here. My main reservations are not about the
> documentation, but about the larger picture, where I don't have a
> decisive word anyway.
>
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-26 21:03 ` Guinevere Larsen
@ 2024-09-27 6:05 ` Eli Zaretskii
2024-10-02 13:25 ` Andrew Burgess
0 siblings, 1 reply; 30+ messages in thread
From: Eli Zaretskii @ 2024-09-27 6:05 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: gdb-patches
> Date: Thu, 26 Sep 2024 18:03:05 -0300
> Cc: gdb-patches@sourceware.org
> From: Guinevere Larsen <guinevere@redhat.com>
>
> > My point was that the manual doesn't clarify these issues. It left me
> > confused.
> >
> >>> 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.
> > You may be right, but without clarifying this whole issue I cannot
> > decide whether your additions about this are okay or not. So I think
> > we have no choice but to clarify those other aspects together with
> > this review, even if just in principle.
> I hope I clarified enough in this email to allow you to judge my changes
> on their own, and a later unrelated patch can fill in the missing
> information, since the changes really aren't related to enable-targets
> and it was my mistake to parse them there instead of just seeing which
> tdep files are compiled in and need a file format.
Unfortunately, I'm still in the dark here. I'd appreciate if you or
someone else who understands the way we deal with targets in all the 3
scenarios -- native debugging, cross-debugging, remote debugging --
could explain how this stuff works (and perhaps how it should work
ideally, if what we have is not ideal/correct), and then we could take
it from there.
We could, of course, just go with your changes disregarding these
larger issues, but I think clarifying them will also help us
understand better this new feature and make sure it is designed and
implemented correctly.
OTOH, if I'm the only one who doesn't fully understand this stuff, and
everyone else agrees with the suggested changes as they are, feel free
to ignore me.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-27 6:05 ` Eli Zaretskii
@ 2024-10-02 13:25 ` Andrew Burgess
2024-10-02 14:15 ` Eli Zaretskii
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Burgess @ 2024-10-02 13:25 UTC (permalink / raw)
To: Eli Zaretskii, Guinevere Larsen; +Cc: gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Thu, 26 Sep 2024 18:03:05 -0300
>> Cc: gdb-patches@sourceware.org
>> From: Guinevere Larsen <guinevere@redhat.com>
>>
>> > My point was that the manual doesn't clarify these issues. It left me
>> > confused.
>> >
>> >>> 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.
>> > You may be right, but without clarifying this whole issue I cannot
>> > decide whether your additions about this are okay or not. So I think
>> > we have no choice but to clarify those other aspects together with
>> > this review, even if just in principle.
>> I hope I clarified enough in this email to allow you to judge my changes
>> on their own, and a later unrelated patch can fill in the missing
>> information, since the changes really aren't related to enable-targets
>> and it was my mistake to parse them there instead of just seeing which
>> tdep files are compiled in and need a file format.
>
Hi Eli,
I was catching up on this thread and though I could help by expanding
the docs on the --enable-targets and --target flags, but I was confused
by one of your questions below...
> Unfortunately, I'm still in the dark here. I'd appreciate if you or
> someone else who understands the way we deal with targets in all the 3
> scenarios -- native debugging, cross-debugging, remote debugging --
In my mind cross-debugging IS remote debugging, I don't understand what
the distinction would be.
Let's build up some examples to work through why I hold the above
belief. What follows are GDB configure lines and then a description of
what capabilities I think that GDB would have. This ignores the work
Gwen has done, as you say, lets get the docs for --target and
--enable-targets sorted first, then Gwen's work can sit on top of that:
(1) ./configure
This GDB will be able to run on the build host and will support
debugging the build host only. This GDB will support remote
debugging, but only to targets that are the same as the build
host, i.e. same architecture and same OS.
(2) ./configure --target=<1>
Assuming that <1> doesn't match the build host, then this GDB will
be able to run on the build host but will not be able to debug
native processes. In fact the native target support will not even
be compiled into GDB. This GDB will be able to remote debug
inferior's running on a remote host that matches <1>.
(3) ./configure --host=<1>
Throwing this one in just for completion. Assuming that <1> doesn't
match my local build machine, this is going to cross-build GDB to
run on a <1> host machine. GDB will be able to debug native
processes on the <1> host, and GDB will also be able to remote debug
processes on machines that are also of <1> type.
(4) ./configure --host=<1> --target=<2>
Assuming that <1> does not match my local build machine, and that
<2> doesn't match <1>, this is going to cross build GDB to run on
machines of type <1>, and GDB will only be able to remote debug
targets of type <2>. As with case (1) native target support isn't
going to be built into GDB.
What you'll notice in all of the above is that --target only allows a
single target type to be specified. If we want GDB to support multiple
targets then --enable-targets can be used. This option takes a list of
targets which are built into GDB in addition to the --target value. So:
(5) ./configure --enable-targets=<1>,<2>,<3>
Without an explicit --target option this is similar to case (1)
above. GDB will run on the build host and can debug either
natively, or remotely, inferiors running on machines that are the
same as the build host. In addition, assuming <1>, <2>, and <3>
are all different from the build host, GDB will be able to remote
debug targets matching <1>, <2>, and <3>.
Of course, we are free to use `--enable-targets=all' which adds
support for all known targets to GDB. In this case the build host
type is effectively passed twice, once implicitly in the `--target'
option, and then again as a member of `--enable-targets=all', but
that's OK, the configure scripts can cope with this, and we just
build everything into GDB.
One important detail is that there is no real difference from specifying
a target via --target vs via --enable-targets. Consider this case:
(6) ./configure --host=<2> --target=<1> --enable-targets=<2>
This GDB will run on <2> and will be able to debug native processes
on <2>. In addition GDB will be able to remote debug <1> and <2>.
This is equivalent to:
(7) ./configure --host=<2> --enable-targets=<1>
Without an explicit `--target' option the configure script assumes
that the target should match the host, then we add the ability to
also remote debug <1>.
> could explain how this stuff works (and perhaps how it should work
> ideally, if what we have is not ideal/correct), and then we could take
> it from there.
This all seems OK to me. If you agree then I'm happy to try and tighten
up the docs a little to (hopefully) better explain things.
> We could, of course, just go with your changes disregarding these
> larger issues, but I think clarifying them will also help us
> understand better this new feature and make sure it is designed and
> implemented correctly.
I think Gwen's changes are broadly fine (I have some minor nits), but I
agree with you, Eli, that we should try to get the docs into a state
where this makes sense. I know you have a huge amount of experience
with configuring / building GNU software, so if you can't read the docs
and understand this, then it feels like a less experienced user is going
to be even more confused.
> OTOH, if I'm the only one who doesn't fully understand this stuff, and
> everyone else agrees with the suggested changes as they are, feel free
> to ignore me.
I certainly don't think you should be ignored, but I'd like to give my
attempt at explaining the motivation for this work, and why I think this
is OK (once it's documented well enough).
Consider the base gdb package as shipped with Fedora Linux. The
`--target' is set to match the `--host', but in addition we pass
`--enable-targets=' to cover a select few architectures running Linux.
There's no Windows, Solaris, FreeBSD, AIX, or any other OS support built
in which means the GDB will not be able to remote debug any of these
targets in a meaningful way[1]. Fedora just doesn't want to commit to
supporting debug for these targets, and this includes both OS and
architecture choices. And that's OK. There are other packages which
offer builds of GDB for specific other target, for example, there's a
package which offers bare-metal AVR support, which is not something the
base gdb package supports.
However, right now, despite only configuring to support Linux based
targets, Fedora GDB still end up building in support for many different
file formats that we just don't care about, and we'd really like a
configure option that allows distributions to compile out the file
formats that they don't want to support, just as, right now, Fedora
chooses not to support debug on many different OSes.
I hope this helps a little.
If you can give your thoughts on what I wrote about --target and
--enable-targets= above, then I'll try to draft a patch that improves
the docs in this area, then Gwen can update on top of that, which
hopefully will bring some clarity.
Thanks,
Andrew
[1] In some cases it might be possible to connect to a remote target and
perform basic (address based) debugging even if support for the target
OS is not available in GDB. GDB would need to have support for the
target architecture compiled in (which is not always the case), and GDB
still might struggle to access inferior state in some cases. That
anything works in this case is, I feel, more by luck than design, so I
don't really consider this "working".
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-02 13:25 ` Andrew Burgess
@ 2024-10-02 14:15 ` Eli Zaretskii
2024-10-04 14:26 ` Andrew Burgess
0 siblings, 1 reply; 30+ messages in thread
From: Eli Zaretskii @ 2024-10-02 14:15 UTC (permalink / raw)
To: Andrew Burgess; +Cc: guinevere, gdb-patches
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Wed, 02 Oct 2024 14:25:05 +0100
>
> Consider the base gdb package as shipped with Fedora Linux. The
> `--target' is set to match the `--host', but in addition we pass
> `--enable-targets=' to cover a select few architectures running Linux.
> There's no Windows, Solaris, FreeBSD, AIX, or any other OS support built
> in which means the GDB will not be able to remote debug any of these
> targets in a meaningful way[1]. Fedora just doesn't want to commit to
> supporting debug for these targets, and this includes both OS and
> architecture choices. And that's OK. There are other packages which
> offer builds of GDB for specific other target, for example, there's a
> package which offers bare-metal AVR support, which is not something the
> base gdb package supports.
>
> However, right now, despite only configuring to support Linux based
> targets, Fedora GDB still end up building in support for many different
> file formats that we just don't care about, and we'd really like a
> configure option that allows distributions to compile out the file
> formats that they don't want to support, just as, right now, Fedora
> chooses not to support debug on many different OSes.
>
> I hope this helps a little.
It does, thanks. What is still missing is the division of labor
between GDB and gdbserver in the remote-debugging scenario. What I
thought was that gdbserver only needs to know some part of the
target's support, like starting and stopping the program, inserting
breakpoints, etc. Whereas GDB needs to be able to read the debug
info, access and show the source and machine code, etc. Is that true?
If so, then building GDB with support for reading debug info in
various formats will allow the user to connect to a gdbserver that
supports a specific target, even though GDB itself wasn't built with
support for that target (since GDB can connect and talk to a gdbserver
from a different build of GDB). Am I wrong about this?
If I'm right, then there are two separate aspects to "target": on the
one side, the ability to start/stop executables, insert breakpoints
and watchpoints, etc., and OTOH the ability to read and process debug
info used by that target, implement frame unwinders, etc. Are we
currently conflating these into a single notion of "target", and if
so, will the proposed changes include or exclude both parts of each
"target"?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-02 14:15 ` Eli Zaretskii
@ 2024-10-04 14:26 ` Andrew Burgess
2024-10-04 14:45 ` Eli Zaretskii
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Burgess @ 2024-10-04 14:26 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: guinevere, gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Wed, 02 Oct 2024 14:25:05 +0100
>>
>> Consider the base gdb package as shipped with Fedora Linux. The
>> `--target' is set to match the `--host', but in addition we pass
>> `--enable-targets=' to cover a select few architectures running Linux.
>> There's no Windows, Solaris, FreeBSD, AIX, or any other OS support built
>> in which means the GDB will not be able to remote debug any of these
>> targets in a meaningful way[1]. Fedora just doesn't want to commit to
>> supporting debug for these targets, and this includes both OS and
>> architecture choices. And that's OK. There are other packages which
>> offer builds of GDB for specific other target, for example, there's a
>> package which offers bare-metal AVR support, which is not something the
>> base gdb package supports.
>>
>> However, right now, despite only configuring to support Linux based
>> targets, Fedora GDB still end up building in support for many different
>> file formats that we just don't care about, and we'd really like a
>> configure option that allows distributions to compile out the file
>> formats that they don't want to support, just as, right now, Fedora
>> chooses not to support debug on many different OSes.
>>
>> I hope this helps a little.
>
> It does, thanks. What is still missing is the division of labor
> between GDB and gdbserver in the remote-debugging scenario. What I
> thought was that gdbserver only needs to know some part of the
> target's support, like starting and stopping the program, inserting
> breakpoints, etc. Whereas GDB needs to be able to read the debug
> info, access and show the source and machine code, etc. Is that true?
Yes.
gdbserver supports far fewer target OSes than GDB itself, as far as I
can tell it's Linux, netbsd, or Windows.
Unlike GDB, gdbserver only every supports a single target. This makes
sense as gdbserver only ever controls native processes running on the
same machine as gdbserver itself. This is enforced at configure time,
if we configure the binutils-gdb tree with --target not equal to --host
then the gdbserver directory will be disabled, and we don't even attempt
to build gdbserver.
GDB splits target support into *-tdep.c and *-nat.c files. The *-nat.c
file is for lower level, native target stuff, e.g. how do I create a new
process, how do I read the registers.
Then the *-tdep.c files are for higher level target specific stuff,
e.g. setting up platform specific types, registering special frame
unwinders, signal handling details, etc.
gdbserver only needs to include the lower level knowledge, the
equivalent of the *-nat.c files, though on the gdbserver side these
files are called *-low.cc. Some code is shared between GDB and
gdbserver in this area, but not everything. This can, and should be
improved.
But the high level logic, the *-tdep.c files, only live in GDB. When a
target is compiled into GDB this causes the tdep files to get built in.
Which nat files we build in depends on the --host configure option, that
is, where is GDB (the program) going to run.
> If so, then building GDB with support for reading debug info in
> various formats will allow the user to connect to a gdbserver that
> supports a specific target, even though GDB itself wasn't built with
> support for that target (since GDB can connect and talk to a gdbserver
> from a different build of GDB). Am I wrong about this?
The problem is that reading the file is only one part of the process.
The target specific tdep file adds higher level knowledge on top of the
file contents.
I dug out an old windows machine and setup the following debug
environment: on the windows machine I built gdbserver for
x86_64-w64-mingw32, then on an x86-64 Linux machine I built GDB only
with support for x86_64-redhat-linux. The source I was building from
didn't include Gwen's patch, so all file formats are usable.
GDB is able to connect to gdbserver, and can read the executable and
libraries, and can extract the symbols. However, GDB gets the addresses
for all symbols wrong.
The reason for this can be found in windows_init_abi_common where we
call set_gdbarch_so_ops. This call registers some callbacks that are
used whenever GDB opens a library or executable. These callbacks can do
things like fix up the objfile section addresses, which is exactly
what's being done in this case. As the windows_init_abi_common is in
windows-tdep.c, when this file is not compiled in, GDB doesn't really
"understand" the executable file within a Windows context, and so the
sections offsets are left as 0, and GDB gets all the symbol addresses
wrong.
So, I can disassemble instructions using an address and length,
e.g. 'x/10i ADDRESS', or place breakpoints by address 'b *ADDRESS', but
I can't do anything that relies on symbols, e.g. 'b main', or
'disassemble main'.
In addition GDB isn't going to be able to backtrace correctly, skip
function prologues, or any of that other good stuff.
Out of interest, I also tried building GDB with only Linux support, and,
using Gwen's patch, with only ELF file format support. My thinking was
that, if previously GDB could read the file, but couldn't actually do
anything useful with the symbols, then maybe we're no worse off if GDB
just can't read the file at all...
...unfortunately, GDB segfault. I'll report this to Gwen separately.
But in theory, we'd probably be better off if GDB didn't read the
symbols from the file, given that GDB can't then figure out the correct
addresses.
> If I'm right, then there are two separate aspects to "target": on the
> one side, the ability to start/stop executables, insert breakpoints
> and watchpoints, etc., and OTOH the ability to read and process debug
> info used by that target, implement frame unwinders, etc. Are we
> currently conflating these into a single notion of "target", and if
> so, will the proposed changes include or exclude both parts of each
> "target"?
I agree with you about "target" having two components, the lower level
stuff, and the higher level stuff.
File format support, being able to read from file, only (I think)
impacts higher level functionality. But just reading the file isn't (I
claim) enough, we need the higher level functionality in order to really
"understand" the file contents.
My claim then is that being able to remove the file format support will
actually make the GDB slightly better (in some cases) as GDB will no
longer be able to read a file which it is then unable to correctly
process the contents of.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-04 14:26 ` Andrew Burgess
@ 2024-10-04 14:45 ` Eli Zaretskii
2024-10-07 18:30 ` Guinevere Larsen
0 siblings, 1 reply; 30+ messages in thread
From: Eli Zaretskii @ 2024-10-04 14:45 UTC (permalink / raw)
To: Andrew Burgess; +Cc: guinevere, gdb-patches
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: guinevere@redhat.com, gdb-patches@sourceware.org
> Date: Fri, 04 Oct 2024 15:26:25 +0100
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> > If I'm right, then there are two separate aspects to "target": on the
> > one side, the ability to start/stop executables, insert breakpoints
> > and watchpoints, etc., and OTOH the ability to read and process debug
> > info used by that target, implement frame unwinders, etc. Are we
> > currently conflating these into a single notion of "target", and if
> > so, will the proposed changes include or exclude both parts of each
> > "target"?
>
> I agree with you about "target" having two components, the lower level
> stuff, and the higher level stuff.
>
> File format support, being able to read from file, only (I think)
> impacts higher level functionality. But just reading the file isn't (I
> claim) enough, we need the higher level functionality in order to really
> "understand" the file contents.
>
> My claim then is that being able to remove the file format support will
> actually make the GDB slightly better (in some cases) as GDB will no
> longer be able to read a file which it is then unable to correctly
> process the contents of.
Thanks.
So given this situation, what exactly will removing, say, mipsread.o
give me? What will the GDB I build be unable to do that it was able
to do before?
And a more practical question: if I want to build GDB that will run on
GNU/Linux and should be able to debug Linux executables natively and
MS-Windows executables via gdbserver, which formats should I specify
with this new --enable-formats option?
See, I think these are the questions that the readers of the manual
will ask themselves, and we should have the answers there for them.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-04 14:45 ` Eli Zaretskii
@ 2024-10-07 18:30 ` Guinevere Larsen
2024-10-07 19:17 ` Eli Zaretskii
0 siblings, 1 reply; 30+ messages in thread
From: Guinevere Larsen @ 2024-10-07 18:30 UTC (permalink / raw)
To: Eli Zaretskii, Andrew Burgess; +Cc: gdb-patches
On 10/4/24 11:45 AM, Eli Zaretskii wrote:
>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: guinevere@redhat.com, gdb-patches@sourceware.org
>> Date: Fri, 04 Oct 2024 15:26:25 +0100
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>> If I'm right, then there are two separate aspects to "target": on the
>>> one side, the ability to start/stop executables, insert breakpoints
>>> and watchpoints, etc., and OTOH the ability to read and process debug
>>> info used by that target, implement frame unwinders, etc. Are we
>>> currently conflating these into a single notion of "target", and if
>>> so, will the proposed changes include or exclude both parts of each
>>> "target"?
>> I agree with you about "target" having two components, the lower level
>> stuff, and the higher level stuff.
>>
>> File format support, being able to read from file, only (I think)
>> impacts higher level functionality. But just reading the file isn't (I
>> claim) enough, we need the higher level functionality in order to really
>> "understand" the file contents.
>>
>> My claim then is that being able to remove the file format support will
>> actually make the GDB slightly better (in some cases) as GDB will no
>> longer be able to read a file which it is then unable to correctly
>> process the contents of.
> Thanks.
>
> So given this situation, what exactly will removing, say, mipsread.o
> give me? What will the GDB I build be unable to do that it was able
> to do before?
The main thing is security. If there was a security bug found in the
reader for mips files, and you're only interested in debugging x86_64
linux and windows. An attacker could maliciously craft a binary file
that appears to be with that file format, and convince a user to load
it, which could trigger the security bug. If you disable the mips
format, there is no physical way to reach the vulnerable code.
I expect this change to be most useful to software packagers who only
wish to support a few configurations, or very security conscious
end-users who compile their own software. This is because, even though
the project will aim to fix these as fast as possible, releases could
take a little longer, and so the users or packagers would need to
manually backport the fixes. Just removing the vulnerable file from
compilation altogether makes the backport unnecessary.
There are also small benefits in compilation time and final binary size,
but they are pretty minor.
>
> And a more practical question: if I want to build GDB that will run on
> GNU/Linux and should be able to debug Linux executables natively and
> MS-Windows executables via gdbserver, which formats should I specify
> with this new --enable-formats option?
Linux uses elf files, and windows uses PE-coff (compiled when coff is
requested), so you would need --enable-formats=elf,coff
However, with the patch as-is in the list, any value you put in there
(including --enable-formats=no, which would add nothing user selected)
would compile support for elf and coff anyway, so there is no need to worry.
Finally, if you aren't sure, the default behavior if the flag is to
compile all readers, keeping with the old behavior.
>
> See, I think these are the questions that the readers of the manual
> will ask themselves, and we should have the answers there for them.
>
That's a reasonable point. I don't think we should need to document all
targets, but I can see the value of documenting the main usage of the
file format when describing the options, as a guideline for users
running on mainstream platforms and not meant as an exhaustive list. I'm
thinking of adding the following to the readme:
`--enable-binary-file-format=FORMAT,FORMAT...'
`--enable-binary-file-format=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. The other possible options are:
* coff: Main format on windows systems. This is required to compile with
windows target support;
* 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;
* elf: Main format of linux systems. This is heavily recommended when
compiling with linux support;
* macho: Main format on MacOS systems, this is heavily recommended
when compiling for those targets;
* mips: (also known as ecoff), this is the main file format for targets
running on MIPS CPUs. It is heavily recommended when supporting those
targets.
* xcoff: Main format of AIX systems, this is required to compile for AIX
targets and rs6000 CPUs.
What do you think?
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-07 18:30 ` Guinevere Larsen
@ 2024-10-07 19:17 ` Eli Zaretskii
2024-10-07 19:58 ` Guinevere Larsen
0 siblings, 1 reply; 30+ messages in thread
From: Eli Zaretskii @ 2024-10-07 19:17 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: aburgess, gdb-patches
> Date: Mon, 7 Oct 2024 15:30:37 -0300
> Cc: gdb-patches@sourceware.org
> From: Guinevere Larsen <guinevere@redhat.com>
>
> > And a more practical question: if I want to build GDB that will run on
> > GNU/Linux and should be able to debug Linux executables natively and
> > MS-Windows executables via gdbserver, which formats should I specify
> > with this new --enable-formats option?
>
> Linux uses elf files, and windows uses PE-coff (compiled when coff is
> requested), so you would need --enable-formats=elf,coff
>
> However, with the patch as-is in the list, any value you put in there
> (including --enable-formats=no, which would add nothing user selected)
> would compile support for elf and coff anyway, so there is no need to worry.
>
> Finally, if you aren't sure, the default behavior if the flag is to
> compile all readers, keeping with the old behavior.
I presume users and packagers will want to use this feature to avoid
compiling in stuff they will never need. The problem, as I see it, is
that it is hard to decide what to specify, and your answer to my
Linux+Windows question illustrates this very well.
By contrast, the security benefits might not be interesting at least
to some.
> > See, I think these are the questions that the readers of the manual
> > will ask themselves, and we should have the answers there for them.
> >
> That's a reasonable point. I don't think we should need to document all
> targets, but I can see the value of documenting the main usage of the
> file format when describing the options, as a guideline for users
> running on mainstream platforms and not meant as an exhaustive list. I'm
> thinking of adding the following to the readme:
>
> `--enable-binary-file-format=FORMAT,FORMAT...'
> `--enable-binary-file-format=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. The other possible options are:
> * coff: Main format on windows systems. This is required to compile with
> windows target support;
> * 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;
> * elf: Main format of linux systems. This is heavily recommended when
> compiling with linux support;
> * macho: Main format on MacOS systems, this is heavily recommended
> when compiling for those targets;
> * mips: (also known as ecoff), this is the main file format for targets
> running on MIPS CPUs. It is heavily recommended when supporting those
> targets.
> * xcoff: Main format of AIX systems, this is required to compile for AIX
> targets and rs6000 CPUs.
>
>
> What do you think?
That's a good starting point, but IMO it stops short of answering the
practical questions people will ask themselves. For example, how do I
configure GDB to support a single target that is my native target and
OS? will specifying the single format from the above list do?
One thing I remember is that many targets produce core files in ELF
format. If that is true, then omitting ELF should be discouraged for
that reason. And maybe also other similar practical factoids.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-07 19:17 ` Eli Zaretskii
@ 2024-10-07 19:58 ` Guinevere Larsen
2024-10-08 11:44 ` Eli Zaretskii
0 siblings, 1 reply; 30+ messages in thread
From: Guinevere Larsen @ 2024-10-07 19:58 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: aburgess, gdb-patches
On 10/7/24 4:17 PM, Eli Zaretskii wrote:
>> Date: Mon, 7 Oct 2024 15:30:37 -0300
>> Cc: gdb-patches@sourceware.org
>> From: Guinevere Larsen <guinevere@redhat.com>
>>
>>> And a more practical question: if I want to build GDB that will run on
>>> GNU/Linux and should be able to debug Linux executables natively and
>>> MS-Windows executables via gdbserver, which formats should I specify
>>> with this new --enable-formats option?
>> Linux uses elf files, and windows uses PE-coff (compiled when coff is
>> requested), so you would need --enable-formats=elf,coff
>>
>> However, with the patch as-is in the list, any value you put in there
>> (including --enable-formats=no, which would add nothing user selected)
>> would compile support for elf and coff anyway, so there is no need to worry.
>>
>> Finally, if you aren't sure, the default behavior if the flag is to
>> compile all readers, keeping with the old behavior.
> I presume users and packagers will want to use this feature to avoid
> compiling in stuff they will never need. The problem, as I see it, is
> that it is hard to decide what to specify, and your answer to my
> Linux+Windows question illustrates this very well.
>
> By contrast, the security benefits might not be interesting at least
> to some.
That is fine. As the documentation below points out, the default
behavior is to add all readers just like GDB already does, so if a user
does not find this feature compelling, they are free to ignore that it
exists and continue using GDB as they have up until this point.
>
>>> See, I think these are the questions that the readers of the manual
>>> will ask themselves, and we should have the answers there for them.
>>>
>> That's a reasonable point. I don't think we should need to document all
>> targets, but I can see the value of documenting the main usage of the
>> file format when describing the options, as a guideline for users
>> running on mainstream platforms and not meant as an exhaustive list. I'm
>> thinking of adding the following to the readme:
>>
>> `--enable-binary-file-format=FORMAT,FORMAT...'
>> `--enable-binary-file-format=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. The other possible options are:
>> * coff: Main format on windows systems. This is required to compile with
>> windows target support;
>> * 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;
>> * elf: Main format of linux systems. This is heavily recommended when
>> compiling with linux support;
>> * macho: Main format on MacOS systems, this is heavily recommended
>> when compiling for those targets;
>> * mips: (also known as ecoff), this is the main file format for targets
>> running on MIPS CPUs. It is heavily recommended when supporting those
>> targets.
>> * xcoff: Main format of AIX systems, this is required to compile for AIX
>> targets and rs6000 CPUs.
>>
>>
>> What do you think?
> That's a good starting point, but IMO it stops short of answering the
> practical questions people will ask themselves. For example, how do I
> configure GDB to support a single target that is my native target and
> OS? will specifying the single format from the above list do?
If a user wants to configure their native target, that is related to the
--target and --enable-target options. Not the file formats. This change
is not related to how a user decides to support any given CPU or
operating system, but rather, which binary files they wish GDB to be
able to understand. If the user knows they want Windows and Linux, but
don't know how to specify that, they should look at documentation for
--target and --enable-targets, which is not touched by this patch or
related to this change whatsoever.
Once they know what target they are running, it will most likely be
Linux, Windows or MacOS, which are called out by name in this
description. If it is something different, the user is likely savvy
enough to look up online what their operating system supports, as there
are no users of these platforms who do so because it's the default in
the mainstream machine they bought.
>
> One thing I remember is that many targets produce core files in ELF
> format. If that is true, then omitting ELF should be discouraged for
> that reason. And maybe also other similar practical factoids.
>
I believe that is the case because there are many targets that use ELF
as the objfile format. My internet search couldn't find one very
reputable source, but it seems that MacOS uses Mach-O format, and
Windows uses PE/COFF format, the same as the default file format of the
target.
And once again, users don't need to interact with this feature. If it is
completely ignored, GDB will continue to work just as it did before this
patch went in.
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-07 19:58 ` Guinevere Larsen
@ 2024-10-08 11:44 ` Eli Zaretskii
2024-10-08 13:03 ` Guinevere Larsen
0 siblings, 1 reply; 30+ messages in thread
From: Eli Zaretskii @ 2024-10-08 11:44 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: aburgess, gdb-patches
> Date: Mon, 7 Oct 2024 16:58:16 -0300
> Cc: aburgess@redhat.com, gdb-patches@sourceware.org
> From: Guinevere Larsen <guinevere@redhat.com>
>
> On 10/7/24 4:17 PM, Eli Zaretskii wrote:
> > That's a good starting point, but IMO it stops short of answering the
> > practical questions people will ask themselves. For example, how do I
> > configure GDB to support a single target that is my native target and
> > OS? will specifying the single format from the above list do?
>
> If a user wants to configure their native target, that is related to the
> --target and --enable-target options. Not the file formats. This change
> is not related to how a user decides to support any given CPU or
> operating system, but rather, which binary files they wish GDB to be
> able to understand. If the user knows they want Windows and Linux, but
> don't know how to specify that, they should look at documentation for
> --target and --enable-targets, which is not touched by this patch or
> related to this change whatsoever.
I think there's a misunderstanding. I said "to support a single
target", meaning _only_ that single target.
You seem to be saying that --target is what I want, but that's not
what I knew. Right now, when I compile GDB for native Windows
debugging, I get all the *read.c files compiled and linked into GDB.
I don't specify --target when I build GDB, but I do specify --build,
and I thought that --build=FOO is a short for --host=FOO --target=FOO.
So I thought that I am already specifying --target, and yet I get all
the binary-format readers compiled and linked in. So I was asking
what should I do at configure time to have only the *read.c files I
need for that single target and only for native debugging.
If you are saying that the --enable-binary-format is unrelated to my
questions, then I guess I'm even more confused than I thought I was.
> > One thing I remember is that many targets produce core files in ELF
> > format. If that is true, then omitting ELF should be discouraged for
> > that reason. And maybe also other similar practical factoids.
> >
> I believe that is the case because there are many targets that use ELF
> as the objfile format.
True, but not what I was saying: I was specifically alluding to
targets whose objfile format is _not_ ELF, and yet they produce core
files in ELF format. ISTR that Cygwin is one such target? I'm sure
others here will be able to find more examples.
> And once again, users don't need to interact with this feature. If it is
> completely ignored, GDB will continue to work just as it did before this
> patch went in.
I understand, and that is good, but I'm talking from the POV of
someone who _wants_ to use this new feature, in order to make GDB
smaller.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-08 11:44 ` Eli Zaretskii
@ 2024-10-08 13:03 ` Guinevere Larsen
2024-10-08 13:21 ` Eli Zaretskii
0 siblings, 1 reply; 30+ messages in thread
From: Guinevere Larsen @ 2024-10-08 13:03 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: aburgess, gdb-patches
On 10/8/24 8:44 AM, Eli Zaretskii wrote:
>> Date: Mon, 7 Oct 2024 16:58:16 -0300
>> Cc: aburgess@redhat.com, gdb-patches@sourceware.org
>> From: Guinevere Larsen <guinevere@redhat.com>
>>
>> On 10/7/24 4:17 PM, Eli Zaretskii wrote:
>>> That's a good starting point, but IMO it stops short of answering the
>>> practical questions people will ask themselves. For example, how do I
>>> configure GDB to support a single target that is my native target and
>>> OS? will specifying the single format from the above list do?
>> If a user wants to configure their native target, that is related to the
>> --target and --enable-target options. Not the file formats. This change
>> is not related to how a user decides to support any given CPU or
>> operating system, but rather, which binary files they wish GDB to be
>> able to understand. If the user knows they want Windows and Linux, but
>> don't know how to specify that, they should look at documentation for
>> --target and --enable-targets, which is not touched by this patch or
>> related to this change whatsoever.
> I think there's a misunderstanding. I said "to support a single
> target", meaning _only_ that single target.
>
> You seem to be saying that --target is what I want, but that's not
> what I knew. Right now, when I compile GDB for native Windows
> debugging, I get all the *read.c files compiled and linked into GDB.
> I don't specify --target when I build GDB, but I do specify --build,
> and I thought that --build=FOO is a short for --host=FOO --target=FOO.
> So I thought that I am already specifying --target, and yet I get all
> the binary-format readers compiled and linked in. So I was asking
> what should I do at configure time to have only the *read.c files I
> need for that single target and only for native debugging.
Then I guess the response you wanted is the paragraph you omitted in
this response:
Once they know what target they are running, it will most likely be
Linux, Windows or MacOS, which are called out by name in this
description. If it is something different, the user is likely savvy
enough to look up online what their operating system supports, as there
are no users of these platforms who do so because it's the default in
the mainstream machine they bought.
If you know that your platform is FOO, you can either easily find it in
the list, or it is a specialized system that you chose to use, and are
likely to know better than me where to find that information.
>
> If you are saying that the --enable-binary-format is unrelated to my
> questions, then I guess I'm even more confused than I thought I was.
>
>>> One thing I remember is that many targets produce core files in ELF
>>> format. If that is true, then omitting ELF should be discouraged for
>>> that reason. And maybe also other similar practical factoids.
>>>
>> I believe that is the case because there are many targets that use ELF
>> as the objfile format.
> True, but not what I was saying: I was specifically alluding to
> targets whose objfile format is _not_ ELF, and yet they produce core
> files in ELF format. ISTR that Cygwin is one such target? I'm sure
> others here will be able to find more examples.
Which is all the more reason for GDB to not be the owner of this type of
documentation.
I couldn't find any information about how core files were handled in
Windows, but as soon as you - someone who clearly knows the system -
brought up that maybe elf should be used with cygwin, a quick search for
the specification found a footnote confirming this.
We should not be expected to keep on top of all the specifications of
all targets we own so we can suggest to possible users that maybe they
want this if they use some specific configuration.
Ultimately, this is a feature for advanced users who have deep domain
knowledge on the systems they expect to handle, and similar to how, even
with the update on the --target and --enable-targets documentation, I
wouldn't expect every single target triplet supported by GDB to be
listed in the documentation, I don't think we should list what formats
are recommended for each target.
I can change the first paragraph of the description to read as follows:
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. If you are unsure of which options
you will need for your debugging sessions, we recommend that you
not make use of this function. The possible options are:
<list of options as sent in previous email>
>
>> And once again, users don't need to interact with this feature. If it is
>> completely ignored, GDB will continue to work just as it did before this
>> patch went in.
> I understand, and that is good, but I'm talking from the POV of
> someone who _wants_ to use this new feature, in order to make GDB
> smaller.
>
> Thanks.
>
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-08 13:03 ` Guinevere Larsen
@ 2024-10-08 13:21 ` Eli Zaretskii
2024-10-10 14:45 ` Guinevere Larsen
2024-10-10 16:10 ` Andrew Burgess
0 siblings, 2 replies; 30+ messages in thread
From: Eli Zaretskii @ 2024-10-08 13:21 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: aburgess, gdb-patches
> Date: Tue, 8 Oct 2024 10:03:58 -0300
> Cc: aburgess@redhat.com, gdb-patches@sourceware.org
> From: Guinevere Larsen <guinevere@redhat.com>
>
> Ultimately, this is a feature for advanced users who have deep domain
> knowledge on the systems they expect to handle, and similar to how, even
> with the update on the --target and --enable-targets documentation, I
> wouldn't expect every single target triplet supported by GDB to be
> listed in the documentation, I don't think we should list what formats
> are recommended for each target.
>
> I can change the first paragraph of the description to read as follows:
> 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. If you are unsure of which options
> you will need for your debugging sessions, we recommend that you
> not make use of this function. The possible options are:
>
> <list of options as sent in previous email>
Maybe this is better.
I'm worried by the fact that I'm the only one talking to you about
this: maybe there's no problem at all, and I'm the only one confused.
In that case, apologies for wasting your time.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-08 13:21 ` Eli Zaretskii
@ 2024-10-10 14:45 ` Guinevere Larsen
2024-10-10 16:10 ` Andrew Burgess
1 sibling, 0 replies; 30+ messages in thread
From: Guinevere Larsen @ 2024-10-10 14:45 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: aburgess, gdb-patches
On 10/8/24 10:21 AM, Eli Zaretskii wrote:
>> Date: Tue, 8 Oct 2024 10:03:58 -0300
>> Cc: aburgess@redhat.com, gdb-patches@sourceware.org
>> From: Guinevere Larsen <guinevere@redhat.com>
>>
>> Ultimately, this is a feature for advanced users who have deep domain
>> knowledge on the systems they expect to handle, and similar to how, even
>> with the update on the --target and --enable-targets documentation, I
>> wouldn't expect every single target triplet supported by GDB to be
>> listed in the documentation, I don't think we should list what formats
>> are recommended for each target.
>>
>> I can change the first paragraph of the description to read as follows:
>> 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. If you are unsure of which options
>> you will need for your debugging sessions, we recommend that you
>> not make use of this function. The possible options are:
>>
>> <list of options as sent in previous email>
> Maybe this is better.
>
> I'm worried by the fact that I'm the only one talking to you about
> this: maybe there's no problem at all, and I'm the only one confused.
> In that case, apologies for wasting your time.
>
If nothing else, at least the penny finally dropped that it isn't
obvious this is a feature for advanced users. I'll post an updated
version as soon as I can fix the crash that andrew reported
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-08 13:21 ` Eli Zaretskii
2024-10-10 14:45 ` Guinevere Larsen
@ 2024-10-10 16:10 ` Andrew Burgess
1 sibling, 0 replies; 30+ messages in thread
From: Andrew Burgess @ 2024-10-10 16:10 UTC (permalink / raw)
To: Eli Zaretskii, Guinevere Larsen; +Cc: gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Tue, 8 Oct 2024 10:03:58 -0300
>> Cc: aburgess@redhat.com, gdb-patches@sourceware.org
>> From: Guinevere Larsen <guinevere@redhat.com>
>>
>> Ultimately, this is a feature for advanced users who have deep domain
>> knowledge on the systems they expect to handle, and similar to how, even
>> with the update on the --target and --enable-targets documentation, I
>> wouldn't expect every single target triplet supported by GDB to be
>> listed in the documentation, I don't think we should list what formats
>> are recommended for each target.
>>
>> I can change the first paragraph of the description to read as follows:
>> 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. If you are unsure of which options
>> you will need for your debugging sessions, we recommend that you
>> not make use of this function. The possible options are:
>>
>> <list of options as sent in previous email>
>
> Maybe this is better.
>
> I'm worried by the fact that I'm the only one talking to you about
> this: maybe there's no problem at all, and I'm the only one confused.
> In that case, apologies for wasting your time.
I think it's OK to be unsure about this. You asked a couple of times,
what values would I need to pass to support all the required formats on
target X? And I don't think you really got a clear answer.
I'll take a stab at answering it: we don't know, and that's OK.
As Gwen said, this option is intended for power users who have a clear
idea about their targets, what file formats are required, and which
formats they want to support.
If a user does NOT have that knowledge then they shouldn't touch this
setting.
The question then becomes, whose job is it to teach the user about their
target, and which file formats it uses. I agree with Gwen here, that's
not our job.
We should carefully document the possible values that can be passed to
this configure flag. We should also make it clear what the consequences
of misusing this flag are (i.e. you'll end up with a GDB that can't read
some file formats), and, I think, we should suggest that if a user is in
any doubt then they should leave this flag as its default. My hope is
that by strengthening the documentation to make this final concrete
recommendation then we hopefully address your concern about a user who
is reading the docs trying to figure out how to build GDB.
On a different note: I haven't forgotten my offer of updating the
--target and --enable-targets docs, it'll probably be next week before I
can work on this though.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-25 17:53 [PATCH] gdb, configure: Add disable-formats option for configure Guinevere Larsen
2024-09-26 5:49 ` Eli Zaretskii
@ 2024-09-26 19:18 ` Tom Tromey
2024-09-26 19:49 ` Guinevere Larsen
2024-10-02 13:56 ` Andrew Burgess
2024-10-04 14:49 ` Andrew Burgess
3 siblings, 1 reply; 30+ messages in thread
From: Tom Tromey @ 2024-09-26 19:18 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: gdb-patches
>>>>> Guinevere Larsen <guinevere@redhat.com> writes:
> 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.
The goal seems totally fine to me.
> +enable-formats=[FORMAT,]...
I think this option name is too generic.
> +# If all targets were requested, this is all formats that should accompany
> +# them.
> +all_target_formats="elf xcoff mips coff"
I think it would be nicer if the full list only had to be written in one
spot. Right now it seems like it's both here and in the makefile? viz:
+# All files that relate to GDB's ability to read debug information.
+# Used with --enable-formats=all.
+ALL_FORMAT_OBS = \
I'd written some more stuff here about maybe just following BFD's lead.
But looking at my "minimal" build tree, I see a native Linux gdb seems
to have COFF and other junk in BFD, so following BFD doesn't seem really
possible.
Tom
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-26 19:18 ` Tom Tromey
@ 2024-09-26 19:49 ` Guinevere Larsen
2024-09-27 18:01 ` Tom Tromey
0 siblings, 1 reply; 30+ messages in thread
From: Guinevere Larsen @ 2024-09-26 19:49 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 9/26/24 4:18 PM, Tom Tromey wrote:
>>>>>> Guinevere Larsen <guinevere@redhat.com> writes:
>> 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.
> The goal seems totally fine to me.
>
>> +enable-formats=[FORMAT,]...
> I think this option name is too generic.
Eli suggested objfile-format and binary-file-format. I was planning on
using the former, unless you still think is too generic, or have a
better suggestion :)
>
>> +# If all targets were requested, this is all formats that should accompany
>> +# them.
>> +all_target_formats="elf xcoff mips coff"
> I think it would be nicer if the full list only had to be written in one
> spot. Right now it seems like it's both here and in the makefile? viz:
>
> +# All files that relate to GDB's ability to read debug information.
> +# Used with --enable-formats=all.
> +ALL_FORMAT_OBS = \
I guess the real problem is a confusing name (and overlap).
All_target_formats is all the formats that have to be compiled in if the
user requested --enable-targets=all. This list doesn't have Mach-O and dbx.
I'll try to think of a better naming scheme so that this doesn't feel
like duplicated information.
>
> I'd written some more stuff here about maybe just following BFD's lead.
> But looking at my "minimal" build tree, I see a native Linux gdb seems
> to have COFF and other junk in BFD, so following BFD doesn't seem really
> possible.
>
> Tom
>
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-26 19:49 ` Guinevere Larsen
@ 2024-09-27 18:01 ` Tom Tromey
0 siblings, 0 replies; 30+ messages in thread
From: Tom Tromey @ 2024-09-27 18:01 UTC (permalink / raw)
To: Guinevere Larsen; +Cc: Tom Tromey, gdb-patches
>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:
>>> +enable-formats=[FORMAT,]...
>> I think this option name is too generic.
Guinevere> Eli suggested objfile-format and binary-file-format. I was planning on
Guinevere> using the former, unless you still think is too generic, or have a
Guinevere> better suggestion :)
I don't believe naming things is really my strong suit.
I tend to rush through it.
"objfile" though seems like gdb internals terminology.
Maybe something like "--enable-gdb-file-formats"?
Having "gdb" in there explicitly may make it more clear that this isn't
affecting BFD.
Guinevere> I guess the real problem is a confusing name (and
Guinevere> overlap). All_target_formats is all the formats that have to be
Guinevere> compiled in if the user requested --enable-targets=all. This list
Guinevere> doesn't have Mach-O and dbx.
dbx I understand, since a.out is largely dead, but surely Mach-O is
needed by --enable-targets=all in order to target macOS?
Tom
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-25 17:53 [PATCH] gdb, configure: Add disable-formats option for configure Guinevere Larsen
2024-09-26 5:49 ` Eli Zaretskii
2024-09-26 19:18 ` Tom Tromey
@ 2024-10-02 13:56 ` Andrew Burgess
2024-10-02 20:37 ` Guinevere Larsen
2024-10-04 14:49 ` Andrew Burgess
3 siblings, 1 reply; 30+ messages in thread
From: Andrew Burgess @ 2024-10-02 13:56 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches; +Cc: Guinevere Larsen
Guinevere Larsen <guinevere@redhat.com> writes:
> 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.
>
> 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.
>
> 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.
> ---
> gdb/Makefile.in | 22 +++++++-----
> gdb/NEWS | 11 ++++++
> gdb/README | 5 +++
> gdb/configure | 82 +++++++++++++++++++++++++++++++++++++++++---
> gdb/configure.ac | 68 ++++++++++++++++++++++++++++++++++--
> gdb/configure.format | 41 ++++++++++++++++++++++
> gdb/configure.tgt | 6 ++--
> gdb/doc/gdb.texinfo | 7 ++++
> 8 files changed, 225 insertions(+), 17 deletions(-)
> create mode 100644 gdb/configure.format
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index bcf1ee45a70..009d68d6de2 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -901,13 +901,24 @@ ALL_TARGET_OBS = \
> vax-tdep.o \
> windows-tdep.o \
> x86-tdep.o \
> - xcoffread.o \
> xstormy16-tdep.o \
> xtensa-config.o \
> xtensa-linux-tdep.o \
> xtensa-tdep.o \
> z80-tdep.o
>
> +# Object files for reading specific types of debug information.
> +FORMAT_OBS = @FORMAT_OBS@
> +
> +# 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
> +
> # The following native-target dependent variables are defined on
> # configure.nat.
> NAT_FILE = @NAT_FILE@
> @@ -1064,8 +1075,6 @@ COMMON_SFILES = \
> c-varobj.c \
> charset.c \
> cli-out.c \
> - coff-pe-read.c \
> - coffread.c \
> complaints.c \
> completer.c \
> copying.c \
> @@ -1079,7 +1088,6 @@ COMMON_SFILES = \
> d-lang.c \
> d-namespace.c \
> d-valprint.c \
> - dbxread.c \
> dcache.c \
> debug.c \
> debuginfod-support.c \
> @@ -1171,7 +1179,6 @@ COMMON_SFILES = \
> memtag.c \
> minidebug.c \
> minsyms.c \
> - mipsread.c \
> namespace.c \
> objc-lang.c \
> objfiles.c \
> @@ -1264,7 +1271,6 @@ SFILES = \
> d-exp.y \
> dtrace-probe.c \
> elf-none-tdep.c \
> - elfread.c \
> f-exp.y \
> gcore-elf.c \
> gdb.c \
> @@ -1886,7 +1892,6 @@ ALLDEPFILES = \
> windows-tdep.c \
> x86-nat.c \
> x86-tdep.c \
> - xcoffread.c \
> xstormy16-tdep.c \
> xtensa-config.c \
> xtensa-linux-nat.c \
> @@ -1908,7 +1913,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
> $(SUBDIR_CLI_OBS) \
> $(SUBDIR_MI_OBS) \
> $(SUBDIR_TARGET_OBS) \
> - $(SUBDIR_GCC_COMPILE_OBS)
> + $(SUBDIR_GCC_COMPILE_OBS) \
> + $(FORMAT_OBS)
>
> SUBDIRS = doc @subdirs@ data-directory
> CLEANDIRS = $(SUBDIRS)
> 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.
> + 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.
> +
> *** Changes in GDB 15
>
> * The MPX commands "show/set mpx bound" have been deprecated, as Intel
> diff --git a/gdb/README b/gdb/README
> index d85c37d5d17..342b2d07eb7 100644
> --- 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`
> + Configure GDB to be unable to read some binary file formats, such as
> + coff, dbx or elf.
> +
> `--with-gdb-datadir=PATH'
> Set the GDB-specific data directory. GDB will look here for
> certain supporting files or scripts. This defaults to the `gdb'
> diff --git a/gdb/configure b/gdb/configure
> index 53eaad4f0e2..792e5cefefe 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -706,6 +706,7 @@ LIBGUI
> LTLIBLZMA
> LIBLZMA
> HAVE_LIBLZMA
> +FORMAT_OBS
> SER_HARDWIRE
> WERROR_CFLAGS
> WARN_CFLAGS
> @@ -933,6 +934,7 @@ with_relocated_sources
> with_auto_load_dir
> with_auto_load_safe_path
> enable_targets
> +enable_formats
> enable_64_bit_bfd
> with_amd_dbgapi
> enable_tui
> @@ -1644,6 +1646,9 @@ Optional Features:
> --disable-nls do not use Native Language Support
> --enable-targets=TARGETS
> alternative target configurations
> + --enable-formats=FILE_FORMATS
> + enable support for selected file formats(default
> + 'all')
> --enable-64-bit-bfd 64-bit support (on hosts with narrower word sizes)
> --enable-tui enable full-screen terminal user interface (TUI)
> --enable-gdbtk enable gdbtk graphical user interface (GUI)
> @@ -11499,7 +11504,7 @@ else
> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
> lt_status=$lt_dlunknown
> cat > conftest.$ac_ext <<_LT_EOF
> -#line 11502 "configure"
> +#line 11507 "configure"
> #include "confdefs.h"
>
> #if HAVE_DLFCN_H
> @@ -11605,7 +11610,7 @@ else
> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
> lt_status=$lt_dlunknown
> cat > conftest.$ac_ext <<_LT_EOF
> -#line 11608 "configure"
> +#line 11613 "configure"
> #include "confdefs.h"
>
> #if HAVE_DLFCN_H
> @@ -24833,6 +24838,20 @@ esac
> fi
>
>
> +all_formats=
> +# Check whether --enable-formats was given.
> +if test "${enable_formats+set}" = set; then :
> + enableval=$enable_formats; case "${enableval}" in
> + yes | "") as_fn_error $? "enable-formats option must specify file formats or 'all'" "$LINENO" 5
> + ;;
> + no) enable_formats= ;;
> + *) enable_formats=$enableval ;;
> +esac
> +else
> + all_formats=true
> +fi
I think it would be simpler to drop `all_formats` here, and make the
default case just set `enable_formats=all`. Then later on, when you
process `enable_formats` (where you have to check for 'all' anyway),
you'll spot the 'all' case there and do whatever needs doing. I have
another note about that logic below.
> +
> +
> # Check whether --enable-64-bit-bfd was given.
> if test "${enable_64_bit_bfd+set}" = set; then :
> enableval=$enable_64_bit_bfd; case $enableval in #(
> @@ -24915,11 +24934,20 @@ fi
> TARGET_OBS=
> all_targets=
> HAVE_NATIVE_GCORE_TARGET=
> +# File formats that will ne enabled based on the selected
type: ne ?
> +# target(s). These are chosen because most, if not all, executables for
> +# the target will follow this file format so it makes no sense to support
> +# the target but not the debug information.
> +target_formats=
> +# If all targets were requested, this is all formats that should accompany
> +# them.
> +all_target_formats="elf xcoff mips coff"
>
> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
> do
> if test "$targ_alias" = "all"; then
> all_targets=true
> + target_formats=$all_target_formats
> else
> # Canonicalize the secondary target names.
> result=`$ac_config_sub $targ_alias 2>/dev/null`
> @@ -24941,6 +24969,19 @@ fi
> *" ${i} "*) ;;
> *)
> TARGET_OBS="$TARGET_OBS ${i}"
> + # 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
Is this required? Or is this left over debug output?
> + 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
I think this logic should be moved into configure.tgt. We already have
a block at the end of that file which makes some choices based on which
files are being added to the object list, which is exactly what you're
doing here. This change would make the formats required by a target be
an output from configure.tgt, which makes sense to me.
> ;;
> esac
> done
> @@ -24964,6 +25005,7 @@ if test x${all_targets} = xtrue; then
> else
> TARGET_OBS='$(ALL_TARGET_OBS)'
> fi
> + target_readers=$all_target_readers
> fi
>
> # AMD debugger API support.
> @@ -31462,6 +31504,7 @@ fi
> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>
> +support_elf=no
> # Add ELF support to GDB, but only if BFD includes ELF support.
>
> OLD_CFLAGS=$CFLAGS
> @@ -31514,7 +31557,7 @@ $as_echo "$gdb_cv_var_elf" >&6; }
> LDFLAGS=$OLD_LDFLAGS
> LIBS=$OLD_LIBS
> 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"
>
> $as_echo "#define HAVE_ELF 1" >>confdefs.h
> @@ -31578,9 +31621,11 @@ if test "$ac_res" != no; then :
> fi
>
> fi
> + support_elf=yes
> fi
>
> # Add macho support to GDB, but only if BFD includes it.
> +support_macho=no
>
> OLD_CFLAGS=$CFLAGS
> OLD_LDFLAGS=$LDFLAGS
> @@ -31632,9 +31677,38 @@ $as_echo "$gdb_cv_var_macho" >&6; }
> LDFLAGS=$OLD_LDFLAGS
> LIBS=$OLD_LIBS
> if test "$gdb_cv_var_macho" = yes; then
> - CONFIG_OBS="$CONFIG_OBS machoread.o"
> + support_macho=yes
> fi
>
> +FORMAT_OBS=
> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
> +
> +if test "$all_formats" = "true"; then
> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
As Tom pointed out, right now we effectively have the list of format
reading .c files twice.
If instead of using ALL_FORMAT_OBS here we instead expanded ' all '
inside enable_formats into the list of all known formats
(e.g. $all_target_formats), then, when we run through the loop below,
FORMAT_OBS would always be set to the complete list, right? Then we'd
not need the file list in the Makefile at all, instead we just need
all_target_formats defined in configure.ac and then handling for each
value in configure.format.
> +else
> + # 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"
Instead of 'echo', maybe try AC_MSG_WARN maybe? Or AC_MSG_NOTICE?
Check out:
https://www.gnu.org/software/autoconf/manual/autoconf-2.60/html_node/Printing-Messages.html
for options.
> + enable_formats="${enable_formats} $req"
> + fi
> + done
> +
> + for format in $enable_formats
> + do
> + if test "$format" = "all"; then
> + all_formats=true
> + fi
> +
> + . ${srcdir}/configure.format
> + done
> +fi
> +
> +echo $FORMAT_OBS
Is this left over debug?
> +
> +
> +
> # Add any host-specific objects to GDB.
> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 8368fea0423..5f5187ecd0f 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -187,6 +187,16 @@ AS_HELP_STRING([--enable-targets=TARGETS], [alternative target configurations]),
> *) enable_targets=$enableval ;;
> esac])
>
> +all_formats=
> +AC_ARG_ENABLE(formats,
> + AS_HELP_STRING([--enable-formats=FILE_FORMATS], [enable support for selected file formats(default 'all')]),
> +[case "${enableval}" in
> + yes | "") AC_MSG_ERROR(enable-formats option must specify file formats or 'all')
> + ;;
> + no) enable_formats= ;;
> + *) enable_formats=$enableval ;;
> +esac], [all_formats=true])
> +
> BFD_64_BIT
>
> # Provide defaults for some variables set by the per-host and per-target
> @@ -206,11 +216,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 most, if not all, executables for
> +# the target will follow this file format so it makes no sense to support
> +# the target but not the debug information.
> +target_formats=
> +# If all targets were requested, this is all formats that should accompany
> +# them.
> +all_target_formats="elf xcoff mips coff"
>
> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
> do
> if test "$targ_alias" = "all"; then
> all_targets=true
> + target_formats=$all_target_formats
> else
> # Canonicalize the secondary target names.
> result=`$ac_config_sub $targ_alias 2>/dev/null`
> @@ -231,6 +250,19 @@ do
> *" ${i} "*) ;;
> *)
> TARGET_OBS="$TARGET_OBS ${i}"
> + # 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
> ;;
> esac
> done
> @@ -1850,11 +1882,12 @@ fi
> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>
> +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"
Maybe this is something we can unpick later, but if we only add all
these .o files if we have ELF support. And if we chose to configure GDB
without ELF support .... should we actually be dropping all these .o
files?
> AC_DEFINE(HAVE_ELF, 1,
> [Define if ELF support should be included.])
> @@ -1862,15 +1895,46 @@ if test "$gdb_cv_var_elf" = yes; then
> if test "$plugins" = "yes"; then
> AC_SEARCH_LIBS(dlopen, dl)
> fi
> + support_elf=yes
> fi
>
> # Add macho support to GDB, but only if BFD includes it.
> +support_macho=no
> 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
> fi
>
> +FORMAT_OBS=
> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
> +
> +if test "$all_formats" = "true"; then
> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
> +else
> + # 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"
> + enable_formats="${enable_formats} $req"
> + fi
> + done
> +
> + for format in $enable_formats
> + do
> + if test "$format" = "all"; then
> + all_formats=true
> + fi
Maybe I'm missing something, but isn't setting 'all_formats' here too
late? That is, we are already inside the 'else' block which checked the
'all_formats' variable, and after this point it's not (as far as I can
tell) checked again.
> +
> + . ${srcdir}/configure.format
> + done
> +fi
> +
> +echo $FORMAT_OBS
> +
> +AC_SUBST(FORMAT_OBS)
> +
> # Add any host-specific objects to GDB.
> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>
> diff --git a/gdb/configure.format b/gdb/configure.format
> new file mode 100644
> index 00000000000..12dd2d25717
> --- /dev/null
> +++ b/gdb/configure.format
> @@ -0,0 +1,41 @@
> +# Copyright (C) 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 <http://www.gnu.org/licenses/>.
> +
> +# This file is used to decide which files need to be compiled to support
> +# the requested file formats
> +
> +case $format in
> + xcoff) FORMAT_OBS="$FORMAT_OBS xcoffread.o" ;;
In all the other configure.* files the format used is like this:
xcoff)
FORMAT_OBS="$FORMAT_OBS xcoffread.o"
;;
I think we should adopt this for consistency (I also prefer it, but lets
go with the consistency argument).
Thanks,
Andrew
> +
> + # 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" ;;
> +
> + dbx) FORMAT_OBS="$FORMAT_OBS dbxread.o" ;;
> +
> + elf) if "$support_elf"="yes"; then
> + FORMAT_OBS="$FORMAT_OBS elfread.o"
> + fi
> + ;;
> +
> + macho) if "$support_macho"="yes"; then
> + FORMAT_OBS="$FORMAT_OBS machoread.o"
> + fi
> + ;;
> +
> + mips) FORMAT_OBS="$FORMAT_OBS mipsread.o" ;;
> +esac
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index 8d85a597ec8..793793601c1 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -507,7 +507,7 @@ powerpc-*-openbsd*)
> ;;
> powerpc-*-aix* | rs6000-*-* | powerpc64-*-aix*)
> # Target: PowerPC running AIX
> - gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o xcoffread.o \
> + gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o \
> ppc-sysv-tdep.o solib-aix.o \
> ravenscar-thread.o ppc-ravenscar-thread.o"
> ;;
> @@ -523,8 +523,8 @@ powerpc*-*-linux*)
> powerpc-*-lynx*178)
> # Target: PowerPC running Lynx178.
> gdb_target_obs="rs6000-tdep.o rs6000-lynx178-tdep.o \
> - xcoffread.o ppc-sysv-tdep.o \
> - ravenscar-thread.o ppc-ravenscar-thread.o"
> + ppc-sysv-tdep.o ravenscar-thread.o \
> + ppc-ravenscar-thread.o"
> ;;
> powerpc*-*-*)
> # Target: PowerPC running eabi
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 77a4021b36a..c569e68060e 100644
> --- 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
> +Configure @value{GDBN} to support certain binary file formats. If a
> +format is the main (or only) file format for a given target, the
> +configure script may add support to it anyway, and warn the user.
> +If not given, all file formats that @value{GDBN} supports are compiled.
> +
> @item --with-gdb-datadir=@var{path}
> Set the @value{GDBN}-specific data directory. @value{GDBN} will look
> here for certain supporting files or scripts. This defaults to the
> --
> 2.46.1
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-02 13:56 ` Andrew Burgess
@ 2024-10-02 20:37 ` Guinevere Larsen
2024-10-03 10:15 ` Andrew Burgess
0 siblings, 1 reply; 30+ messages in thread
From: Guinevere Larsen @ 2024-10-02 20:37 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 10/2/24 10:56 AM, Andrew Burgess wrote:
> Guinevere Larsen <guinevere@redhat.com> writes:
>
>> 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.
>>
>> 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.
>>
>> 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.
>> ---
>> gdb/Makefile.in | 22 +++++++-----
>> gdb/NEWS | 11 ++++++
>> gdb/README | 5 +++
>> gdb/configure | 82 +++++++++++++++++++++++++++++++++++++++++---
>> gdb/configure.ac | 68 ++++++++++++++++++++++++++++++++++--
>> gdb/configure.format | 41 ++++++++++++++++++++++
>> gdb/configure.tgt | 6 ++--
>> gdb/doc/gdb.texinfo | 7 ++++
>> 8 files changed, 225 insertions(+), 17 deletions(-)
>> create mode 100644 gdb/configure.format
>>
>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>> index bcf1ee45a70..009d68d6de2 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -901,13 +901,24 @@ ALL_TARGET_OBS = \
>> vax-tdep.o \
>> windows-tdep.o \
>> x86-tdep.o \
>> - xcoffread.o \
>> xstormy16-tdep.o \
>> xtensa-config.o \
>> xtensa-linux-tdep.o \
>> xtensa-tdep.o \
>> z80-tdep.o
>>
>> +# Object files for reading specific types of debug information.
>> +FORMAT_OBS = @FORMAT_OBS@
>> +
>> +# 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
>> +
>> # The following native-target dependent variables are defined on
>> # configure.nat.
>> NAT_FILE = @NAT_FILE@
>> @@ -1064,8 +1075,6 @@ COMMON_SFILES = \
>> c-varobj.c \
>> charset.c \
>> cli-out.c \
>> - coff-pe-read.c \
>> - coffread.c \
>> complaints.c \
>> completer.c \
>> copying.c \
>> @@ -1079,7 +1088,6 @@ COMMON_SFILES = \
>> d-lang.c \
>> d-namespace.c \
>> d-valprint.c \
>> - dbxread.c \
>> dcache.c \
>> debug.c \
>> debuginfod-support.c \
>> @@ -1171,7 +1179,6 @@ COMMON_SFILES = \
>> memtag.c \
>> minidebug.c \
>> minsyms.c \
>> - mipsread.c \
>> namespace.c \
>> objc-lang.c \
>> objfiles.c \
>> @@ -1264,7 +1271,6 @@ SFILES = \
>> d-exp.y \
>> dtrace-probe.c \
>> elf-none-tdep.c \
>> - elfread.c \
>> f-exp.y \
>> gcore-elf.c \
>> gdb.c \
>> @@ -1886,7 +1892,6 @@ ALLDEPFILES = \
>> windows-tdep.c \
>> x86-nat.c \
>> x86-tdep.c \
>> - xcoffread.c \
>> xstormy16-tdep.c \
>> xtensa-config.c \
>> xtensa-linux-nat.c \
>> @@ -1908,7 +1913,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>> $(SUBDIR_CLI_OBS) \
>> $(SUBDIR_MI_OBS) \
>> $(SUBDIR_TARGET_OBS) \
>> - $(SUBDIR_GCC_COMPILE_OBS)
>> + $(SUBDIR_GCC_COMPILE_OBS) \
>> + $(FORMAT_OBS)
>>
>> SUBDIRS = doc @subdirs@ data-directory
>> CLEANDIRS = $(SUBDIRS)
>> 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.
>> + 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.
>> +
>> *** Changes in GDB 15
>>
>> * The MPX commands "show/set mpx bound" have been deprecated, as Intel
>> diff --git a/gdb/README b/gdb/README
>> index d85c37d5d17..342b2d07eb7 100644
>> --- 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`
>> + Configure GDB to be unable to read some binary file formats, such as
>> + coff, dbx or elf.
>> +
>> `--with-gdb-datadir=PATH'
>> Set the GDB-specific data directory. GDB will look here for
>> certain supporting files or scripts. This defaults to the `gdb'
>> diff --git a/gdb/configure b/gdb/configure
>> index 53eaad4f0e2..792e5cefefe 100755
>> --- a/gdb/configure
>> +++ b/gdb/configure
>> @@ -706,6 +706,7 @@ LIBGUI
>> LTLIBLZMA
>> LIBLZMA
>> HAVE_LIBLZMA
>> +FORMAT_OBS
>> SER_HARDWIRE
>> WERROR_CFLAGS
>> WARN_CFLAGS
>> @@ -933,6 +934,7 @@ with_relocated_sources
>> with_auto_load_dir
>> with_auto_load_safe_path
>> enable_targets
>> +enable_formats
>> enable_64_bit_bfd
>> with_amd_dbgapi
>> enable_tui
>> @@ -1644,6 +1646,9 @@ Optional Features:
>> --disable-nls do not use Native Language Support
>> --enable-targets=TARGETS
>> alternative target configurations
>> + --enable-formats=FILE_FORMATS
>> + enable support for selected file formats(default
>> + 'all')
>> --enable-64-bit-bfd 64-bit support (on hosts with narrower word sizes)
>> --enable-tui enable full-screen terminal user interface (TUI)
>> --enable-gdbtk enable gdbtk graphical user interface (GUI)
>> @@ -11499,7 +11504,7 @@ else
>> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>> lt_status=$lt_dlunknown
>> cat > conftest.$ac_ext <<_LT_EOF
>> -#line 11502 "configure"
>> +#line 11507 "configure"
>> #include "confdefs.h"
>>
>> #if HAVE_DLFCN_H
>> @@ -11605,7 +11610,7 @@ else
>> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>> lt_status=$lt_dlunknown
>> cat > conftest.$ac_ext <<_LT_EOF
>> -#line 11608 "configure"
>> +#line 11613 "configure"
>> #include "confdefs.h"
>>
>> #if HAVE_DLFCN_H
>> @@ -24833,6 +24838,20 @@ esac
>> fi
>>
>>
>> +all_formats=
>> +# Check whether --enable-formats was given.
>> +if test "${enable_formats+set}" = set; then :
>> + enableval=$enable_formats; case "${enableval}" in
>> + yes | "") as_fn_error $? "enable-formats option must specify file formats or 'all'" "$LINENO" 5
>> + ;;
>> + no) enable_formats= ;;
>> + *) enable_formats=$enableval ;;
>> +esac
>> +else
>> + all_formats=true
>> +fi
> I think it would be simpler to drop `all_formats` here, and make the
> default case just set `enable_formats=all`. Then later on, when you
> process `enable_formats` (where you have to check for 'all' anyway),
> you'll spot the 'all' case there and do whatever needs doing. I have
> another note about that logic below.
This makes sense, I'm not sure what I was thinking originally tbh.
>
>> +
>> +
>> # Check whether --enable-64-bit-bfd was given.
>> if test "${enable_64_bit_bfd+set}" = set; then :
>> enableval=$enable_64_bit_bfd; case $enableval in #(
>> @@ -24915,11 +24934,20 @@ fi
>> TARGET_OBS=
>> all_targets=
>> HAVE_NATIVE_GCORE_TARGET=
>> +# File formats that will ne enabled based on the selected
> type: ne ?
fixed
>> +# target(s). These are chosen because most, if not all, executables for
>> +# the target will follow this file format so it makes no sense to support
>> +# the target but not the debug information.
>> +target_formats=
>> +# If all targets were requested, this is all formats that should accompany
>> +# them.
>> +all_target_formats="elf xcoff mips coff"
>>
>> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
>> do
>> if test "$targ_alias" = "all"; then
>> all_targets=true
>> + target_formats=$all_target_formats
>> else
>> # Canonicalize the secondary target names.
>> result=`$ac_config_sub $targ_alias 2>/dev/null`
>> @@ -24941,6 +24969,19 @@ fi
>> *" ${i} "*) ;;
>> *)
>> TARGET_OBS="$TARGET_OBS ${i}"
>> + # 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
> Is this required? Or is this left over debug output?
debug output, oops. fixed
>
>> + 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
> I think this logic should be moved into configure.tgt. We already have
> a block at the end of that file which makes some choices based on which
> files are being added to the object list, which is exactly what you're
> doing here. This change would make the formats required by a target be
> an output from configure.tgt, which makes sense to me.
This makes sense. I'll do it. This also helps with the issue of needing
to run this check after determining the result of --target!
>
>> ;;
>> esac
>> done
>> @@ -24964,6 +25005,7 @@ if test x${all_targets} = xtrue; then
>> else
>> TARGET_OBS='$(ALL_TARGET_OBS)'
>> fi
>> + target_readers=$all_target_readers
>> fi
>>
>> # AMD debugger API support.
>> @@ -31462,6 +31504,7 @@ fi
>> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
>> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>>
>> +support_elf=no
>> # Add ELF support to GDB, but only if BFD includes ELF support.
>>
>> OLD_CFLAGS=$CFLAGS
>> @@ -31514,7 +31557,7 @@ $as_echo "$gdb_cv_var_elf" >&6; }
>> LDFLAGS=$OLD_LDFLAGS
>> LIBS=$OLD_LIBS
>> 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"
>>
>> $as_echo "#define HAVE_ELF 1" >>confdefs.h
>> @@ -31578,9 +31621,11 @@ if test "$ac_res" != no; then :
>> fi
>>
>> fi
>> + support_elf=yes
>> fi
>>
>> # Add macho support to GDB, but only if BFD includes it.
>> +support_macho=no
>>
>> OLD_CFLAGS=$CFLAGS
>> OLD_LDFLAGS=$LDFLAGS
>> @@ -31632,9 +31677,38 @@ $as_echo "$gdb_cv_var_macho" >&6; }
>> LDFLAGS=$OLD_LDFLAGS
>> LIBS=$OLD_LIBS
>> if test "$gdb_cv_var_macho" = yes; then
>> - CONFIG_OBS="$CONFIG_OBS machoread.o"
>> + support_macho=yes
>> fi
>>
>> +FORMAT_OBS=
>> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
>> +
>> +if test "$all_formats" = "true"; then
>> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
> As Tom pointed out, right now we effectively have the list of format
> reading .c files twice.
>
> If instead of using ALL_FORMAT_OBS here we instead expanded ' all '
> inside enable_formats into the list of all known formats
> (e.g. $all_target_formats), then, when we run through the loop below,
> FORMAT_OBS would always be set to the complete list, right? Then we'd
> not need the file list in the Makefile at all, instead we just need
> all_target_formats defined in configure.ac and then handling for each
> value in configure.format.
$all_target_formats doesn't contain dbx, which is why things look
duplicated but aren't really.
I guess I could have something like non_target_formats to list all the
file formats that aren't required to compile some target, and then use
both to set FORMAT_OBS. This way in the future if we have other formats
that aren't required by a target, we can place them there. What do you
think?
>
>> +else
>> + # 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"
> Instead of 'echo', maybe try AC_MSG_WARN maybe? Or AC_MSG_NOTICE?
> Check out:
>
> https://www.gnu.org/software/autoconf/manual/autoconf-2.60/html_node/Printing-Messages.html
>
> for options.
I'll go with AC_MSG_WARN, so it's unlikely the user will not see it,
thanks for the suggestion!
>
>> + enable_formats="${enable_formats} $req"
>> + fi
>> + done
>> +
>> + for format in $enable_formats
>> + do
>> + if test "$format" = "all"; then
>> + all_formats=true
>> + fi
>> +
>> + . ${srcdir}/configure.format
>> + done
>> +fi
>> +
>> +echo $FORMAT_OBS
> Is this left over debug?
yes, removed.
>
>> +
>> +
>> +
>> # Add any host-specific objects to GDB.
>> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>>
>> diff --git a/gdb/configure.ac b/gdb/configure.ac
>> index 8368fea0423..5f5187ecd0f 100644
>> --- a/gdb/configure.ac
>> +++ b/gdb/configure.ac
>> @@ -187,6 +187,16 @@ AS_HELP_STRING([--enable-targets=TARGETS], [alternative target configurations]),
>> *) enable_targets=$enableval ;;
>> esac])
>>
>> +all_formats=
>> +AC_ARG_ENABLE(formats,
>> + AS_HELP_STRING([--enable-formats=FILE_FORMATS], [enable support for selected file formats(default 'all')]),
>> +[case "${enableval}" in
>> + yes | "") AC_MSG_ERROR(enable-formats option must specify file formats or 'all')
>> + ;;
>> + no) enable_formats= ;;
>> + *) enable_formats=$enableval ;;
>> +esac], [all_formats=true])
>> +
>> BFD_64_BIT
>>
>> # Provide defaults for some variables set by the per-host and per-target
>> @@ -206,11 +216,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 most, if not all, executables for
>> +# the target will follow this file format so it makes no sense to support
>> +# the target but not the debug information.
>> +target_formats=
>> +# If all targets were requested, this is all formats that should accompany
>> +# them.
>> +all_target_formats="elf xcoff mips coff"
>>
>> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
>> do
>> if test "$targ_alias" = "all"; then
>> all_targets=true
>> + target_formats=$all_target_formats
>> else
>> # Canonicalize the secondary target names.
>> result=`$ac_config_sub $targ_alias 2>/dev/null`
>> @@ -231,6 +250,19 @@ do
>> *" ${i} "*) ;;
>> *)
>> TARGET_OBS="$TARGET_OBS ${i}"
>> + # 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
>> ;;
>> esac
>> done
>> @@ -1850,11 +1882,12 @@ fi
>> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
>> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>>
>> +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"
> Maybe this is something we can unpick later, but if we only add all
> these .o files if we have ELF support. And if we chose to configure GDB
> without ELF support .... should we actually be dropping all these .o
> files?
I could also invert the logic of my change here. Instead of setting this
part of "elf support" and adding the .o file later, if I moved that
format part to be higher up, closer to the target stuff (where I had it
originally), I can have that section request the elf target, and this
part be fully skipped if elf wasn't requested (and error out if we can't
support but the user asked for it).
Same would go for Mach-O.
What do you think?
>
>> AC_DEFINE(HAVE_ELF, 1,
>> [Define if ELF support should be included.])
>> @@ -1862,15 +1895,46 @@ if test "$gdb_cv_var_elf" = yes; then
>> if test "$plugins" = "yes"; then
>> AC_SEARCH_LIBS(dlopen, dl)
>> fi
>> + support_elf=yes
>> fi
>>
>> # Add macho support to GDB, but only if BFD includes it.
>> +support_macho=no
>> 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
>> fi
>>
>> +FORMAT_OBS=
>> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
>> +
>> +if test "$all_formats" = "true"; then
>> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
>> +else
>> + # 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"
>> + enable_formats="${enable_formats} $req"
>> + fi
>> + done
>> +
>> + for format in $enable_formats
>> + do
>> + if test "$format" = "all"; then
>> + all_formats=true
>> + fi
> Maybe I'm missing something, but isn't setting 'all_formats' here too
> late? That is, we are already inside the 'else' block which checked the
> 'all_formats' variable, and after this point it's not (as far as I can
> tell) checked again.
You're right! I think this was a leftover from moving things around.
However, I think I like your idea of dealing with "all" inside
configure.format anyway, I just didn't do it at first because I was
copying what the target stuff does.
>> +
>> + . ${srcdir}/configure.format
>> + done
>> +fi
>> +
>> +echo $FORMAT_OBS
>> +
>> +AC_SUBST(FORMAT_OBS)
>> +
>> # Add any host-specific objects to GDB.
>> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>>
>> diff --git a/gdb/configure.format b/gdb/configure.format
>> new file mode 100644
>> index 00000000000..12dd2d25717
>> --- /dev/null
>> +++ b/gdb/configure.format
>> @@ -0,0 +1,41 @@
>> +# Copyright (C) 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 <http://www.gnu.org/licenses/>.
>> +
>> +# This file is used to decide which files need to be compiled to support
>> +# the requested file formats
>> +
>> +case $format in
>> + xcoff) FORMAT_OBS="$FORMAT_OBS xcoffread.o" ;;
> In all the other configure.* files the format used is like this:
>
> xcoff)
> FORMAT_OBS="$FORMAT_OBS xcoffread.o"
> ;;
>
> I think we should adopt this for consistency (I also prefer it, but lets
> go with the consistency argument).
Alright, will fix for the next version.
--
Cheers,
Guinevere Larsen
She/Her/Hers
>
> Thanks,
> Andrew
>
>> +
>> + # 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" ;;
>> +
>> + dbx) FORMAT_OBS="$FORMAT_OBS dbxread.o" ;;
>> +
>> + elf) if "$support_elf"="yes"; then
>> + FORMAT_OBS="$FORMAT_OBS elfread.o"
>> + fi
>> + ;;
>> +
>> + macho) if "$support_macho"="yes"; then
>> + FORMAT_OBS="$FORMAT_OBS machoread.o"
>> + fi
>> + ;;
>> +
>> + mips) FORMAT_OBS="$FORMAT_OBS mipsread.o" ;;
>> +esac
>> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
>> index 8d85a597ec8..793793601c1 100644
>> --- a/gdb/configure.tgt
>> +++ b/gdb/configure.tgt
>> @@ -507,7 +507,7 @@ powerpc-*-openbsd*)
>> ;;
>> powerpc-*-aix* | rs6000-*-* | powerpc64-*-aix*)
>> # Target: PowerPC running AIX
>> - gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o xcoffread.o \
>> + gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o \
>> ppc-sysv-tdep.o solib-aix.o \
>> ravenscar-thread.o ppc-ravenscar-thread.o"
>> ;;
>> @@ -523,8 +523,8 @@ powerpc*-*-linux*)
>> powerpc-*-lynx*178)
>> # Target: PowerPC running Lynx178.
>> gdb_target_obs="rs6000-tdep.o rs6000-lynx178-tdep.o \
>> - xcoffread.o ppc-sysv-tdep.o \
>> - ravenscar-thread.o ppc-ravenscar-thread.o"
>> + ppc-sysv-tdep.o ravenscar-thread.o \
>> + ppc-ravenscar-thread.o"
>> ;;
>> powerpc*-*-*)
>> # Target: PowerPC running eabi
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 77a4021b36a..c569e68060e 100644
>> --- 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
>> +Configure @value{GDBN} to support certain binary file formats. If a
>> +format is the main (or only) file format for a given target, the
>> +configure script may add support to it anyway, and warn the user.
>> +If not given, all file formats that @value{GDBN} supports are compiled.
>> +
>> @item --with-gdb-datadir=@var{path}
>> Set the @value{GDBN}-specific data directory. @value{GDBN} will look
>> here for certain supporting files or scripts. This defaults to the
>> --
>> 2.46.1
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-02 20:37 ` Guinevere Larsen
@ 2024-10-03 10:15 ` Andrew Burgess
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Burgess @ 2024-10-03 10:15 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches
Guinevere Larsen <guinevere@redhat.com> writes:
> On 10/2/24 10:56 AM, Andrew Burgess wrote:
>> Guinevere Larsen <guinevere@redhat.com> writes:
>>
>>> 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.
>>>
>>> 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.
>>>
>>> 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.
>>> ---
>>> gdb/Makefile.in | 22 +++++++-----
>>> gdb/NEWS | 11 ++++++
>>> gdb/README | 5 +++
>>> gdb/configure | 82 +++++++++++++++++++++++++++++++++++++++++---
>>> gdb/configure.ac | 68 ++++++++++++++++++++++++++++++++++--
>>> gdb/configure.format | 41 ++++++++++++++++++++++
>>> gdb/configure.tgt | 6 ++--
>>> gdb/doc/gdb.texinfo | 7 ++++
>>> 8 files changed, 225 insertions(+), 17 deletions(-)
>>> create mode 100644 gdb/configure.format
>>>
>>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>>> index bcf1ee45a70..009d68d6de2 100644
>>> --- a/gdb/Makefile.in
>>> +++ b/gdb/Makefile.in
>>> @@ -901,13 +901,24 @@ ALL_TARGET_OBS = \
>>> vax-tdep.o \
>>> windows-tdep.o \
>>> x86-tdep.o \
>>> - xcoffread.o \
>>> xstormy16-tdep.o \
>>> xtensa-config.o \
>>> xtensa-linux-tdep.o \
>>> xtensa-tdep.o \
>>> z80-tdep.o
>>>
>>> +# Object files for reading specific types of debug information.
>>> +FORMAT_OBS = @FORMAT_OBS@
>>> +
>>> +# 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
>>> +
>>> # The following native-target dependent variables are defined on
>>> # configure.nat.
>>> NAT_FILE = @NAT_FILE@
>>> @@ -1064,8 +1075,6 @@ COMMON_SFILES = \
>>> c-varobj.c \
>>> charset.c \
>>> cli-out.c \
>>> - coff-pe-read.c \
>>> - coffread.c \
>>> complaints.c \
>>> completer.c \
>>> copying.c \
>>> @@ -1079,7 +1088,6 @@ COMMON_SFILES = \
>>> d-lang.c \
>>> d-namespace.c \
>>> d-valprint.c \
>>> - dbxread.c \
>>> dcache.c \
>>> debug.c \
>>> debuginfod-support.c \
>>> @@ -1171,7 +1179,6 @@ COMMON_SFILES = \
>>> memtag.c \
>>> minidebug.c \
>>> minsyms.c \
>>> - mipsread.c \
>>> namespace.c \
>>> objc-lang.c \
>>> objfiles.c \
>>> @@ -1264,7 +1271,6 @@ SFILES = \
>>> d-exp.y \
>>> dtrace-probe.c \
>>> elf-none-tdep.c \
>>> - elfread.c \
>>> f-exp.y \
>>> gcore-elf.c \
>>> gdb.c \
>>> @@ -1886,7 +1892,6 @@ ALLDEPFILES = \
>>> windows-tdep.c \
>>> x86-nat.c \
>>> x86-tdep.c \
>>> - xcoffread.c \
>>> xstormy16-tdep.c \
>>> xtensa-config.c \
>>> xtensa-linux-nat.c \
>>> @@ -1908,7 +1913,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>>> $(SUBDIR_CLI_OBS) \
>>> $(SUBDIR_MI_OBS) \
>>> $(SUBDIR_TARGET_OBS) \
>>> - $(SUBDIR_GCC_COMPILE_OBS)
>>> + $(SUBDIR_GCC_COMPILE_OBS) \
>>> + $(FORMAT_OBS)
>>>
>>> SUBDIRS = doc @subdirs@ data-directory
>>> CLEANDIRS = $(SUBDIRS)
>>> 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.
>>> + 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.
>>> +
>>> *** Changes in GDB 15
>>>
>>> * The MPX commands "show/set mpx bound" have been deprecated, as Intel
>>> diff --git a/gdb/README b/gdb/README
>>> index d85c37d5d17..342b2d07eb7 100644
>>> --- 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`
>>> + Configure GDB to be unable to read some binary file formats, such as
>>> + coff, dbx or elf.
>>> +
>>> `--with-gdb-datadir=PATH'
>>> Set the GDB-specific data directory. GDB will look here for
>>> certain supporting files or scripts. This defaults to the `gdb'
>>> diff --git a/gdb/configure b/gdb/configure
>>> index 53eaad4f0e2..792e5cefefe 100755
>>> --- a/gdb/configure
>>> +++ b/gdb/configure
>>> @@ -706,6 +706,7 @@ LIBGUI
>>> LTLIBLZMA
>>> LIBLZMA
>>> HAVE_LIBLZMA
>>> +FORMAT_OBS
>>> SER_HARDWIRE
>>> WERROR_CFLAGS
>>> WARN_CFLAGS
>>> @@ -933,6 +934,7 @@ with_relocated_sources
>>> with_auto_load_dir
>>> with_auto_load_safe_path
>>> enable_targets
>>> +enable_formats
>>> enable_64_bit_bfd
>>> with_amd_dbgapi
>>> enable_tui
>>> @@ -1644,6 +1646,9 @@ Optional Features:
>>> --disable-nls do not use Native Language Support
>>> --enable-targets=TARGETS
>>> alternative target configurations
>>> + --enable-formats=FILE_FORMATS
>>> + enable support for selected file formats(default
>>> + 'all')
>>> --enable-64-bit-bfd 64-bit support (on hosts with narrower word sizes)
>>> --enable-tui enable full-screen terminal user interface (TUI)
>>> --enable-gdbtk enable gdbtk graphical user interface (GUI)
>>> @@ -11499,7 +11504,7 @@ else
>>> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>>> lt_status=$lt_dlunknown
>>> cat > conftest.$ac_ext <<_LT_EOF
>>> -#line 11502 "configure"
>>> +#line 11507 "configure"
>>> #include "confdefs.h"
>>>
>>> #if HAVE_DLFCN_H
>>> @@ -11605,7 +11610,7 @@ else
>>> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>>> lt_status=$lt_dlunknown
>>> cat > conftest.$ac_ext <<_LT_EOF
>>> -#line 11608 "configure"
>>> +#line 11613 "configure"
>>> #include "confdefs.h"
>>>
>>> #if HAVE_DLFCN_H
>>> @@ -24833,6 +24838,20 @@ esac
>>> fi
>>>
>>>
>>> +all_formats=
>>> +# Check whether --enable-formats was given.
>>> +if test "${enable_formats+set}" = set; then :
>>> + enableval=$enable_formats; case "${enableval}" in
>>> + yes | "") as_fn_error $? "enable-formats option must specify file formats or 'all'" "$LINENO" 5
>>> + ;;
>>> + no) enable_formats= ;;
>>> + *) enable_formats=$enableval ;;
>>> +esac
>>> +else
>>> + all_formats=true
>>> +fi
>> I think it would be simpler to drop `all_formats` here, and make the
>> default case just set `enable_formats=all`. Then later on, when you
>> process `enable_formats` (where you have to check for 'all' anyway),
>> you'll spot the 'all' case there and do whatever needs doing. I have
>> another note about that logic below.
> This makes sense, I'm not sure what I was thinking originally tbh.
>>
>>> +
>>> +
>>> # Check whether --enable-64-bit-bfd was given.
>>> if test "${enable_64_bit_bfd+set}" = set; then :
>>> enableval=$enable_64_bit_bfd; case $enableval in #(
>>> @@ -24915,11 +24934,20 @@ fi
>>> TARGET_OBS=
>>> all_targets=
>>> HAVE_NATIVE_GCORE_TARGET=
>>> +# File formats that will ne enabled based on the selected
>> type: ne ?
> fixed
>>> +# target(s). These are chosen because most, if not all, executables for
>>> +# the target will follow this file format so it makes no sense to support
>>> +# the target but not the debug information.
>>> +target_formats=
>>> +# If all targets were requested, this is all formats that should accompany
>>> +# them.
>>> +all_target_formats="elf xcoff mips coff"
>>>
>>> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
>>> do
>>> if test "$targ_alias" = "all"; then
>>> all_targets=true
>>> + target_formats=$all_target_formats
>>> else
>>> # Canonicalize the secondary target names.
>>> result=`$ac_config_sub $targ_alias 2>/dev/null`
>>> @@ -24941,6 +24969,19 @@ fi
>>> *" ${i} "*) ;;
>>> *)
>>> TARGET_OBS="$TARGET_OBS ${i}"
>>> + # 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
>> Is this required? Or is this left over debug output?
> debug output, oops. fixed
>>
>>> + 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
>> I think this logic should be moved into configure.tgt. We already have
>> a block at the end of that file which makes some choices based on which
>> files are being added to the object list, which is exactly what you're
>> doing here. This change would make the formats required by a target be
>> an output from configure.tgt, which makes sense to me.
> This makes sense. I'll do it. This also helps with the issue of needing
> to run this check after determining the result of --target!
>>
>>> ;;
>>> esac
>>> done
>>> @@ -24964,6 +25005,7 @@ if test x${all_targets} = xtrue; then
>>> else
>>> TARGET_OBS='$(ALL_TARGET_OBS)'
>>> fi
>>> + target_readers=$all_target_readers
>>> fi
>>>
>>> # AMD debugger API support.
>>> @@ -31462,6 +31504,7 @@ fi
>>> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
>>> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>>>
>>> +support_elf=no
>>> # Add ELF support to GDB, but only if BFD includes ELF support.
>>>
>>> OLD_CFLAGS=$CFLAGS
>>> @@ -31514,7 +31557,7 @@ $as_echo "$gdb_cv_var_elf" >&6; }
>>> LDFLAGS=$OLD_LDFLAGS
>>> LIBS=$OLD_LIBS
>>> 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"
>>>
>>> $as_echo "#define HAVE_ELF 1" >>confdefs.h
>>> @@ -31578,9 +31621,11 @@ if test "$ac_res" != no; then :
>>> fi
>>>
>>> fi
>>> + support_elf=yes
>>> fi
>>>
>>> # Add macho support to GDB, but only if BFD includes it.
>>> +support_macho=no
>>>
>>> OLD_CFLAGS=$CFLAGS
>>> OLD_LDFLAGS=$LDFLAGS
>>> @@ -31632,9 +31677,38 @@ $as_echo "$gdb_cv_var_macho" >&6; }
>>> LDFLAGS=$OLD_LDFLAGS
>>> LIBS=$OLD_LIBS
>>> if test "$gdb_cv_var_macho" = yes; then
>>> - CONFIG_OBS="$CONFIG_OBS machoread.o"
>>> + support_macho=yes
>>> fi
>>>
>>> +FORMAT_OBS=
>>> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
>>> +
>>> +if test "$all_formats" = "true"; then
>>> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
>> As Tom pointed out, right now we effectively have the list of format
>> reading .c files twice.
>>
>> If instead of using ALL_FORMAT_OBS here we instead expanded ' all '
>> inside enable_formats into the list of all known formats
>> (e.g. $all_target_formats), then, when we run through the loop below,
>> FORMAT_OBS would always be set to the complete list, right? Then we'd
>> not need the file list in the Makefile at all, instead we just need
>> all_target_formats defined in configure.ac and then handling for each
>> value in configure.format.
>
> $all_target_formats doesn't contain dbx, which is why things look
> duplicated but aren't really.
>
> I guess I could have something like non_target_formats to list all the
> file formats that aren't required to compile some target, and then use
> both to set FORMAT_OBS. This way in the future if we have other formats
> that aren't required by a target, we can place them there. What do you
> think?
Whatever works I guess. I'm sure there must be some way we can figure
out the list of require .o files from the configure script and then pass
that to the Makefile. It might mean a little more complexity in the
configure script, but I think the benefit of having the configure script
hold all the logic will be worth it.
Thanks,
Andrew
>
>>
>>> +else
>>> + # 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"
>> Instead of 'echo', maybe try AC_MSG_WARN maybe? Or AC_MSG_NOTICE?
>> Check out:
>>
>> https://www.gnu.org/software/autoconf/manual/autoconf-2.60/html_node/Printing-Messages.html
>>
>> for options.
> I'll go with AC_MSG_WARN, so it's unlikely the user will not see it,
> thanks for the suggestion!
>>
>>> + enable_formats="${enable_formats} $req"
>>> + fi
>>> + done
>>> +
>>> + for format in $enable_formats
>>> + do
>>> + if test "$format" = "all"; then
>>> + all_formats=true
>>> + fi
>>> +
>>> + . ${srcdir}/configure.format
>>> + done
>>> +fi
>>> +
>>> +echo $FORMAT_OBS
>> Is this left over debug?
> yes, removed.
>>
>>> +
>>> +
>>> +
>>> # Add any host-specific objects to GDB.
>>> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>>>
>>> diff --git a/gdb/configure.ac b/gdb/configure.ac
>>> index 8368fea0423..5f5187ecd0f 100644
>>> --- a/gdb/configure.ac
>>> +++ b/gdb/configure.ac
>>> @@ -187,6 +187,16 @@ AS_HELP_STRING([--enable-targets=TARGETS], [alternative target configurations]),
>>> *) enable_targets=$enableval ;;
>>> esac])
>>>
>>> +all_formats=
>>> +AC_ARG_ENABLE(formats,
>>> + AS_HELP_STRING([--enable-formats=FILE_FORMATS], [enable support for selected file formats(default 'all')]),
>>> +[case "${enableval}" in
>>> + yes | "") AC_MSG_ERROR(enable-formats option must specify file formats or 'all')
>>> + ;;
>>> + no) enable_formats= ;;
>>> + *) enable_formats=$enableval ;;
>>> +esac], [all_formats=true])
>>> +
>>> BFD_64_BIT
>>>
>>> # Provide defaults for some variables set by the per-host and per-target
>>> @@ -206,11 +216,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 most, if not all, executables for
>>> +# the target will follow this file format so it makes no sense to support
>>> +# the target but not the debug information.
>>> +target_formats=
>>> +# If all targets were requested, this is all formats that should accompany
>>> +# them.
>>> +all_target_formats="elf xcoff mips coff"
>>>
>>> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
>>> do
>>> if test "$targ_alias" = "all"; then
>>> all_targets=true
>>> + target_formats=$all_target_formats
>>> else
>>> # Canonicalize the secondary target names.
>>> result=`$ac_config_sub $targ_alias 2>/dev/null`
>>> @@ -231,6 +250,19 @@ do
>>> *" ${i} "*) ;;
>>> *)
>>> TARGET_OBS="$TARGET_OBS ${i}"
>>> + # 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
>>> ;;
>>> esac
>>> done
>>> @@ -1850,11 +1882,12 @@ fi
>>> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
>>> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>>>
>>> +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"
>> Maybe this is something we can unpick later, but if we only add all
>> these .o files if we have ELF support. And if we chose to configure GDB
>> without ELF support .... should we actually be dropping all these .o
>> files?
>
> I could also invert the logic of my change here. Instead of setting this
> part of "elf support" and adding the .o file later, if I moved that
> format part to be higher up, closer to the target stuff (where I had it
> originally), I can have that section request the elf target, and this
> part be fully skipped if elf wasn't requested (and error out if we can't
> support but the user asked for it).
>
> Same would go for Mach-O.
>
> What do you think?
>
>>
>>> AC_DEFINE(HAVE_ELF, 1,
>>> [Define if ELF support should be included.])
>>> @@ -1862,15 +1895,46 @@ if test "$gdb_cv_var_elf" = yes; then
>>> if test "$plugins" = "yes"; then
>>> AC_SEARCH_LIBS(dlopen, dl)
>>> fi
>>> + support_elf=yes
>>> fi
>>>
>>> # Add macho support to GDB, but only if BFD includes it.
>>> +support_macho=no
>>> 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
>>> fi
>>>
>>> +FORMAT_OBS=
>>> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
>>> +
>>> +if test "$all_formats" = "true"; then
>>> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
>>> +else
>>> + # 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"
>>> + enable_formats="${enable_formats} $req"
>>> + fi
>>> + done
>>> +
>>> + for format in $enable_formats
>>> + do
>>> + if test "$format" = "all"; then
>>> + all_formats=true
>>> + fi
>> Maybe I'm missing something, but isn't setting 'all_formats' here too
>> late? That is, we are already inside the 'else' block which checked the
>> 'all_formats' variable, and after this point it's not (as far as I can
>> tell) checked again.
> You're right! I think this was a leftover from moving things around.
> However, I think I like your idea of dealing with "all" inside
> configure.format anyway, I just didn't do it at first because I was
> copying what the target stuff does.
>>> +
>>> + . ${srcdir}/configure.format
>>> + done
>>> +fi
>>> +
>>> +echo $FORMAT_OBS
>>> +
>>> +AC_SUBST(FORMAT_OBS)
>>> +
>>> # Add any host-specific objects to GDB.
>>> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>>>
>>> diff --git a/gdb/configure.format b/gdb/configure.format
>>> new file mode 100644
>>> index 00000000000..12dd2d25717
>>> --- /dev/null
>>> +++ b/gdb/configure.format
>>> @@ -0,0 +1,41 @@
>>> +# Copyright (C) 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 <http://www.gnu.org/licenses/>.
>>> +
>>> +# This file is used to decide which files need to be compiled to support
>>> +# the requested file formats
>>> +
>>> +case $format in
>>> + xcoff) FORMAT_OBS="$FORMAT_OBS xcoffread.o" ;;
>> In all the other configure.* files the format used is like this:
>>
>> xcoff)
>> FORMAT_OBS="$FORMAT_OBS xcoffread.o"
>> ;;
>>
>> I think we should adopt this for consistency (I also prefer it, but lets
>> go with the consistency argument).
> Alright, will fix for the next version.
>
> --
> Cheers,
> Guinevere Larsen
> She/Her/Hers
>
>>
>> Thanks,
>> Andrew
>>
>>> +
>>> + # 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" ;;
>>> +
>>> + dbx) FORMAT_OBS="$FORMAT_OBS dbxread.o" ;;
>>> +
>>> + elf) if "$support_elf"="yes"; then
>>> + FORMAT_OBS="$FORMAT_OBS elfread.o"
>>> + fi
>>> + ;;
>>> +
>>> + macho) if "$support_macho"="yes"; then
>>> + FORMAT_OBS="$FORMAT_OBS machoread.o"
>>> + fi
>>> + ;;
>>> +
>>> + mips) FORMAT_OBS="$FORMAT_OBS mipsread.o" ;;
>>> +esac
>>> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
>>> index 8d85a597ec8..793793601c1 100644
>>> --- a/gdb/configure.tgt
>>> +++ b/gdb/configure.tgt
>>> @@ -507,7 +507,7 @@ powerpc-*-openbsd*)
>>> ;;
>>> powerpc-*-aix* | rs6000-*-* | powerpc64-*-aix*)
>>> # Target: PowerPC running AIX
>>> - gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o xcoffread.o \
>>> + gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o \
>>> ppc-sysv-tdep.o solib-aix.o \
>>> ravenscar-thread.o ppc-ravenscar-thread.o"
>>> ;;
>>> @@ -523,8 +523,8 @@ powerpc*-*-linux*)
>>> powerpc-*-lynx*178)
>>> # Target: PowerPC running Lynx178.
>>> gdb_target_obs="rs6000-tdep.o rs6000-lynx178-tdep.o \
>>> - xcoffread.o ppc-sysv-tdep.o \
>>> - ravenscar-thread.o ppc-ravenscar-thread.o"
>>> + ppc-sysv-tdep.o ravenscar-thread.o \
>>> + ppc-ravenscar-thread.o"
>>> ;;
>>> powerpc*-*-*)
>>> # Target: PowerPC running eabi
>>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>>> index 77a4021b36a..c569e68060e 100644
>>> --- 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
>>> +Configure @value{GDBN} to support certain binary file formats. If a
>>> +format is the main (or only) file format for a given target, the
>>> +configure script may add support to it anyway, and warn the user.
>>> +If not given, all file formats that @value{GDBN} supports are compiled.
>>> +
>>> @item --with-gdb-datadir=@var{path}
>>> Set the @value{GDBN}-specific data directory. @value{GDBN} will look
>>> here for certain supporting files or scripts. This defaults to the
>>> --
>>> 2.46.1
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-09-25 17:53 [PATCH] gdb, configure: Add disable-formats option for configure Guinevere Larsen
` (2 preceding siblings ...)
2024-10-02 13:56 ` Andrew Burgess
@ 2024-10-04 14:49 ` Andrew Burgess
2024-10-10 20:18 ` Guinevere Larsen
3 siblings, 1 reply; 30+ messages in thread
From: Andrew Burgess @ 2024-10-04 14:49 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches; +Cc: Guinevere Larsen
Hey Gwen,
Sorry for starting a second review thread, but while responding to Eli I
had some additional thoughts:
Guinevere Larsen <guinevere@redhat.com> writes:
> 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.
>
> 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.
>
> 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.
I think it would be useful if the commit message actually mentioned the
new configure option, and what values it takes.
But in addition, I ran into a couple of issues, including a crash.
I built gdbserver on a Windows machine, configured for
x86_64-w64-mingw32, and I built GDB on a Linux machine configured _only_
for x86_64-redhat-linux with --enable-formats=elf.
Then I start GDB and:
(gdb) set target-file-system-kind dos-based
(gdb) target remote 192.168.129.25:55443
Remote debugging using 192.168.129.25:55443
Reading C:/msys64/home/andrew/segv.exe from remote target...
warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
Reading C:/msys64/home/andrew/segv.exe from remote target...
Reading symbols from target:C:/msys64/home/andrew/segv.exe...
warning: I'm sorry, Dave, I can't do that. Symbol format `pei-x86-64' unknown.
Here's the first issue. I suspect that the above warning was probably
never expected to actually be seen. But if we start removing file
format support then it can show up more often. I think this message
needs to be made clearer, and dare I say it, more professional?
But moving on ... after downloading the library files, I ran straight
into this crash:
Fatal signal: Segmentation fault
----- Backtrace -----
0x4fd7c3 gdb_internal_backtrace_1
../../src/gdb/bt-utils.c:121
0x4fd7c3 _Z22gdb_internal_backtracev
../../src/gdb/bt-utils.c:167
0x5fa5a9 handle_fatal_signal
../../src/gdb/event-top.c:917
0x5fa63f handle_sigsegv
../../src/gdb/event-top.c:990
0x7fd7dce67b1f ???
/usr/src/debug/glibc-2.30-73-gd59630f995/nptl/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
0x7200e7 _ZNK11obj_section4addrEv
../../src/gdb/objfiles.h:381
0x7200e7 sort_cmp
../../src/gdb/objfiles.c:823
0x723775 _ZN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPK11obj_sectionS4_EEclIPPS2_SA_EEbT_T0_
/usr/include/c++/9/bits/predefined_ops.h:143
0x723775 _ZSt22__move_median_to_firstIPP11obj_sectionN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPKS0_S7_EEEEvT_SB_SB_SB_T0_
/usr/include/c++/9/bits/stl_algo.h:81
0x723775 _ZSt27__unguarded_partition_pivotIPP11obj_sectionN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPKS0_S7_EEEET_SB_SB_T0_
/usr/include/c++/9/bits/stl_algo.h:1920
0x723775 _ZSt16__introsort_loopIPP11obj_sectionlN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPKS0_S7_EEEEvT_SB_T0_T1_
/usr/include/c++/9/bits/stl_algo.h:1952
0x722184 _ZSt6__sortIPP11obj_sectionN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPKS0_S7_EEEEvT_SB_T0_
/usr/include/c++/9/bits/stl_algo.h:1967
0x722184 _ZSt4sortIPP11obj_sectionPFbPKS0_S4_EEvT_S7_T0_
/usr/include/c++/9/bits/stl_algo.h:4899
0x722184 update_section_map
../../src/gdb/objfiles.c:1090
0x722184 _Z15find_pc_sectionm
../../src/gdb/objfiles.c:1137
0x70f619 _Z35lookup_minimal_symbol_by_pc_sectionmP11obj_section18lookup_msym_preferP20bound_minimal_symbol
../../src/gdb/minsyms.c:771
0x82fccf _Z28find_pc_sect_compunit_symtabmP11obj_section
../../src/gdb/symtab.c:2914
0x61ee2c _Z12select_frameRK14frame_info_ptr
../../src/gdb/frame.c:1994
0x68a373 _Z11normal_stopv
../../src/gdb/infrun.c:9620
0x69a9c3 _Z12start_remotei
../../src/gdb/infrun.c:3832
0x7c3d03 _ZN13remote_target14start_remote_1Eii
../../src/gdb/remote.c:5350
0x7c43d6 _ZN13remote_target12start_remoteEii
../../src/gdb/remote.c:5441
0x7c43d6 _ZN13remote_target6open_1EPKcii
../../src/gdb/remote.c:6312
0x86c6de open_target
../../src/gdb/target.c:838
0x52c0a4 _Z8cmd_funcP16cmd_list_elementPKci
../../src/gdb/cli/cli-decode.c:2741
0x87aa7e _Z15execute_commandPKci
../../src/gdb/top.c:570
0x5fad3f _Z15command_handlerPKc
../../src/gdb/event-top.c:580
0x5fbe2d _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
../../src/gdb/event-top.c:816
0x5fb6d6 gdb_rl_callback_handler
../../src/gdb/event-top.c:272
0x92bc87 rl_callback_read_char
../../../src/readline/readline/callback.c:290
0x5fa8dd gdb_rl_callback_read_char_wrapper_noexcept
../../src/gdb/event-top.c:197
0x5fb58d gdb_rl_callback_read_char_wrapper
../../src/gdb/event-top.c:236
0x8afccf stdin_event_handler
../../src/gdb/ui.c:154
Which appears to be crashing in:
CORE_ADDR obj_section::addr () const
{
return bfd_section_vma (this->the_bfd_section) + this->offset ();
}
When trying to access 'this->offset ()' (I think):
(top-gdb) list .
376 void set_offset (CORE_ADDR offset);
377
378 /* The memory address of the section (vma + offset). */
379 CORE_ADDR addr () const
380 {
381 return bfd_section_vma (this->the_bfd_section) + this->offset ();
382 }
383
384 /* The one-passed-the-end memory address of the section
385 (vma + size + offset). */
(top-gdb) p this->objfile->sect_index_text
$24 = -1
Though I don't understand why this is segfaulting rather than throwing
an internal error from:
#define SECT_OFF_TEXT(objfile) \
((objfile->sect_index_text == -1) \
? (internal_error (_("sect_index_text not initialized")), -1) \
: objfile->sect_index_text)
Anyway, I think this probably needs investigating. My guess would be
that, when GDB can't open the file, (e.g. "I'm sorry Dave, ...") then we
shouldn't be processing the file beyond this point, but it looks like we
might be trying to anyway ... which feels wrong.
Thanks,
Andrew
> ---
> gdb/Makefile.in | 22 +++++++-----
> gdb/NEWS | 11 ++++++
> gdb/README | 5 +++
> gdb/configure | 82 +++++++++++++++++++++++++++++++++++++++++---
> gdb/configure.ac | 68 ++++++++++++++++++++++++++++++++++--
> gdb/configure.format | 41 ++++++++++++++++++++++
> gdb/configure.tgt | 6 ++--
> gdb/doc/gdb.texinfo | 7 ++++
> 8 files changed, 225 insertions(+), 17 deletions(-)
> create mode 100644 gdb/configure.format
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index bcf1ee45a70..009d68d6de2 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -901,13 +901,24 @@ ALL_TARGET_OBS = \
> vax-tdep.o \
> windows-tdep.o \
> x86-tdep.o \
> - xcoffread.o \
> xstormy16-tdep.o \
> xtensa-config.o \
> xtensa-linux-tdep.o \
> xtensa-tdep.o \
> z80-tdep.o
>
> +# Object files for reading specific types of debug information.
> +FORMAT_OBS = @FORMAT_OBS@
> +
> +# 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
> +
> # The following native-target dependent variables are defined on
> # configure.nat.
> NAT_FILE = @NAT_FILE@
> @@ -1064,8 +1075,6 @@ COMMON_SFILES = \
> c-varobj.c \
> charset.c \
> cli-out.c \
> - coff-pe-read.c \
> - coffread.c \
> complaints.c \
> completer.c \
> copying.c \
> @@ -1079,7 +1088,6 @@ COMMON_SFILES = \
> d-lang.c \
> d-namespace.c \
> d-valprint.c \
> - dbxread.c \
> dcache.c \
> debug.c \
> debuginfod-support.c \
> @@ -1171,7 +1179,6 @@ COMMON_SFILES = \
> memtag.c \
> minidebug.c \
> minsyms.c \
> - mipsread.c \
> namespace.c \
> objc-lang.c \
> objfiles.c \
> @@ -1264,7 +1271,6 @@ SFILES = \
> d-exp.y \
> dtrace-probe.c \
> elf-none-tdep.c \
> - elfread.c \
> f-exp.y \
> gcore-elf.c \
> gdb.c \
> @@ -1886,7 +1892,6 @@ ALLDEPFILES = \
> windows-tdep.c \
> x86-nat.c \
> x86-tdep.c \
> - xcoffread.c \
> xstormy16-tdep.c \
> xtensa-config.c \
> xtensa-linux-nat.c \
> @@ -1908,7 +1913,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
> $(SUBDIR_CLI_OBS) \
> $(SUBDIR_MI_OBS) \
> $(SUBDIR_TARGET_OBS) \
> - $(SUBDIR_GCC_COMPILE_OBS)
> + $(SUBDIR_GCC_COMPILE_OBS) \
> + $(FORMAT_OBS)
>
> SUBDIRS = doc @subdirs@ data-directory
> CLEANDIRS = $(SUBDIRS)
> 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.
> + 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.
> +
> *** Changes in GDB 15
>
> * The MPX commands "show/set mpx bound" have been deprecated, as Intel
> diff --git a/gdb/README b/gdb/README
> index d85c37d5d17..342b2d07eb7 100644
> --- 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`
> + Configure GDB to be unable to read some binary file formats, such as
> + coff, dbx or elf.
> +
> `--with-gdb-datadir=PATH'
> Set the GDB-specific data directory. GDB will look here for
> certain supporting files or scripts. This defaults to the `gdb'
> diff --git a/gdb/configure b/gdb/configure
> index 53eaad4f0e2..792e5cefefe 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -706,6 +706,7 @@ LIBGUI
> LTLIBLZMA
> LIBLZMA
> HAVE_LIBLZMA
> +FORMAT_OBS
> SER_HARDWIRE
> WERROR_CFLAGS
> WARN_CFLAGS
> @@ -933,6 +934,7 @@ with_relocated_sources
> with_auto_load_dir
> with_auto_load_safe_path
> enable_targets
> +enable_formats
> enable_64_bit_bfd
> with_amd_dbgapi
> enable_tui
> @@ -1644,6 +1646,9 @@ Optional Features:
> --disable-nls do not use Native Language Support
> --enable-targets=TARGETS
> alternative target configurations
> + --enable-formats=FILE_FORMATS
> + enable support for selected file formats(default
> + 'all')
> --enable-64-bit-bfd 64-bit support (on hosts with narrower word sizes)
> --enable-tui enable full-screen terminal user interface (TUI)
> --enable-gdbtk enable gdbtk graphical user interface (GUI)
> @@ -11499,7 +11504,7 @@ else
> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
> lt_status=$lt_dlunknown
> cat > conftest.$ac_ext <<_LT_EOF
> -#line 11502 "configure"
> +#line 11507 "configure"
> #include "confdefs.h"
>
> #if HAVE_DLFCN_H
> @@ -11605,7 +11610,7 @@ else
> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
> lt_status=$lt_dlunknown
> cat > conftest.$ac_ext <<_LT_EOF
> -#line 11608 "configure"
> +#line 11613 "configure"
> #include "confdefs.h"
>
> #if HAVE_DLFCN_H
> @@ -24833,6 +24838,20 @@ esac
> fi
>
>
> +all_formats=
> +# Check whether --enable-formats was given.
> +if test "${enable_formats+set}" = set; then :
> + enableval=$enable_formats; case "${enableval}" in
> + yes | "") as_fn_error $? "enable-formats option must specify file formats or 'all'" "$LINENO" 5
> + ;;
> + no) enable_formats= ;;
> + *) enable_formats=$enableval ;;
> +esac
> +else
> + all_formats=true
> +fi
> +
> +
> # Check whether --enable-64-bit-bfd was given.
> if test "${enable_64_bit_bfd+set}" = set; then :
> enableval=$enable_64_bit_bfd; case $enableval in #(
> @@ -24915,11 +24934,20 @@ fi
> TARGET_OBS=
> all_targets=
> HAVE_NATIVE_GCORE_TARGET=
> +# File formats that will ne enabled based on the selected
> +# target(s). These are chosen because most, if not all, executables for
> +# the target will follow this file format so it makes no sense to support
> +# the target but not the debug information.
> +target_formats=
> +# If all targets were requested, this is all formats that should accompany
> +# them.
> +all_target_formats="elf xcoff mips coff"
>
> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
> do
> if test "$targ_alias" = "all"; then
> all_targets=true
> + target_formats=$all_target_formats
> else
> # Canonicalize the secondary target names.
> result=`$ac_config_sub $targ_alias 2>/dev/null`
> @@ -24941,6 +24969,19 @@ fi
> *" ${i} "*) ;;
> *)
> TARGET_OBS="$TARGET_OBS ${i}"
> + # 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
> ;;
> esac
> done
> @@ -24964,6 +25005,7 @@ if test x${all_targets} = xtrue; then
> else
> TARGET_OBS='$(ALL_TARGET_OBS)'
> fi
> + target_readers=$all_target_readers
> fi
>
> # AMD debugger API support.
> @@ -31462,6 +31504,7 @@ fi
> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>
> +support_elf=no
> # Add ELF support to GDB, but only if BFD includes ELF support.
>
> OLD_CFLAGS=$CFLAGS
> @@ -31514,7 +31557,7 @@ $as_echo "$gdb_cv_var_elf" >&6; }
> LDFLAGS=$OLD_LDFLAGS
> LIBS=$OLD_LIBS
> 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"
>
> $as_echo "#define HAVE_ELF 1" >>confdefs.h
> @@ -31578,9 +31621,11 @@ if test "$ac_res" != no; then :
> fi
>
> fi
> + support_elf=yes
> fi
>
> # Add macho support to GDB, but only if BFD includes it.
> +support_macho=no
>
> OLD_CFLAGS=$CFLAGS
> OLD_LDFLAGS=$LDFLAGS
> @@ -31632,9 +31677,38 @@ $as_echo "$gdb_cv_var_macho" >&6; }
> LDFLAGS=$OLD_LDFLAGS
> LIBS=$OLD_LIBS
> if test "$gdb_cv_var_macho" = yes; then
> - CONFIG_OBS="$CONFIG_OBS machoread.o"
> + support_macho=yes
> fi
>
> +FORMAT_OBS=
> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
> +
> +if test "$all_formats" = "true"; then
> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
> +else
> + # 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"
> + enable_formats="${enable_formats} $req"
> + fi
> + done
> +
> + for format in $enable_formats
> + do
> + if test "$format" = "all"; then
> + all_formats=true
> + fi
> +
> + . ${srcdir}/configure.format
> + done
> +fi
> +
> +echo $FORMAT_OBS
> +
> +
> +
> # Add any host-specific objects to GDB.
> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 8368fea0423..5f5187ecd0f 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -187,6 +187,16 @@ AS_HELP_STRING([--enable-targets=TARGETS], [alternative target configurations]),
> *) enable_targets=$enableval ;;
> esac])
>
> +all_formats=
> +AC_ARG_ENABLE(formats,
> + AS_HELP_STRING([--enable-formats=FILE_FORMATS], [enable support for selected file formats(default 'all')]),
> +[case "${enableval}" in
> + yes | "") AC_MSG_ERROR(enable-formats option must specify file formats or 'all')
> + ;;
> + no) enable_formats= ;;
> + *) enable_formats=$enableval ;;
> +esac], [all_formats=true])
> +
> BFD_64_BIT
>
> # Provide defaults for some variables set by the per-host and per-target
> @@ -206,11 +216,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 most, if not all, executables for
> +# the target will follow this file format so it makes no sense to support
> +# the target but not the debug information.
> +target_formats=
> +# If all targets were requested, this is all formats that should accompany
> +# them.
> +all_target_formats="elf xcoff mips coff"
>
> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
> do
> if test "$targ_alias" = "all"; then
> all_targets=true
> + target_formats=$all_target_formats
> else
> # Canonicalize the secondary target names.
> result=`$ac_config_sub $targ_alias 2>/dev/null`
> @@ -231,6 +250,19 @@ do
> *" ${i} "*) ;;
> *)
> TARGET_OBS="$TARGET_OBS ${i}"
> + # 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
> ;;
> esac
> done
> @@ -1850,11 +1882,12 @@ fi
> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>
> +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"
> AC_DEFINE(HAVE_ELF, 1,
> [Define if ELF support should be included.])
> @@ -1862,15 +1895,46 @@ if test "$gdb_cv_var_elf" = yes; then
> if test "$plugins" = "yes"; then
> AC_SEARCH_LIBS(dlopen, dl)
> fi
> + support_elf=yes
> fi
>
> # Add macho support to GDB, but only if BFD includes it.
> +support_macho=no
> 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
> fi
>
> +FORMAT_OBS=
> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
> +
> +if test "$all_formats" = "true"; then
> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
> +else
> + # 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"
> + enable_formats="${enable_formats} $req"
> + fi
> + done
> +
> + for format in $enable_formats
> + do
> + if test "$format" = "all"; then
> + all_formats=true
> + fi
> +
> + . ${srcdir}/configure.format
> + done
> +fi
> +
> +echo $FORMAT_OBS
> +
> +AC_SUBST(FORMAT_OBS)
> +
> # Add any host-specific objects to GDB.
> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>
> diff --git a/gdb/configure.format b/gdb/configure.format
> new file mode 100644
> index 00000000000..12dd2d25717
> --- /dev/null
> +++ b/gdb/configure.format
> @@ -0,0 +1,41 @@
> +# Copyright (C) 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 <http://www.gnu.org/licenses/>.
> +
> +# This file is used to decide which files need to be compiled to support
> +# the requested file formats
> +
> +case $format in
> + xcoff) FORMAT_OBS="$FORMAT_OBS xcoffread.o" ;;
> +
> + # 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" ;;
> +
> + dbx) FORMAT_OBS="$FORMAT_OBS dbxread.o" ;;
> +
> + elf) if "$support_elf"="yes"; then
> + FORMAT_OBS="$FORMAT_OBS elfread.o"
> + fi
> + ;;
> +
> + macho) if "$support_macho"="yes"; then
> + FORMAT_OBS="$FORMAT_OBS machoread.o"
> + fi
> + ;;
> +
> + mips) FORMAT_OBS="$FORMAT_OBS mipsread.o" ;;
> +esac
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index 8d85a597ec8..793793601c1 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -507,7 +507,7 @@ powerpc-*-openbsd*)
> ;;
> powerpc-*-aix* | rs6000-*-* | powerpc64-*-aix*)
> # Target: PowerPC running AIX
> - gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o xcoffread.o \
> + gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o \
> ppc-sysv-tdep.o solib-aix.o \
> ravenscar-thread.o ppc-ravenscar-thread.o"
> ;;
> @@ -523,8 +523,8 @@ powerpc*-*-linux*)
> powerpc-*-lynx*178)
> # Target: PowerPC running Lynx178.
> gdb_target_obs="rs6000-tdep.o rs6000-lynx178-tdep.o \
> - xcoffread.o ppc-sysv-tdep.o \
> - ravenscar-thread.o ppc-ravenscar-thread.o"
> + ppc-sysv-tdep.o ravenscar-thread.o \
> + ppc-ravenscar-thread.o"
> ;;
> powerpc*-*-*)
> # Target: PowerPC running eabi
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 77a4021b36a..c569e68060e 100644
> --- 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
> +Configure @value{GDBN} to support certain binary file formats. If a
> +format is the main (or only) file format for a given target, the
> +configure script may add support to it anyway, and warn the user.
> +If not given, all file formats that @value{GDBN} supports are compiled.
> +
> @item --with-gdb-datadir=@var{path}
> Set the @value{GDBN}-specific data directory. @value{GDBN} will look
> here for certain supporting files or scripts. This defaults to the
> --
> 2.46.1
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-04 14:49 ` Andrew Burgess
@ 2024-10-10 20:18 ` Guinevere Larsen
2024-10-16 10:50 ` Andrew Burgess
0 siblings, 1 reply; 30+ messages in thread
From: Guinevere Larsen @ 2024-10-10 20:18 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 10/4/24 11:49 AM, Andrew Burgess wrote:
> Hey Gwen,
>
> Sorry for starting a second review thread, but while responding to Eli I
> had some additional thoughts:
>
> Guinevere Larsen <guinevere@redhat.com> writes:
>
>> 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.
>>
>> 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.
>>
>> 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.
> I think it would be useful if the commit message actually mentioned the
> new configure option, and what values it takes.
>
> But in addition, I ran into a couple of issues, including a crash.
>
> I built gdbserver on a Windows machine, configured for
> x86_64-w64-mingw32, and I built GDB on a Linux machine configured _only_
> for x86_64-redhat-linux with --enable-formats=elf.
>
> Then I start GDB and:
>
> (gdb) set target-file-system-kind dos-based
> (gdb) target remote 192.168.129.25:55443
> Remote debugging using 192.168.129.25:55443
> Reading C:/msys64/home/andrew/segv.exe from remote target...
> warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
> Reading C:/msys64/home/andrew/segv.exe from remote target...
> Reading symbols from target:C:/msys64/home/andrew/segv.exe...
> warning: I'm sorry, Dave, I can't do that. Symbol format `pei-x86-64' unknown.
>
> Here's the first issue. I suspect that the above warning was probably
> never expected to actually be seen. But if we start removing file
> format support then it can show up more often. I think this message
> needs to be made clearer, and dare I say it, more professional?
I agree, we should have something straight-forward saying the format is
not supported.
>
> But moving on ... after downloading the library files, I ran straight
> into this crash:
>
> Fatal signal: Segmentation fault
> ----- Backtrace -----
> 0x4fd7c3 gdb_internal_backtrace_1
> ../../src/gdb/bt-utils.c:121
> 0x4fd7c3 _Z22gdb_internal_backtracev
> ../../src/gdb/bt-utils.c:167
> 0x5fa5a9 handle_fatal_signal
> ../../src/gdb/event-top.c:917
> 0x5fa63f handle_sigsegv
> ../../src/gdb/event-top.c:990
> 0x7fd7dce67b1f ???
> /usr/src/debug/glibc-2.30-73-gd59630f995/nptl/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
> 0x7200e7 _ZNK11obj_section4addrEv
> ../../src/gdb/objfiles.h:381
> 0x7200e7 sort_cmp
> ../../src/gdb/objfiles.c:823
> 0x723775 _ZN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPK11obj_sectionS4_EEclIPPS2_SA_EEbT_T0_
> /usr/include/c++/9/bits/predefined_ops.h:143
> 0x723775 _ZSt22__move_median_to_firstIPP11obj_sectionN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPKS0_S7_EEEEvT_SB_SB_SB_T0_
> /usr/include/c++/9/bits/stl_algo.h:81
> 0x723775 _ZSt27__unguarded_partition_pivotIPP11obj_sectionN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPKS0_S7_EEEET_SB_SB_T0_
> /usr/include/c++/9/bits/stl_algo.h:1920
> 0x723775 _ZSt16__introsort_loopIPP11obj_sectionlN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPKS0_S7_EEEEvT_SB_T0_T1_
> /usr/include/c++/9/bits/stl_algo.h:1952
> 0x722184 _ZSt6__sortIPP11obj_sectionN9__gnu_cxx5__ops15_Iter_comp_iterIPFbPKS0_S7_EEEEvT_SB_T0_
> /usr/include/c++/9/bits/stl_algo.h:1967
> 0x722184 _ZSt4sortIPP11obj_sectionPFbPKS0_S4_EEvT_S7_T0_
> /usr/include/c++/9/bits/stl_algo.h:4899
> 0x722184 update_section_map
> ../../src/gdb/objfiles.c:1090
> 0x722184 _Z15find_pc_sectionm
> ../../src/gdb/objfiles.c:1137
> 0x70f619 _Z35lookup_minimal_symbol_by_pc_sectionmP11obj_section18lookup_msym_preferP20bound_minimal_symbol
> ../../src/gdb/minsyms.c:771
> 0x82fccf _Z28find_pc_sect_compunit_symtabmP11obj_section
> ../../src/gdb/symtab.c:2914
> 0x61ee2c _Z12select_frameRK14frame_info_ptr
> ../../src/gdb/frame.c:1994
> 0x68a373 _Z11normal_stopv
> ../../src/gdb/infrun.c:9620
> 0x69a9c3 _Z12start_remotei
> ../../src/gdb/infrun.c:3832
> 0x7c3d03 _ZN13remote_target14start_remote_1Eii
> ../../src/gdb/remote.c:5350
> 0x7c43d6 _ZN13remote_target12start_remoteEii
> ../../src/gdb/remote.c:5441
> 0x7c43d6 _ZN13remote_target6open_1EPKcii
> ../../src/gdb/remote.c:6312
> 0x86c6de open_target
> ../../src/gdb/target.c:838
> 0x52c0a4 _Z8cmd_funcP16cmd_list_elementPKci
> ../../src/gdb/cli/cli-decode.c:2741
> 0x87aa7e _Z15execute_commandPKci
> ../../src/gdb/top.c:570
> 0x5fad3f _Z15command_handlerPKc
> ../../src/gdb/event-top.c:580
> 0x5fbe2d _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
> ../../src/gdb/event-top.c:816
> 0x5fb6d6 gdb_rl_callback_handler
> ../../src/gdb/event-top.c:272
> 0x92bc87 rl_callback_read_char
> ../../../src/readline/readline/callback.c:290
> 0x5fa8dd gdb_rl_callback_read_char_wrapper_noexcept
> ../../src/gdb/event-top.c:197
> 0x5fb58d gdb_rl_callback_read_char_wrapper
> ../../src/gdb/event-top.c:236
> 0x8afccf stdin_event_handler
> ../../src/gdb/ui.c:154
>
> Which appears to be crashing in:
>
> CORE_ADDR obj_section::addr () const
> {
> return bfd_section_vma (this->the_bfd_section) + this->offset ();
> }
>
> When trying to access 'this->offset ()' (I think):
>
> (top-gdb) list .
> 376 void set_offset (CORE_ADDR offset);
> 377
> 378 /* The memory address of the section (vma + offset). */
> 379 CORE_ADDR addr () const
> 380 {
> 381 return bfd_section_vma (this->the_bfd_section) + this->offset ();
> 382 }
> 383
> 384 /* The one-passed-the-end memory address of the section
> 385 (vma + size + offset). */
> (top-gdb) p this->objfile->sect_index_text
> $24 = -1
>
> Though I don't understand why this is segfaulting rather than throwing
> an internal error from:
>
> #define SECT_OFF_TEXT(objfile) \
> ((objfile->sect_index_text == -1) \
> ? (internal_error (_("sect_index_text not initialized")), -1) \
> : objfile->sect_index_text)
>
> Anyway, I think this probably needs investigating. My guess would be
> that, when GDB can't open the file, (e.g. "I'm sorry Dave, ...") then we
> shouldn't be processing the file beyond this point, but it looks like we
> might be trying to anyway ... which feels wrong.
I managed to reproduce a crash on Linux with a similar setup, but my
backtrace looks a little different. In my version, GDB is trying to
setup solib hooks as a "post start inferior" step, whereas on yours it
seems that GDB is trying to print the location at which the inferior is
stopped.
In both cases, though, what happens is that we're calling
find_pc_section, which will try to create a map of sections for quick
lookup, but since we don't understand the file format, GDB thinks we
have no sections (and at least in the linux case, the pointer to the
sections is null). so the issue is not that the offset is -1, but rather
that this->the_bfd_section is 0x0.
If I'm correct that this is the cause of the crash on windows as well as
linux, I think this should fix the crash. Could you sanity check it for me?
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 0e076fe36be..cc8bb253d11 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -1053,6 +1053,11 @@ update_section_map (struct program_space *pspace,
gdb_assert (pspace_info->section_map_dirty != 0
|| pspace_info->new_objfiles_available != 0);
+ /* If there are 0 sections, or the point to the sections is null, it
makes
+ no sense to try and have a section map at all. */
+ if (pspace_info->num_sections == 0 || pspace_info->sections == nullptr)
+ return;
+
map = *pmap;
xfree (map);
--
Cheers,
Guinevere Larsen
She/Her/Hers
>
> Thanks,
> Andrew
>
>> ---
>> gdb/Makefile.in | 22 +++++++-----
>> gdb/NEWS | 11 ++++++
>> gdb/README | 5 +++
>> gdb/configure | 82 +++++++++++++++++++++++++++++++++++++++++---
>> gdb/configure.ac | 68 ++++++++++++++++++++++++++++++++++--
>> gdb/configure.format | 41 ++++++++++++++++++++++
>> gdb/configure.tgt | 6 ++--
>> gdb/doc/gdb.texinfo | 7 ++++
>> 8 files changed, 225 insertions(+), 17 deletions(-)
>> create mode 100644 gdb/configure.format
>>
>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>> index bcf1ee45a70..009d68d6de2 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -901,13 +901,24 @@ ALL_TARGET_OBS = \
>> vax-tdep.o \
>> windows-tdep.o \
>> x86-tdep.o \
>> - xcoffread.o \
>> xstormy16-tdep.o \
>> xtensa-config.o \
>> xtensa-linux-tdep.o \
>> xtensa-tdep.o \
>> z80-tdep.o
>>
>> +# Object files for reading specific types of debug information.
>> +FORMAT_OBS = @FORMAT_OBS@
>> +
>> +# 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
>> +
>> # The following native-target dependent variables are defined on
>> # configure.nat.
>> NAT_FILE = @NAT_FILE@
>> @@ -1064,8 +1075,6 @@ COMMON_SFILES = \
>> c-varobj.c \
>> charset.c \
>> cli-out.c \
>> - coff-pe-read.c \
>> - coffread.c \
>> complaints.c \
>> completer.c \
>> copying.c \
>> @@ -1079,7 +1088,6 @@ COMMON_SFILES = \
>> d-lang.c \
>> d-namespace.c \
>> d-valprint.c \
>> - dbxread.c \
>> dcache.c \
>> debug.c \
>> debuginfod-support.c \
>> @@ -1171,7 +1179,6 @@ COMMON_SFILES = \
>> memtag.c \
>> minidebug.c \
>> minsyms.c \
>> - mipsread.c \
>> namespace.c \
>> objc-lang.c \
>> objfiles.c \
>> @@ -1264,7 +1271,6 @@ SFILES = \
>> d-exp.y \
>> dtrace-probe.c \
>> elf-none-tdep.c \
>> - elfread.c \
>> f-exp.y \
>> gcore-elf.c \
>> gdb.c \
>> @@ -1886,7 +1892,6 @@ ALLDEPFILES = \
>> windows-tdep.c \
>> x86-nat.c \
>> x86-tdep.c \
>> - xcoffread.c \
>> xstormy16-tdep.c \
>> xtensa-config.c \
>> xtensa-linux-nat.c \
>> @@ -1908,7 +1913,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>> $(SUBDIR_CLI_OBS) \
>> $(SUBDIR_MI_OBS) \
>> $(SUBDIR_TARGET_OBS) \
>> - $(SUBDIR_GCC_COMPILE_OBS)
>> + $(SUBDIR_GCC_COMPILE_OBS) \
>> + $(FORMAT_OBS)
>>
>> SUBDIRS = doc @subdirs@ data-directory
>> CLEANDIRS = $(SUBDIRS)
>> 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.
>> + 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.
>> +
>> *** Changes in GDB 15
>>
>> * The MPX commands "show/set mpx bound" have been deprecated, as Intel
>> diff --git a/gdb/README b/gdb/README
>> index d85c37d5d17..342b2d07eb7 100644
>> --- 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`
>> + Configure GDB to be unable to read some binary file formats, such as
>> + coff, dbx or elf.
>> +
>> `--with-gdb-datadir=PATH'
>> Set the GDB-specific data directory. GDB will look here for
>> certain supporting files or scripts. This defaults to the `gdb'
>> diff --git a/gdb/configure b/gdb/configure
>> index 53eaad4f0e2..792e5cefefe 100755
>> --- a/gdb/configure
>> +++ b/gdb/configure
>> @@ -706,6 +706,7 @@ LIBGUI
>> LTLIBLZMA
>> LIBLZMA
>> HAVE_LIBLZMA
>> +FORMAT_OBS
>> SER_HARDWIRE
>> WERROR_CFLAGS
>> WARN_CFLAGS
>> @@ -933,6 +934,7 @@ with_relocated_sources
>> with_auto_load_dir
>> with_auto_load_safe_path
>> enable_targets
>> +enable_formats
>> enable_64_bit_bfd
>> with_amd_dbgapi
>> enable_tui
>> @@ -1644,6 +1646,9 @@ Optional Features:
>> --disable-nls do not use Native Language Support
>> --enable-targets=TARGETS
>> alternative target configurations
>> + --enable-formats=FILE_FORMATS
>> + enable support for selected file formats(default
>> + 'all')
>> --enable-64-bit-bfd 64-bit support (on hosts with narrower word sizes)
>> --enable-tui enable full-screen terminal user interface (TUI)
>> --enable-gdbtk enable gdbtk graphical user interface (GUI)
>> @@ -11499,7 +11504,7 @@ else
>> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>> lt_status=$lt_dlunknown
>> cat > conftest.$ac_ext <<_LT_EOF
>> -#line 11502 "configure"
>> +#line 11507 "configure"
>> #include "confdefs.h"
>>
>> #if HAVE_DLFCN_H
>> @@ -11605,7 +11610,7 @@ else
>> lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>> lt_status=$lt_dlunknown
>> cat > conftest.$ac_ext <<_LT_EOF
>> -#line 11608 "configure"
>> +#line 11613 "configure"
>> #include "confdefs.h"
>>
>> #if HAVE_DLFCN_H
>> @@ -24833,6 +24838,20 @@ esac
>> fi
>>
>>
>> +all_formats=
>> +# Check whether --enable-formats was given.
>> +if test "${enable_formats+set}" = set; then :
>> + enableval=$enable_formats; case "${enableval}" in
>> + yes | "") as_fn_error $? "enable-formats option must specify file formats or 'all'" "$LINENO" 5
>> + ;;
>> + no) enable_formats= ;;
>> + *) enable_formats=$enableval ;;
>> +esac
>> +else
>> + all_formats=true
>> +fi
>> +
>> +
>> # Check whether --enable-64-bit-bfd was given.
>> if test "${enable_64_bit_bfd+set}" = set; then :
>> enableval=$enable_64_bit_bfd; case $enableval in #(
>> @@ -24915,11 +24934,20 @@ fi
>> TARGET_OBS=
>> all_targets=
>> HAVE_NATIVE_GCORE_TARGET=
>> +# File formats that will ne enabled based on the selected
>> +# target(s). These are chosen because most, if not all, executables for
>> +# the target will follow this file format so it makes no sense to support
>> +# the target but not the debug information.
>> +target_formats=
>> +# If all targets were requested, this is all formats that should accompany
>> +# them.
>> +all_target_formats="elf xcoff mips coff"
>>
>> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
>> do
>> if test "$targ_alias" = "all"; then
>> all_targets=true
>> + target_formats=$all_target_formats
>> else
>> # Canonicalize the secondary target names.
>> result=`$ac_config_sub $targ_alias 2>/dev/null`
>> @@ -24941,6 +24969,19 @@ fi
>> *" ${i} "*) ;;
>> *)
>> TARGET_OBS="$TARGET_OBS ${i}"
>> + # 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
>> ;;
>> esac
>> done
>> @@ -24964,6 +25005,7 @@ if test x${all_targets} = xtrue; then
>> else
>> TARGET_OBS='$(ALL_TARGET_OBS)'
>> fi
>> + target_readers=$all_target_readers
>> fi
>>
>> # AMD debugger API support.
>> @@ -31462,6 +31504,7 @@ fi
>> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
>> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>>
>> +support_elf=no
>> # Add ELF support to GDB, but only if BFD includes ELF support.
>>
>> OLD_CFLAGS=$CFLAGS
>> @@ -31514,7 +31557,7 @@ $as_echo "$gdb_cv_var_elf" >&6; }
>> LDFLAGS=$OLD_LDFLAGS
>> LIBS=$OLD_LIBS
>> 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"
>>
>> $as_echo "#define HAVE_ELF 1" >>confdefs.h
>> @@ -31578,9 +31621,11 @@ if test "$ac_res" != no; then :
>> fi
>>
>> fi
>> + support_elf=yes
>> fi
>>
>> # Add macho support to GDB, but only if BFD includes it.
>> +support_macho=no
>>
>> OLD_CFLAGS=$CFLAGS
>> OLD_LDFLAGS=$LDFLAGS
>> @@ -31632,9 +31677,38 @@ $as_echo "$gdb_cv_var_macho" >&6; }
>> LDFLAGS=$OLD_LDFLAGS
>> LIBS=$OLD_LIBS
>> if test "$gdb_cv_var_macho" = yes; then
>> - CONFIG_OBS="$CONFIG_OBS machoread.o"
>> + support_macho=yes
>> fi
>>
>> +FORMAT_OBS=
>> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
>> +
>> +if test "$all_formats" = "true"; then
>> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
>> +else
>> + # 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"
>> + enable_formats="${enable_formats} $req"
>> + fi
>> + done
>> +
>> + for format in $enable_formats
>> + do
>> + if test "$format" = "all"; then
>> + all_formats=true
>> + fi
>> +
>> + . ${srcdir}/configure.format
>> + done
>> +fi
>> +
>> +echo $FORMAT_OBS
>> +
>> +
>> +
>> # Add any host-specific objects to GDB.
>> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>>
>> diff --git a/gdb/configure.ac b/gdb/configure.ac
>> index 8368fea0423..5f5187ecd0f 100644
>> --- a/gdb/configure.ac
>> +++ b/gdb/configure.ac
>> @@ -187,6 +187,16 @@ AS_HELP_STRING([--enable-targets=TARGETS], [alternative target configurations]),
>> *) enable_targets=$enableval ;;
>> esac])
>>
>> +all_formats=
>> +AC_ARG_ENABLE(formats,
>> + AS_HELP_STRING([--enable-formats=FILE_FORMATS], [enable support for selected file formats(default 'all')]),
>> +[case "${enableval}" in
>> + yes | "") AC_MSG_ERROR(enable-formats option must specify file formats or 'all')
>> + ;;
>> + no) enable_formats= ;;
>> + *) enable_formats=$enableval ;;
>> +esac], [all_formats=true])
>> +
>> BFD_64_BIT
>>
>> # Provide defaults for some variables set by the per-host and per-target
>> @@ -206,11 +216,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 most, if not all, executables for
>> +# the target will follow this file format so it makes no sense to support
>> +# the target but not the debug information.
>> +target_formats=
>> +# If all targets were requested, this is all formats that should accompany
>> +# them.
>> +all_target_formats="elf xcoff mips coff"
>>
>> for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
>> do
>> if test "$targ_alias" = "all"; then
>> all_targets=true
>> + target_formats=$all_target_formats
>> else
>> # Canonicalize the secondary target names.
>> result=`$ac_config_sub $targ_alias 2>/dev/null`
>> @@ -231,6 +250,19 @@ do
>> *" ${i} "*) ;;
>> *)
>> TARGET_OBS="$TARGET_OBS ${i}"
>> + # 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
>> ;;
>> esac
>> done
>> @@ -1850,11 +1882,12 @@ fi
>> # Note that WIN32APILIBS is set by GDB_AC_COMMON.
>> WIN32LIBS="$WIN32LIBS $WIN32APILIBS"
>>
>> +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"
>> AC_DEFINE(HAVE_ELF, 1,
>> [Define if ELF support should be included.])
>> @@ -1862,15 +1895,46 @@ if test "$gdb_cv_var_elf" = yes; then
>> if test "$plugins" = "yes"; then
>> AC_SEARCH_LIBS(dlopen, dl)
>> fi
>> + support_elf=yes
>> fi
>>
>> # Add macho support to GDB, but only if BFD includes it.
>> +support_macho=no
>> 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
>> fi
>>
>> +FORMAT_OBS=
>> +enable_formats=$(echo $enable_formats | sed 's/,/ /g')
>> +
>> +if test "$all_formats" = "true"; then
>> + FORMAT_OBS='$(ALL_FORMAT_OBS)'
>> +else
>> + # 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"
>> + enable_formats="${enable_formats} $req"
>> + fi
>> + done
>> +
>> + for format in $enable_formats
>> + do
>> + if test "$format" = "all"; then
>> + all_formats=true
>> + fi
>> +
>> + . ${srcdir}/configure.format
>> + done
>> +fi
>> +
>> +echo $FORMAT_OBS
>> +
>> +AC_SUBST(FORMAT_OBS)
>> +
>> # Add any host-specific objects to GDB.
>> CONFIG_OBS="${CONFIG_OBS} ${gdb_host_obs}"
>>
>> diff --git a/gdb/configure.format b/gdb/configure.format
>> new file mode 100644
>> index 00000000000..12dd2d25717
>> --- /dev/null
>> +++ b/gdb/configure.format
>> @@ -0,0 +1,41 @@
>> +# Copyright (C) 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 <http://www.gnu.org/licenses/>.
>> +
>> +# This file is used to decide which files need to be compiled to support
>> +# the requested file formats
>> +
>> +case $format in
>> + xcoff) FORMAT_OBS="$FORMAT_OBS xcoffread.o" ;;
>> +
>> + # 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" ;;
>> +
>> + dbx) FORMAT_OBS="$FORMAT_OBS dbxread.o" ;;
>> +
>> + elf) if "$support_elf"="yes"; then
>> + FORMAT_OBS="$FORMAT_OBS elfread.o"
>> + fi
>> + ;;
>> +
>> + macho) if "$support_macho"="yes"; then
>> + FORMAT_OBS="$FORMAT_OBS machoread.o"
>> + fi
>> + ;;
>> +
>> + mips) FORMAT_OBS="$FORMAT_OBS mipsread.o" ;;
>> +esac
>> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
>> index 8d85a597ec8..793793601c1 100644
>> --- a/gdb/configure.tgt
>> +++ b/gdb/configure.tgt
>> @@ -507,7 +507,7 @@ powerpc-*-openbsd*)
>> ;;
>> powerpc-*-aix* | rs6000-*-* | powerpc64-*-aix*)
>> # Target: PowerPC running AIX
>> - gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o xcoffread.o \
>> + gdb_target_obs="rs6000-tdep.o rs6000-aix-tdep.o \
>> ppc-sysv-tdep.o solib-aix.o \
>> ravenscar-thread.o ppc-ravenscar-thread.o"
>> ;;
>> @@ -523,8 +523,8 @@ powerpc*-*-linux*)
>> powerpc-*-lynx*178)
>> # Target: PowerPC running Lynx178.
>> gdb_target_obs="rs6000-tdep.o rs6000-lynx178-tdep.o \
>> - xcoffread.o ppc-sysv-tdep.o \
>> - ravenscar-thread.o ppc-ravenscar-thread.o"
>> + ppc-sysv-tdep.o ravenscar-thread.o \
>> + ppc-ravenscar-thread.o"
>> ;;
>> powerpc*-*-*)
>> # Target: PowerPC running eabi
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 77a4021b36a..c569e68060e 100644
>> --- 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
>> +Configure @value{GDBN} to support certain binary file formats. If a
>> +format is the main (or only) file format for a given target, the
>> +configure script may add support to it anyway, and warn the user.
>> +If not given, all file formats that @value{GDBN} supports are compiled.
>> +
>> @item --with-gdb-datadir=@var{path}
>> Set the @value{GDBN}-specific data directory. @value{GDBN} will look
>> here for certain supporting files or scripts. This defaults to the
>> --
>> 2.46.1
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-10 20:18 ` Guinevere Larsen
@ 2024-10-16 10:50 ` Andrew Burgess
2024-10-16 21:00 ` Guinevere Larsen
2024-10-17 19:43 ` Tom Tromey
0 siblings, 2 replies; 30+ messages in thread
From: Andrew Burgess @ 2024-10-16 10:50 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches
Guinevere Larsen <guinevere@redhat.com> writes:
> I managed to reproduce a crash on Linux with a similar setup, but my
> backtrace looks a little different. In my version, GDB is trying to
> setup solib hooks as a "post start inferior" step, whereas on yours it
> seems that GDB is trying to print the location at which the inferior is
> stopped.
>
> In both cases, though, what happens is that we're calling
> find_pc_section, which will try to create a map of sections for quick
> lookup, but since we don't understand the file format, GDB thinks we
> have no sections (and at least in the linux case, the pointer to the
> sections is null). so the issue is not that the offset is -1, but rather
> that this->the_bfd_section is 0x0.
>
> If I'm correct that this is the cause of the crash on windows as well as
> linux, I think this should fix the crash. Could you sanity check it for me?
>
>
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index 0e076fe36be..cc8bb253d11 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -1053,6 +1053,11 @@ update_section_map (struct program_space *pspace,
> gdb_assert (pspace_info->section_map_dirty != 0
> || pspace_info->new_objfiles_available != 0);
>
> + /* If there are 0 sections, or the point to the sections is null, it
> makes
> + no sense to try and have a section map at all. */
> + if (pspace_info->num_sections == 0 || pspace_info->sections == nullptr)
> + return;
> +
> map = *pmap;
> xfree (map);
>
This does fix the crash in my setup (Windows gdbserver with Linux GDB
configured to only support ELF), but I'm not sure this is the approach
that I would take.
While it is true that this change allows GDB to handle an objfile that
is can't otherwise understand, I wonder, how useful is it to even keep
the objfile around at all?
GDB realises that it can't handle the objfile in find_sym_fns, this is
where the "I'm sorry, Dave, I can't do that" message comes from. The
backtrace at this point is:
#0 find_sym_fns (abfd=0x21b4fa0) at ../../src/gdb/symfile.c:1808
#1 0x0000000000c13002 in syms_from_objfile_1 (objfile=0x21d3bf0, addrs=0x0, add_flags=...) at ../../src/gdb/symfile.c:916
#2 0x0000000000c1330c in syms_from_objfile (objfile=0x21d3bf0, addrs=0x0, add_flags=...) at ../../src/gdb/symfile.c:998
#3 0x0000000000c13838 in symbol_file_add_with_addrs (abfd=..., name=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=..., addrs=0x0, flags=..., parent=0x0) at ../../src/gdb/symfile.c:1104
#4 0x0000000000c13bd1 in symbol_file_add_from_bfd (abfd=..., name=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=..., addrs=0x0, flags=..., parent=0x0) at ../../src/gdb/symfile.c:1180
#5 0x0000000000c13c20 in symbol_file_add (name=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=..., addrs=0x0, flags=...) at ../../src/gdb/symfile.c:1193
#6 0x0000000000c13ce6 in symbol_file_add_main_1 (args=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=..., flags=..., reloff=0x0) at ../../src/gdb/symfile.c:1217
#7 0x0000000000c13c8d in symbol_file_add_main (args=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=...) at ../../src/gdb/symfile.c:1208
#8 0x000000000081cfa3 in try_open_exec_file (exec_file_host=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", inf=0x1a1e9f0, add_flags=...) at ../../src/gdb/exec.c:202
#9 0x000000000081d863 in exec_file_locate_attach (pid=42000, defer_bp_reset=0, from_tty=1) at ../../src/gdb/exec.c:344
The exception thrown in find_sym_fns is not caught until
try_open_exec_file.
The objfile itself is created in symbol_file_add_with_addrs, and
interestingly, if we checkout the end of this function we'll find this
line:
gdb::observers::new_objfile.notify (objfile);
which is going to be completely skipped if we throw an exception as we
do in this case.
I'm tempted to suggest that what we should consider is removing the
objfile if it turns out that GDB doesn't understand it. Attached at the
end of this email is a **very** rough patch which does this. After
creating the objfile we immediately wrap it in an struct, now if we
throw an exception then the objfile is auto-removed from the program
space (and deleted). Only if we reach the end of
symbol_file_add_with_addrs do we call a method on the local variable to
mark the objfile as successfully created.
I'd be tempted to convert the `if` checks you added into `gdb_assert`
calls, at least initially. I'm not sure if it's possible to have a
"valid" objfile that we understand that would otherwise trigger that
`if`, hence asserts for now.
The patch below is really **very** rough. I'm tempted to think that
objfile::make should return the object which auto-removes the objfile,
that way anyone who creates an objfile is forced to think about this
pattern of creation, validation, accepting...
Anyway, would be interested to hear your thoughts.
Thanks,
Andrew
---
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 1502fdbe500..17a7be6093b 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -854,6 +854,33 @@ init_entry_point_info (struct objfile *objfile)
}
}
+struct scoped_objfile_remover
+{
+ explicit scoped_objfile_remover (struct objfile *objfile)
+ : m_objfile (objfile)
+ {
+ /* Nothing. */
+ }
+
+ ~scoped_objfile_remover ()
+ {
+ if (m_objfile != nullptr)
+ {
+ fprintf (stderr, "APB: Removing the objfile, something went wrong\n");
+ m_objfile->unlink ();
+ }
+ }
+
+ void release_objfile ()
+ {
+ m_objfile = nullptr;
+ }
+
+private:
+
+ struct objfile *m_objfile;
+};
+
/* Process a symbol file, as either the main file or as a dynamically
loaded file.
@@ -1061,6 +1088,7 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
objfile *objfile
= objfile::make (abfd, current_program_space, name, flags, parent);
+ scoped_objfile_remover remove_objfile_on_error (objfile);
/* We either created a new mapped symbol table, mapped an existing
symbol table file which has not had initial symbol reading
@@ -1112,6 +1140,9 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
if (objfile->sf != nullptr)
finish_new_objfile (objfile, add_flags);
+ /* No errors. The objfile is officially added. */
+ remove_objfile_on_error.release_objfile ();
+
gdb::observers::new_objfile.notify (objfile);
return objfile;
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-16 10:50 ` Andrew Burgess
@ 2024-10-16 21:00 ` Guinevere Larsen
2024-10-17 19:43 ` Tom Tromey
1 sibling, 0 replies; 30+ messages in thread
From: Guinevere Larsen @ 2024-10-16 21:00 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 7681 bytes --]
On 10/16/24 7:50 AM, Andrew Burgess wrote:
> Guinevere Larsen<guinevere@redhat.com> writes:
>
>> I managed to reproduce a crash on Linux with a similar setup, but my
>> backtrace looks a little different. In my version, GDB is trying to
>> setup solib hooks as a "post start inferior" step, whereas on yours it
>> seems that GDB is trying to print the location at which the inferior is
>> stopped.
>>
>> In both cases, though, what happens is that we're calling
>> find_pc_section, which will try to create a map of sections for quick
>> lookup, but since we don't understand the file format, GDB thinks we
>> have no sections (and at least in the linux case, the pointer to the
>> sections is null). so the issue is not that the offset is -1, but rather
>> that this->the_bfd_section is 0x0.
>>
>> If I'm correct that this is the cause of the crash on windows as well as
>> linux, I think this should fix the crash. Could you sanity check it for me?
>>
>>
>> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
>> index 0e076fe36be..cc8bb253d11 100644
>> --- a/gdb/objfiles.c
>> +++ b/gdb/objfiles.c
>> @@ -1053,6 +1053,11 @@ update_section_map (struct program_space *pspace,
>> gdb_assert (pspace_info->section_map_dirty != 0
>> || pspace_info->new_objfiles_available != 0);
>>
>> + /* If there are 0 sections, or the point to the sections is null, it
>> makes
>> + no sense to try and have a section map at all. */
>> + if (pspace_info->num_sections == 0 || pspace_info->sections == nullptr)
>> + return;
>> +
>> map = *pmap;
>> xfree (map);
>>
> This does fix the crash in my setup (Windows gdbserver with Linux GDB
> configured to only support ELF), but I'm not sure this is the approach
> that I would take.
>
> While it is true that this change allows GDB to handle an objfile that
> is can't otherwise understand, I wonder, how useful is it to even keep
> the objfile around at all?
>
> GDB realises that it can't handle the objfile in find_sym_fns, this is
> where the "I'm sorry, Dave, I can't do that" message comes from. The
> backtrace at this point is:
>
> #0 find_sym_fns (abfd=0x21b4fa0) at ../../src/gdb/symfile.c:1808
> #1 0x0000000000c13002 in syms_from_objfile_1 (objfile=0x21d3bf0, addrs=0x0, add_flags=...) at ../../src/gdb/symfile.c:916
> #2 0x0000000000c1330c in syms_from_objfile (objfile=0x21d3bf0, addrs=0x0, add_flags=...) at ../../src/gdb/symfile.c:998
> #3 0x0000000000c13838 in symbol_file_add_with_addrs (abfd=..., name=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=..., addrs=0x0, flags=..., parent=0x0) at ../../src/gdb/symfile.c:1104
> #4 0x0000000000c13bd1 in symbol_file_add_from_bfd (abfd=..., name=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=..., addrs=0x0, flags=..., parent=0x0) at ../../src/gdb/symfile.c:1180
> #5 0x0000000000c13c20 in symbol_file_add (name=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=..., addrs=0x0, flags=...) at ../../src/gdb/symfile.c:1193
> #6 0x0000000000c13ce6 in symbol_file_add_main_1 (args=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=..., flags=..., reloff=0x0) at ../../src/gdb/symfile.c:1217
> #7 0x0000000000c13c8d in symbol_file_add_main (args=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", add_flags=...) at ../../src/gdb/symfile.c:1208
> #8 0x000000000081cfa3 in try_open_exec_file (exec_file_host=0x21aeab0 "target:C:/msys64/home/andrew/segv.exe", inf=0x1a1e9f0, add_flags=...) at ../../src/gdb/exec.c:202
> #9 0x000000000081d863 in exec_file_locate_attach (pid=42000, defer_bp_reset=0, from_tty=1) at ../../src/gdb/exec.c:344
>
> The exception thrown in find_sym_fns is not caught until
> try_open_exec_file.
>
> The objfile itself is created in symbol_file_add_with_addrs, and
> interestingly, if we checkout the end of this function we'll find this
> line:
>
> gdb::observers::new_objfile.notify (objfile);
>
> which is going to be completely skipped if we throw an exception as we
> do in this case.
>
> I'm tempted to suggest that what we should consider is removing the
> objfile if it turns out that GDB doesn't understand it. Attached at the
> end of this email is a **very** rough patch which does this. After
> creating the objfile we immediately wrap it in an struct, now if we
> throw an exception then the objfile is auto-removed from the program
> space (and deleted). Only if we reach the end of
> symbol_file_add_with_addrs do we call a method on the local variable to
> mark the objfile as successfully created.
This makes sense. I was a bit worried about what would happen if there
were no objfiles for a program space (since that is likely to happen if
we don't support the main objfile format), but testing with your rough
patch doesn't show any problems in a basic "read, break, run, change
memory" test session, so it should be ok enough, considering this isn't
meant to be supported.
>
> I'd be tempted to convert the `if` checks you added into `gdb_assert`
> calls, at least initially. I'm not sure if it's possible to have a
> "valid" objfile that we understand that would otherwise trigger that
> `if`, hence asserts for now.
I tested this and, it doesn't work. Adding the asserts give me hundreds
of errors in the gdb.base subfolder alone. Which must also mean that my
patch is incorrect.
I'm going to take a quick look that the errors makes sense and aren't an
unrelated bug, but I think we should just go with your solution...
>
> The patch below is really **very** rough. I'm tempted to think that
> objfile::make should return the object which auto-removes the objfile,
> that way anyone who creates an objfile is forced to think about this
> pattern of creation, validation, accepting...
I tested it, things also seem to work. I'll work a bit on the change to
objfile::make that you suggested, this looks like a good direction to go on.
--
Cheers,
Guinevere Larsen
She/Her/Hers
>
> Anyway, would be interested to hear your thoughts.
>
> Thanks,
> Andrew
>
> ---
>
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 1502fdbe500..17a7be6093b 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -854,6 +854,33 @@ init_entry_point_info (struct objfile *objfile)
> }
> }
>
> +struct scoped_objfile_remover
> +{
> + explicit scoped_objfile_remover (struct objfile *objfile)
> + : m_objfile (objfile)
> + {
> + /* Nothing. */
> + }
> +
> + ~scoped_objfile_remover ()
> + {
> + if (m_objfile != nullptr)
> + {
> + fprintf (stderr, "APB: Removing the objfile, something went wrong\n");
> + m_objfile->unlink ();
> + }
> + }
> +
> + void release_objfile ()
> + {
> + m_objfile = nullptr;
> + }
> +
> +private:
> +
> + struct objfile *m_objfile;
> +};
> +
> /* Process a symbol file, as either the main file or as a dynamically
> loaded file.
>
> @@ -1061,6 +1088,7 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
>
> objfile *objfile
> = objfile::make (abfd, current_program_space, name, flags, parent);
> + scoped_objfile_remover remove_objfile_on_error (objfile);
>
> /* We either created a new mapped symbol table, mapped an existing
> symbol table file which has not had initial symbol reading
> @@ -1112,6 +1140,9 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
> if (objfile->sf != nullptr)
> finish_new_objfile (objfile, add_flags);
>
> + /* No errors. The objfile is officially added. */
> + remove_objfile_on_error.release_objfile ();
> +
> gdb::observers::new_objfile.notify (objfile);
>
> return objfile;
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-16 10:50 ` Andrew Burgess
2024-10-16 21:00 ` Guinevere Larsen
@ 2024-10-17 19:43 ` Tom Tromey
2024-10-17 19:48 ` Guinevere Larsen
1 sibling, 1 reply; 30+ messages in thread
From: Tom Tromey @ 2024-10-17 19:43 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Guinevere Larsen, gdb-patches
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
Andrew> While it is true that this change allows GDB to handle an objfile that
Andrew> is can't otherwise understand, I wonder, how useful is it to even keep
Andrew> the objfile around at all?
IMO not very. What contribution could it really make?
Andrew> +struct scoped_objfile_remover
Andrew> +{
If it's at all possible it might be nice to clean up objfile creation
and registration. That is, it seems gross that creation registers the
objfile, as opposed to there being two steps: create and populate it,
and only then transfer ownership to the pspace.
I understand in advance if that's not practical.
Tom
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] gdb, configure: Add disable-formats option for configure
2024-10-17 19:43 ` Tom Tromey
@ 2024-10-17 19:48 ` Guinevere Larsen
0 siblings, 0 replies; 30+ messages in thread
From: Guinevere Larsen @ 2024-10-17 19:48 UTC (permalink / raw)
To: Tom Tromey, Andrew Burgess; +Cc: gdb-patches
On 10/17/24 4:43 PM, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
> Andrew> While it is true that this change allows GDB to handle an objfile that
> Andrew> is can't otherwise understand, I wonder, how useful is it to even keep
> Andrew> the objfile around at all?
>
> IMO not very. What contribution could it really make?
>
> Andrew> +struct scoped_objfile_remover
> Andrew> +{
>
> If it's at all possible it might be nice to clean up objfile creation
> and registration. That is, it seems gross that creation registers the
> objfile, as opposed to there being two steps: create and populate it,
> and only then transfer ownership to the pspace.
>
> I understand in advance if that's not practical.
>
> Tom
>
We just chatted about this today, and I decided to give it a shot,
because it also sounds better to me.
Apparently things just work after one run of the testsuite, but I'll do
a bit more due diligence before sending that.
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 30+ messages in thread