Re: [PATCH V2 3/3] bfd: fix the deletion of relocs in mips64

[jose dot marchesi at oracle dot com (Jose E. Marchesi)]

    > This patch fixes the deletion of relocations in BFD sections in mips64
    > targets.
    > 
    > A new field `reloc_count' is added to the `_bfd_mips_elf_section_data'
    > in order to hold the number of internal relocations that the section
    > contains.  A specialized `_bfd_set_reloc' function is provided that
    > updates this internal field, and the logic in the relocation-related
    > functions in elf64-mips.c is adapted to use this new field.
    
     Offhand, have you investigated reusing RELOC_AGAINST_DISCARDED_SECTION 
    infrastructure here for the deletion of discarded relocations?
    
     It looks to me like the infrastructure could be used with little effort, 
    e.g. `info' is only needed for `->relocatable', so the flag could be 
    passed by itself rather than the whole link info structure.  The MIPS n64 
    case is already handled by `mips_reloc_against_discarded_section', which 
    could be one handler, beside a generic one, and a SPARC64 one (which would 
    have to be added), exported as a BFD method.
    
     I might be missing a detail here or there, so any actual implementation 
    might come out a tad more complicated, but my high-level conclusion is I 
    don't really like the duplication of the same mechanism across different 
    pieces of code; being easy to miss this is really hard to maintain 
    long-term, as this case has also shown.

     I've looked through your change as it is, on the basis that Alan has 
    already approved the other parts, so please do not consider my observation 
    above a request to you for further work unless you really want to look 
    into it.  An imperfect solution that works is certainly better short-term 
    than an ideal one that yet has to be written by someone.

Nope, I haven't looked at RELOC_AGAINST_DISCARDED_SECTION.  How would it
be used to make bfd_canonicalize_reloc -> bfd_set_reloc ->
bfd_canonicalize_reloc sequences to work?

     That written I have a couple of concerns I do want you to address with 
    your current proposal, see below for details.
    
    > This patch also removes the safety check recently introduced in
    > objcopy, to avoid deleting relocations if the count returned by
    > `bfd_canonicalize_relocs' is bigger than `sec->reloc_count', as it is
    > no longer necessary.
    
     I think this needs to be a separate followup commit, as it's not strictly 
    MIPS-specific, and both changes will then be self-contained.

Ok.

    > diff --git a/bfd/elf64-mips.c b/bfd/elf64-mips.c
    > index a66c319..63880c6 100644
    > --- a/bfd/elf64-mips.c
    > +++ b/bfd/elf64-mips.c
    > @@ -3728,6 +3730,22 @@ mips_elf64_canonicalize_dynamic_reloc (bfd *abfd, arelent **storage,
    >    return ret;
    >  }
    >  
    > +/* A similar problem happens with set_reloc.  This version updates the
    > +   internal `reloc_count' field in `struct _mips_elf_section_data' in
    > +   order to hold the new number of internal relocations.  This avoids
    > +   overwriting `asect->reloc_count' that holds the number of external
    > +   relocations.  */
    
     What does this comment refer to, i.e. what is the problem similar to?

To the comment two functions before, for
mips_elf64_canonicalize[_dynamic]_reloc:

/* We must also copy more relocations than the corresponding functions
   in elf.c would, so the two following functions are slightly
   modified from elf.c, that multiply the external relocation count by
   3 to obtain the internal relocation count.  */

    > +
    > +static void
    > +mips_elf64_set_reloc (bfd *abfd ATTRIBUTE_UNUSED,
    > +                      asection *asect,
    > +                      arelent **location,
    > +                      unsigned int count)
    > +{
    > +  asect->orelocation = location;
    > +  canon_reloc_count (asect) = count;
    > +}
    
     And why do you need to keep track of the number of internal relocations 
    separately, given that the mapping between the internal and the external 
    count is fixed at all times.  Is it not enough to:
    
      BFD_ASSERT (count % 3 == 0);
      asect->reloc_count = count / 3;
    
    ?

That was indeed my first approach :) But GAS seems to be setting mips64
relocations one by one (for whatever reason) using bfd_set_reloc so the
assert is triggered in the GAS tests.

    > diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
    > index 70c7f1c..8aa979e 100644
    > --- a/bfd/elfxx-mips.c
    > +++ b/bfd/elfxx-mips.c
    > @@ -221,18 +221,6 @@ struct mips_elf_traverse_got_arg
    >    int value;
    >  };
    >  
    > -struct _mips_elf_section_data
    > -{
    > -  struct bfd_elf_section_data elf;
    > -  union
    > -  {
    > -    bfd_byte *tdata;
    > -  } u;
    > -};
    > -
    > -#define mips_elf_section_data(sec) \
    > -  ((struct _mips_elf_section_data *) elf_section_data (sec))
    > -
    >  #define is_mips_elf(bfd)				\
    >    (bfd_get_flavour (bfd) == bfd_target_elf_flavour	\
    >     && elf_tdata (bfd) != NULL				\
    > diff --git a/bfd/elfxx-mips.h b/bfd/elfxx-mips.h
    > index 44ad789..5710c61 100644
    > --- a/bfd/elfxx-mips.h
    > +++ b/bfd/elfxx-mips.h
    > @@ -22,6 +22,22 @@
    >  #include "elf/internal.h"
    >  #include "elf/mips.h"
    >  
    > +struct _mips_elf_section_data
    > +{
    > +  struct bfd_elf_section_data elf;
    > +  union
    > +  {
    > +    bfd_byte *tdata;
    > +  } u;
    > +  unsigned int reloc_count;
    
     Add an explanatory comment for a newly-added structure member and give it 
    a more meaningful name, such as `internal_reloc_count' (assuming that it 
    is needed indeed in the first place; see above).

Ok.
    
    > +};
    > +
    > +#define mips_elf_section_data(sec) \
    > +  ((struct _mips_elf_section_data *) elf_section_data (sec))
    > +
    > +#define canon_reloc_count(sec) \
    > +  ((struct _mips_elf_section_data *) elf_section_data (sec))->reloc_count
    > +
    
     Please do not export this structure or the `mips_elf_section_data' 
    accessor outside elfxx-mips.c.  Given that all the use is the 
    `canon_reloc_count' macro, make it a function and place it in 
    elfxx-mips.c.
    
     Also avoid namespace pollution and give it a target prefix, i.e. 
    `mips_elf_'.

Ok.