Re: [RFA] Make sym_read routines handle separate debug files

[Tristan Gingold <gingold at adacore dot com>]

On Dec 4, 2009, at 7:01 PM, Tom Tromey wrote:

>>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:
> 
> Tristan> Each object file reader is now responsible to read separate
> Tristan> debug files as they are object file dependant.  Of course, in
> Tristan> the case of gnu .debuglink a common subprogram is used.
> 
> You updated coffread and elfread, but the existing code in gdb does not
> depend on the format.  So it seems to me that any other code defining
> sym_fns will regress.  That is, dbxread, mipsread, somread, and
> xcoffread (I omitted machoread since I presume your later patch fixes
> that up).

dbxread is for a.out, which doesn't support .gnu_debuglink because it only has 2 sections (.text and .data).
somread is for PA-HP/UX which doesn't support .gnu_debuglink because it doesn't fit in space/subspace
scheme.
xcoffread is for AIX and xcoff section names are limited to 8 bytes.  So .gnu_debuglink will never exist
on this platform.
mipsread is for ecoff used by mips and alpha.  Their coff sections name are limited to 8 bytes too.

machoread bfd backend doesn't know about .gnu_debuglink so this feature doesn't work with Mach-O.  This is
useless because Mach-O has dsymfiles which are roughly equivalent to .gnu_debuglink.

> Could we not just keep the existing logic as a format-independent
> fallback?

I was hesitant to do that given that only ELF and some COFF support .gnu_debuglink.

> Or am I mistaken about something?  Maybe it is impossible for those
> formats to have separate debug files?  I know nothing of the details,
> I'm afraid.

Sorry, that's my fault.  I should have explained why I changed only ELF and COFF.
(I also forgot to say that there were no regression on x86 GNU/Linux).

> Tristan> +      debugfile = find_separate_debug_file_by_buildid (objfile);
> 
> If this is only meaningful for ELF, as it seems to be, then it seems we
> might as well put it in elfread.c.

To be honest, I reserved this move for a following patch.  Do you prefer I resubmit this patch with this
change ?
(I'd prefer to make a separate patch to keep this one smaller, even if it already contains some
re-indentation).

> Tristan> +/* Add BFD as a separate debug file for OBJFILE.  */
> Tristan> +
> Tristan> +void
> Tristan> +symbol_file_add_separate (bfd *bfd, int symfile_flags, struct objfile *objfile)
> Tristan> +{
> Tristan> +  objfile->separate_debug_objfile =
> 
> I'm wondering if this should have a sanity check and call internal_error
> if the separate debug objfile is already set.

Fine.  Will do this change.

> Tristan> @@ -2320,204 +2314,217 @@ reread_symbols (void)
> [...]
> Tristan> +	  /* The "mainline" parameter is a hideous hack; I think leaving it
> Tristan> +	     zero is OK since dbxread.c also does what it needs to do if
> Tristan> +	     objfile->global_psymbols.size is 0.  */
> 
> Normally I wouldn't ask for changes in a simple reindentation (and there
> are other stale comments here that I didn't single out), but this
> particular comment seems relevant to the overall change.  I suggest just
> removing it, but rewording would be ok too.

Thank for this catch.  I think we could remove it.

Again thank you for the whole review,
Tristan.