Re: [PATCH][gdb] Fix fbsd core matching

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

On 6/14/22 02:05, Alan Modra wrote:
> On Thu, Jun 09, 2022 at 08:59:37AM -0700, John Baldwin wrote:
>> On 6/9/22 1:58 AM, Tom de Vries via Gdb-patches wrote:
>>> Hi,
>>>
>>> With an --enable-targets=all build and target board unix/-m32 I run into a
>>> FAIL in test-case gdb.base/corefile.exp:
>>> ...
>>> (gdb) file outputs/gdb.base/corefile/corefile^M
>>> Reading symbols from outputs/gdb.base/corefile/corefile...^M
>>> (gdb) core-file outputs/gdb.base/corefile/corefile.core^M
>>> warning: core file may not match specified executable file.^M
>>> [New LWP 12011]^M
>>> Core was generated by `outputs/gdb.base/corefile/co'.^M
>>> Program terminated with signal SIGABRT, Aborted.^M
>>> (gdb) FAIL: gdb.base/corefile.exp: core-file warning-free
>>> ...
>>>
>>> The warning is there because of this mismatch between core and exec:
>>> ...
>>> (gdb) p core_bfd->xvec
>>> $3 = (const struct bfd_target *) 0x20112a0 <i386_elf32_fbsd_vec>
>>> (gdb) p exec_bfd->xvec
>>> $4 = (const struct bfd_target *) 0x2010b00 <i386_elf32_vec>
>>> ...
>>>
>>> In the exec case, the detected architecture is i386_elf32_vec because this bit
>>> of code in elfcode.h:elf_object_p():
>>> ...
>>>     if (ebd->elf_machine_code != EM_NONE
>>>         && i_ehdrp->e_ident[EI_OSABI] != ebd->elf_osabi
>>>         && ebd->elf_osabi != ELFOSABI_NONE)
>>>       goto got_wrong_format_error;
>>> ...
>>> prevents i386_elf32_fbsd from matching.
>>>
>>> Fix the core matching by copying that code to elfcore.h:elf_core_file_p().
>>>
>>> Tested on x86_64-linux.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29227
>>>
>>> Any comments?
> 
> Looks good.
> 
>> Looking at elfcore.h, it seems to have not gotten changes made to elfcode.h over
>> time and is a bit rotted.  I suspect that all of changes made in commit 0aabe54e6222
>> that added these lines in elfcode.h (along with several other changes) need to
>> be applied to this function in elfcore.h, not just adding these lines.
> 
> Yes, the commit 0aabe54e6222 changes likely should go in too.  I'm a
> little wary of adding all the sanity checks to elf_core_file_p since
> that might result in some core files not being recognised at all.  For
> example, despite the FIXME I'd guess leaving out the EI_VERSION check
> was deliberate.  The following seems reasonable to me.  Please test.
> 

I've run the gdb testsuite on x86_64-linux with native and target board 
unix-/m32, no regressions and originally reported FAIL fixed.

Thanks,
- Tom

> diff --git a/bfd/elfcore.h b/bfd/elfcore.h
> index 809f6711aed..59b73bd57de 100644
> --- a/bfd/elfcore.h
> +++ b/bfd/elfcore.h
> @@ -149,37 +149,14 @@ elf_core_file_p (bfd *abfd)
>         && (ebd->elf_machine_alt1 == 0
>   	  || i_ehdrp->e_machine != ebd->elf_machine_alt1)
>         && (ebd->elf_machine_alt2 == 0
> -	  || i_ehdrp->e_machine != ebd->elf_machine_alt2))
> -    {
> -      const bfd_target * const *target_ptr;
> -
> -      if (ebd->elf_machine_code != EM_NONE)
> -	goto wrong;
> -
> -      /* This is the generic ELF target.  Let it match any ELF target
> -	 for which we do not have a specific backend.  */
> +	  || i_ehdrp->e_machine != ebd->elf_machine_alt2)
> +      && ebd->elf_machine_code != EM_NONE)
> +    goto wrong;
>   
> -      for (target_ptr = bfd_target_vector; *target_ptr != NULL; target_ptr++)
> -	{
> -	  const struct elf_backend_data *back;
> -
> -	  if ((*target_ptr)->flavour != bfd_target_elf_flavour)
> -	    continue;
> -	  back = xvec_get_elf_backend_data (*target_ptr);
> -	  if (back->s->arch_size != ARCH_SIZE)
> -	    continue;
> -	  if (back->elf_machine_code == i_ehdrp->e_machine
> -	      || (back->elf_machine_alt1 != 0
> -		  && i_ehdrp->e_machine == back->elf_machine_alt1)
> -	      || (back->elf_machine_alt2 != 0
> -		  && i_ehdrp->e_machine == back->elf_machine_alt2))
> -	    {
> -	      /* target_ptr is an ELF backend which matches this
> -		 object file, so reject the generic ELF target.  */
> -	      goto wrong;
> -	    }
> -	}
> -    }
> +  if (ebd->elf_machine_code != EM_NONE
> +      && i_ehdrp->e_ident[EI_OSABI] != ebd->elf_osabi
> +      && ebd->elf_osabi != ELFOSABI_NONE)
> +    goto wrong;
>   
>     /* If there is no program header, or the type is not a core file, then
>        we are hosed.  */
> @@ -199,6 +176,9 @@ elf_core_file_p (bfd *abfd)
>         Elf_Internal_Shdr i_shdr;
>         file_ptr where = (file_ptr) i_ehdrp->e_shoff;
>   
> +      if (i_ehdrp->e_shoff < sizeof (x_ehdr))
> +	goto wrong;
> +
>         /* Seek to the section header table in the file.  */
>         if (bfd_seek (abfd, where, SEEK_SET) != 0)
>   	goto fail;
> 
>> I've cc'd Alan who made that commit above to elfcore.h.  There also seem to be
>> some other inconsistencies between the functions in elfcore.h and and elfcode.h
>> and perhaps the common functionality in these functions could be refactored
>> into a separate function to avoid the code duplication?
>>
>> Note that the issue isn't really FreeBSD specific, I just suspect that the FreeBSD
>> i386 vec happens to be sorted first in the list for some reason.
>>> Thanks,
>>> - Tom
>>>
>>> [gdb] Fix fbsd core matching
>>>
>>> ---
>>>    bfd/elfcore.h | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/bfd/elfcore.h b/bfd/elfcore.h
>>> index 809f6711aed..4ce81e2e383 100644
>>> --- a/bfd/elfcore.h
>>> +++ b/bfd/elfcore.h
>>> @@ -272,6 +272,11 @@ elf_core_file_p (bfd *abfd)
>>>          && ebd->elf_machine_code != EM_NONE)
>>>        goto fail;
>>> +  if (ebd->elf_machine_code != EM_NONE
>>> +      && i_ehdrp->e_ident[EI_OSABI] != ebd->elf_osabi
>>> +      && ebd->elf_osabi != ELFOSABI_NONE)
>>> +    goto fail;
>>> +
>>>      /* Let the backend double check the format and override global
>>>         information.  We do this before processing the program headers
>>>         to allow the correct machine (as opposed to just the default
>>
>>
>> -- 
>> John Baldwin
>