[PATCH 1/2] readelf: Fix symbol display for RELR relocs

[Szabolcs Nagy]

The 05/29/2024 10:54, Nick Clifton wrote:
> Hi Szabolcs,
> 
> > Filter symbols before binary searching for the right symbol to display
> > for a given address, such that only displayable symbols are present and
> > at most one per address.
> > 
> > The current logic does not handle multiple symbols for the same address
> > well if some of them are empty, the selected symbol is not stable with
> > respect to an unrelated symbol table change and on aarch64 often mapping
> > symbols are displayed which is not useful.
> > 
> > Filtering solves these problems at the cost of a linear scan of the
> > sorted symbol table.
> > 
> > The heuristic to select the best symbol likely could be improved, this
> > patch aims to improve symbol display for RELR without complex logic
> > such that the output is useful and stable for ld tests.
> > ---
> 
> I like the patch, but there are a couple of things that I think should be
> changed:
> 
> > +static bool
> > +is_aarch64_special_symbol_name (const char *name)
> > +{
> > +  if (!name || name[0] != '$')
> > +    return false;
> > +  if (name[1] == 'x' || name[1] == 'd')
> > +    return true;
> > +  else if (name[1] == 'm' || name[1] == 'f' || name[1] == 'p')
> > +    return true;
> > +  else
> > +    return false;
> > +  return name[2] == 0 || name[2] == '.';
> > +}
> 
> Can that last line (checking name[2]) ever be reached ?

i tried to copy the logic from bfd/cpu-aarch64.c, but i
see that i messed up: the 'return true;' should be nop ';'.


> > +  if (filedata->file_header.e_machine == EM_AARCH64)
> > +    {
> > +      /* Don't display mapping symbols.  */
> > +      if (is_aarch64_special_symbol_name (s))
> > +	return best;
> > +    }
> 
> I think that you should change this to calling a generic checking
> function which then runs architecture specific tests.  (Since there
> are other architectures which also have special symbols).  ie:
> 
>    if (is_special_symbol (filedata, s))
>      return best;

ok.

thanks for the quick review.

> 
>   [...]
> 
>   static bool
>   is_special_symbol (Filedata * filedata, Elf_Internal_Sym * s)
>   {
>      switch (filedata->file_header.e_machine)
>      {
>      case EM_AARCh64: return is_aarch64_special_symbol (s);
>   #if 0 /* Future expansion here...  */
>      case EM_MIPS: return is_mips_special_symbol (s);
>   #endif
>      default: return false;
>   }
> 
> Cheers
>   Nick
>