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

[Eli Zaretskii <eliz@gnu.org>]

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