[PATCH] bfd: fix missing warnings from bfd_check_format_matches

[Alan Modra]

On Sun, Apr 06, 2025 at 11:12:16PM +0100, Andrew Burgess wrote:
> In PR gdb/31846 the user reported an issue where GDB is unable to find
> the build-id within an ELF, despite the build-id being
> present (confirmed using readelf).
> 
> The user was able to try several different builds of GDB, and in one
> build they observed the warning:
> 
>   warning: BFD: FILENAME: unable to decompress section .debug_info
> 
> But in may other builds of GDB this warning was missing.
> 
> There are, I think, a couple of issues that the user is running into,
> but this patch is about why the above warning is often missing from
> GDB's output.
> 
> I wasn't able to reproduce a corrupted .debug_info section such that
> the above warning would be triggered, but it is pretty easy to patch
> the _bfd_elf_make_section_from_shdr function (in bfd/elf.c) such that
> the call to bfd_init_section_decompress_status is reported as a
> failure, thus triggering the warning.  There is a patch that achieves
> this in the bug report.
> 
> I did this, and can confirm that on my build of GDB, I don't see the
> above warning, even though I can confirm that the _bfd_error_handler
> call (in _bfd_elf_make_section_from_shdr) is being reached.
> 
> The problem is back in format.c, in bfd_check_format_matches.  This
> function intercepts all the warnings and places them into a
> per_xvec_messages structure.  These warnings are then printed with a
> call to print_and_clear_messages.
> 
> If bfd_check_format_matches finds a single matching format, then
> print_and_clear_messages, will print all warnings associated with that
> single format.
> 
> But if no format matches, print_and_clear_messages will print all the
> warnings, so long as all targets have emitted the same set of
> warnings, and unfortunately, that is not the case for me.
> 
> The warnings are collected by iterating over bfd_target_vector and
> trying each target.  My target happens to be x86_64_elf64_vec, and, as
> expected this target appears in bfd_target_vector.
> 
> However, bfd_target_vector also includes DEFAULT_VECTOR near the top.
> And in my build, DEFAULT_VECTOR is x86_64_elf64_vec.  Thus, for me,
> the x86_64_elf64_vec entry appears twice in bfd_target_vector, this
> means that x86_64_elf64_vec ends up being tried twice, and, as each
> try generates one warning, the x86_64_elf64_vec entry in the
> per_xvec_messages structure, has two warnings, while the other
> per_xvec_messages entries only have one copy of the warning.
> 
> Because of this difference, print_and_clear_messages decides not to
> print any of the warnings, which is not very helpful.
> 
> I considered a few different approaches to fix this issue:
> 
> We could de-duplicate warnings in the per_xvec_messages structure as
> new entries are added.  So for any particular xvec, each time a new
> warning arrives, if the new warning is identical to an existing
> warning, then don't record it.  This might be an interesting change in
> the future, but for now I rejected this solution as it felt like a
> bodge, the duplicate warnings aren't really from a single attempt at
> an xvec, but are from two distinct attempts at the same xvec. And so:
> 
> I wondered if we could remove the duplicate entries from
> bfd_target_vector.  Or if we could avoid processing the same xvec
> twice maybe?  For the single DEFAULT_VECTOR this wouldn't be too hard
> to do, but bfd_target_vector also includes SELECT_VECS, which I think
> could contain more duplicates.  Changing bfd_check_format_matches to

Yes, SELECT_VECS itself won't contain duplicates due to configure.ac
making selvecs unique, but I think someone could configure with say,
"srec" in --enable-targets which would result in a duplicate.

> avoid attempting any duplicate vectors would now require more
> complexity than a single flag, and I felt there was an easier
> solution, which was:

I'm happy with your solution, and the patch.  To avoid checking
duplicate vectors we'd need to go back to an array effectively
extending _bfd_target_vector with some read/write fields, and use that
for the per-xvec messages plus a flag to say whether that particular
target had already been checked.

> 
> I propose that within bfd_check_format_matches, within the loop that
> tries each entry from bfd_target_vector, as we switch to each vector
> in turn, we should delete any existing warnings within the
> per_xvec_messages structure for the target vector we are about to try.
> 
> This means that, if we repeat a target, only the last set of warnings
> will survive.
> 
> With this change in place, print_and_clear_messages now sees the same
> set of warnings for each target, and so prints out the warning
> message.
> 
> Additionally, while I was investigating this issue I managed to call
> print_and_clear_messages twice.  This caused a crash because the first
> call to print_and_clear_messages frees all the associated memory, but
> leaves the per_xvec_messages::next field pointing to the now
> deallocated object.  I'd like to propose that we set the next field to
> NULL in print_and_clear_messages.  This clearly isn't needed so long
> as print_and_clear_messages is only called once, but (personally) I
> like to set pointers back to NULL if the object they are pointing to
> is free and the parent object is going to live for some additional
> time.  I can drop this extra change if people don't like it.
> 
> This change doesn't really "fix" PR gdb/31846, but it does mean that
> the warning about being unable to decompress .debug_info should now be
> printed consistently, which is a good thing.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31846
> ---
>  bfd/format.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/bfd/format.c b/bfd/format.c
> index 7769ad095f6..3db0792d412 100644
> --- a/bfd/format.c
> +++ b/bfd/format.c
> @@ -331,6 +331,25 @@ print_and_clear_messages (struct per_xvec_messages *list,
>  	free (iter);
>        iter = next;
>      }
> +
> +  /* Don't retain a pointer to free'd memory.  */
> +  list->next = NULL;
> +}
> +
> +/* Discard all messages associated with TARG in LIST.  Unlike
> +   print_and_clear_messages, PER_XVEC_NO_TARGET is not valid for TARG.  */
> +
> +static void
> +clear_messages (struct per_xvec_messages *list,
> +		const bfd_target *targ)
> +{
> +  struct per_xvec_messages *iter;
> +
> +  for (iter = list; iter != NULL; iter = iter->next)
> +    {
> +      if (iter->targ == targ)
> +	clear_warnmsg (&iter->messages);
> +    }
>  }
>  
>  /* This a copy of lto_section defined in GCC (lto-streamer.h).  */
> @@ -545,6 +564,12 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
>        /* Change BFD's target temporarily.  */
>        abfd->xvec = *target;
>  
> +      /* It is possible that targets appear multiple times in
> +	 bfd_target_vector.  If this is the case, then we want to avoid
> +	 accumulating duplicate messages for a target in MESSAGES, so
> +	 discard any previous messages associated with this target.  */
> +      clear_messages (&messages, abfd->xvec);
> +
>        if (bfd_seek (abfd, 0, SEEK_SET) != 0)
>  	goto err_ret;
>  
> 
> base-commit: c2f55040d34784a5c40d25f6a58615da1b4a52be
> -- 
> 2.47.1

-- 
Alan Modra