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

[Guinevere Larsen]

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