Re: [PATCH 2/5] readelf: Adapt src/readelf -h/-S/-r/-w/-l/-d/-a on mips

["Ying Huang" <ying.huang@oss.cipunited.com>]

[-- Attachment #1: Type: text/plain, Size: 11967 bytes --]

Hi,

在 2023/5/11 22:31, Mark Wielaard 写道:
> Hi,
>
> On Tue, 2023-04-11 at 16:12 +0800, Ying Huang wrote:
>> diff --git a/backends/mips_attrs.c b/backends/mips_attrs.c
>> new file mode 100644
>> index 00000000..1419814e
>> --- /dev/null
>> +++ b/backends/mips_attrs.c
>> @@ -0,0 +1,107 @@
>> +/* Object attribute tags for MIPS.
>> +   Copyright (C) 2023 CIP United Inc.
>> +   This file is part of elfutils.
>> +
>> +   This file is free software; you can redistribute it and/or modify
>> +   it under the terms of either
>> +
>> +     * the GNU Lesser General Public License as published by the Free
>> +       Software Foundation; either version 3 of the License, or (at
>> +       your option) any later version
>> +
>> +   or
>> +
>> +     * the GNU General Public License as published by the Free
>> +       Software Foundation; either version 2 of the License, or (at
>> +       your option) any later version
>> +
>> +   or both in parallel, as here.
>> +
>> +   elfutils is distributed in the hope that it will be useful, but
>> +   WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   General Public License for more details.
>> +
>> +   You should have received copies of the GNU General Public License and
>> +   the GNU Lesser General Public License along with this program.  If
>> +   not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#ifdef HAVE_CONFIG_H
>> +# include <config.h>
>> +#endif
>> +
>> +#include <string.h>
>> +#include <dwarf.h>
>> +
>> +#define BACKEND arm_
> You mean mips_

    Yes, it should be mips_, thanks!


>> diff --git a/backends/mips_init.c b/backends/mips_init.c
>> index f70d62e4..5bba822b 100644
>> --- a/backends/mips_init.c
>> +++ b/backends/mips_init.c
>> @@ -45,5 +45,12 @@ mips_init (Elf *elf __attribute__ ((unused)),
>>    /* We handle it.  */
>>    mips_init_reloc (eh);
>>    HOOK (eh, reloc_simple_type);
>> +  HOOK (eh, section_type_name);
>> +  HOOK (eh, machine_flag_check);
>> +  HOOK (eh, machine_flag_name);
>> +  HOOK (eh, segment_type_name);
>> +  HOOK (eh, dynamic_tag_check);
>> +  HOOK (eh, dynamic_tag_name);
>> +  HOOK (eh, check_object_attribute);
>>    return eh;
>>  }
> OK
> But see below for also hooking reloc_type_check and reloc_type_name.


    OK, I would add hook for reloc_type_check and reloc_type_name.


>>  
>>  typedef union
>> @@ -2218,8 +2302,11 @@ enum
>>    Val_GNU_MIPS_ABI_FP_64 = 6,
>>    /* Using -mips32r2 -mfp64 -mno-odd-spreg.  */
>>    Val_GNU_MIPS_ABI_FP_64A = 7,
>> +  /* This is reserved for backward-compatibility with an earlier
>> +     implementation of the MIPS NaN2008 functionality.  */
>> +  Val_GNU_MIPS_ABI_FP_NAN2008 = 8,
>>    /* Maximum allocated FP ABI value.  */
>> -  Val_GNU_MIPS_ABI_FP_MAX = 7
>> +  Val_GNU_MIPS_ABI_FP_MAX = 9
>>  };
>>  
>>  /* HPPA specific definitions.  */
> We take elf.h from glibc so please suggest these additions first on
> libc-alpha@sourceware.org

    I have submitted patch to libc-alpha@sourceware.org and modify the  Val_GNU_MIPS_ABI_FP_MAX to 8.

  https://sourceware.org/pipermail/libc-alpha/2023-May/148112.html

>
>> diff --git a/src/readelf.c b/src/readelf.c
>> index 6950204e..6e9a02c1 100644
>> --- a/src/readelf.c
>> +++ b/src/readelf.c
>> @@ -1125,7 +1125,7 @@ print_ehdr (Ebl *ebl, GElf_Ehdr *ehdr)
>>  	  ehdr->e_ident[EI_VERSION] == EV_CURRENT ? _("(current)")
>>  	  : "(\?\?\?)");
>>  
>> -  char buf[512];
>> +  char buf[64];
>>    printf (_("  OS/ABI:                            %s\n"),
>>  	  ebl_osabi_name (ebl, ehdr->e_ident[EI_OSABI], buf, sizeof (buf)));
>>
> Can you explain why reducing this buffer to 64 is OK?

    Because the OS/ABI and Flags are not as long as 256, maybe I did not think enough, can not it be reduced?


>
>> @@ -2193,17 +2193,41 @@ handle_relocs_rel (Ebl *ebl, GElf_Ehdr *ehdr, Elf_Scn *scn, GElf_Shdr *shdr)
>>  			(long int) GELF_R_SYM (rel->r_info));
>>  	    }
>>  	  else if (GELF_ST_TYPE (sym->st_info) != STT_SECTION)
>> -	    printf ("  %#0*" PRIx64 "  %-20s %#0*" PRIx64 "  %s\n",
>> -		    class == ELFCLASS32 ? 10 : 18, rel->r_offset,
>> -		    likely (ebl_reloc_type_check (ebl,
>> -						  GELF_R_TYPE (rel->r_info)))
>> -		    /* Avoid the leading R_ which isn't carrying any
>> -		       information.  */
>> -		    ? ebl_reloc_type_name (ebl, GELF_R_TYPE (rel->r_info),
>> -					   buf, sizeof (buf)) + 2
>> -		    : _("<INVALID RELOC>"),
>> -		    class == ELFCLASS32 ? 10 : 18, sym->st_value,
>> -		    elf_strptr (ebl->elf, symshdr->sh_link, sym->st_name));
>> +	    {
>> +	      unsigned long inf = rel->r_info;
>> +	      printf ("  %#0*" PRIx64 "  %-20s %#0*" PRIx64 "  %s\n",
>> +		      class == ELFCLASS32 ? 10 : 18, rel->r_offset,
>> +		      likely (ebl_reloc_type_check (ebl,
>> +						    GELF_R_TYPE (rel->r_info)))
>> +		      /* Avoid the leading R_ which isn't carrying any
>> +		         information.  */
>> +		      ? ebl_reloc_type_name (ebl, GELF_R_TYPE (rel->r_info),
>> +					     buf, sizeof (buf)) + 2
>> +		      : _("<INVALID RELOC>"),
>> +		      class == ELFCLASS32 ? 10 : 18, sym->st_value,
>> +		      elf_strptr (ebl->elf, symshdr->sh_link, sym->st_name));
>> +
>> +	      /* copy binutils-2.34/binutils/readelf.c dump_relocations+1753  */
>> +              if(ebl->elf->class == ELFCLASS64 && ebl->elf->state.elf64.ehdr->e_machine == EM_MIPS)
>> +	        {
>> +		  unsigned int type2 = ELF64_MIPS_R_TYPE2 (inf);
>> +		  unsigned int type3 = ELF64_MIPS_R_TYPE3 (inf);
>> +		  const char * rtype2 = ebl_reloc_type_name (ebl, type2, buf, sizeof (buf)) + 2;
>> +		  const char * rtype3 = ebl_reloc_type_name (ebl, type3, buf, sizeof (buf)) + 2;
>> +		  printf("                      Type2: ");
>> +		  if (rtype2 == NULL)
>> +	            printf (_("unrecognized: %-7lx"), (unsigned long) type2 & 0xffffffff);
>> +	          else
>> +	            printf ("%-17.17s", rtype2);
>> +
>> +	          printf ("\n                      Type3: ");
>> +		  if (rtype3 == NULL)
>> +	            printf (_("unrecognized: %-7lx"), (unsigned long) type3 & 0xffffffff);
>> +	          else
>> +	            printf ("%-17.17s", rtype3);
>> +		  printf("\n");
>> +	        }
>> +	    }
> I think this is OK, but it would be good to see an example.

  

    Relocation section [ 2] '.rela.text' for section [ 1] '.text' at offset 0x29908 contains 1979 entries:
  Offset              Type            Value               Addend Name
  0x0000000000000008  MIPS_GPREL16    000000000000000000      +0 .text
                      Type2: MIPS_SUB         
                      Type3: MIPS_HI16        
  0x0000000000000010  MIPS_GPREL16    000000000000000000      +0 .text
                      Type2: MIPS_SUB         
                      Type3: MIPS_LO16        
  0x0000000000000014  MIPS_CALL16     000000000000000000      +0 gelf_getehdr
                      Type2: MIPS_NONE        
                      Type3: MIPS_NONE        
  0x0000000000000034  MIPS_JALR       000000000000000000      +0 gelf_getehdr
                      Type2: MIPS_NONE        
                      Type3: MIPS_NONE        
  0x000000000000004c  MIPS_CALL16     000000000000000000      +0 elf_getscn
                      Type2: MIPS_NONE        
                      Type3: MIPS_NONE        
  0x0000000000000054  MIPS_CALL16     000000000000000000      +0 gelf_update_ehdr
                      Type2: MIPS_NONE        
                      Type3: MIPS_NONE

   

>>  	  else
>>  	    {
>>  	      /* This is a relocation against a STT_SECTION symbol.  */
>> @@ -2420,18 +2492,42 @@ handle_relocs_rela (Ebl *ebl, GElf_Ehdr *ehdr, Elf_Scn *scn, GElf_Shdr *shdr)
>>  			(long int) (sym->st_shndx == SHN_XINDEX
>>  				    ? xndx : sym->st_shndx));
>>  	      else
>> -		printf ("\
>> +	        {
>> +                  unsigned long inf = rel->r_info;
>> +		  printf ("\
>>    %#0*" PRIx64 "  %-15s %#0*" PRIx64 "  %+6" PRId64 " %s\n",
>> -			class == ELFCLASS32 ? 10 : 18, rel->r_offset,
>> -			ebl_reloc_type_check (ebl, GELF_R_TYPE (rel->r_info))
>> -			/* Avoid the leading R_ which isn't carrying any
>> -			   information.  */
>> -			? ebl_reloc_type_name (ebl, GELF_R_TYPE (rel->r_info),
>> -					       buf, sizeof (buf)) + 2
>> -			: _("<INVALID RELOC>"),
>> -			class == ELFCLASS32 ? 10 : 18, sym->st_value,
>> -			rel->r_addend,
>> -			elf_strptr (ebl->elf, shstrndx, secshdr->sh_name));
>> +			  class == ELFCLASS32 ? 10 : 18, rel->r_offset,
>> +			  ebl_reloc_type_check (ebl, GELF_R_TYPE (rel->r_info))
>> +			  /* Avoid the leading R_ which isn't carrying any
>> +			     information.  */
>> +			  ? ebl_reloc_type_name (ebl, GELF_R_TYPE (rel->r_info),
>> +					         buf, sizeof (buf)) + 2
>> +			  : _("<INVALID RELOC>"),
>> +			  class == ELFCLASS32 ? 10 : 18, sym->st_value,
>> +			  rel->r_addend,
>> +			  elf_strptr (ebl->elf, shstrndx, secshdr->sh_name));
>> +
>> +                  /* copy binutils-2.34/binutils/readelf.c dump_relocations+1753  */
>> +		  if(ebl->elf->class == ELFCLASS64 && ebl->elf->state.elf64.ehdr->e_machine == EM_MIPS)
>> +		    {
>> +		      unsigned int type2 = ELF64_MIPS_R_TYPE2 (inf);
>> +		      unsigned int type3 = ELF64_MIPS_R_TYPE3 (inf);
>> +		      const char * rtype2 = ebl_reloc_type_name (ebl, type2, buf, sizeof (buf)) + 2;
>> +		      const char * rtype3 = ebl_reloc_type_name (ebl, type3, buf, sizeof (buf)) + 2;
>> +		      printf("                      Type2: ");
>> +		      if (rtype2 == NULL)
>> +	                printf (_("unrecognized: %-7lx"), (unsigned long) type2 & 0xffffffff);
>> +	              else
>> +	                printf ("%-17.17s", rtype2);
>> +
>> +	              printf ("\n                      Type3: ");
>> +		      if (rtype3 == NULL)
>> +	                printf (_("unrecognized: %-7lx"), (unsigned long) type3 & 0xffffffff);
>> +	              else
>> +	                printf ("%-17.17s", rtype3);
>> +		      printf("\n");
>> +		    }
>> +	        }
>>  	    }
>>  	}
>>      }
> And again. Adding testcase would be nice.
>
> So this handles both REL and RELA, does mips support both?


    OK, I would like to add testcase.

    Both support.


>
>> @@ -11633,7 +11729,7 @@ print_debug (Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr)
>>  	  GElf_Shdr shdr_mem;
>>  	  GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
>>  
>> -	  if (shdr != NULL && shdr->sh_type == SHT_PROGBITS)
>> +	  if (shdr != NULL && (shdr->sh_type == SHT_PROGBITS || shdr->sh_type == SHT_MIPS_DWARF))
>>  	    {
>>  	      const char *name = elf_strptr (ebl->elf, shstrndx,
>>  					     shdr->sh_name);
>> @@ -11663,7 +11759,7 @@ print_debug (Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr)
>>        GElf_Shdr shdr_mem;
>>        GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
>>  
>> -      if (shdr != NULL && shdr->sh_type == SHT_PROGBITS)
>> +      if (shdr != NULL && (shdr->sh_type == SHT_PROGBITS || shdr->sh_type == SHT_MIPS_DWARF))
>>  	{
>>  	  static const struct
>>  	  {
> OK. There are probably other places in the code where this might be an
> issue. Maybe we need an is_debug_section_type macro?
>
> Cheers,
>
> Mark

    Yes, I think it is good to add a new macro && need to check carefully.


Thanks,

Ying