This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


 Thank you for your submission and apologies for the long RTT.

 Please post future patches with actual diffs for ChangeLog files omitted, 
so that they can be applied without a need for hand-editing.

> 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.

 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.

> 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?

> +
> +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;


> 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).

> +};
> +
> +#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 

 Also avoid namespace pollution and give it a target prefix, i.e. 

 Again, if indeed needed.

 I have successfully passed your change through regression-testing over my 
list of MIPS targets as follows:

mips-ecoff mips-elf mips-img-elf mips-mti-elf mips-sde-elf mips-sgi-irix5 
mips-sgi-irix6 mips-freebsd mips-linux mips-img-linux mips-mti-linux 
mips-netbsd mips-vxworks mips64-freebsd mips64-linux mips64-img-linux 
mips64-mti-linux mips64-openbsd mips64el-freebsd mips64el-linux 
mips64el-img-linux mips64el-mti-linux mips64el-openbsd mipsel-ecoff 
mipsel-elf mipsel-img-elf mipsel-mti-elf mipsel-freebsd mipsel-linux 
mipsel-img-linux mipsel-mti-linux mipsel-netbsd mipsel-vxworks 
mipsisa32-elf mipsisa32-linux mipsisa32el-elf mipsisa32el-linux 
mipsisa64-elf mipsisa64-linux mipsisa64el-elf mipsisa64el-linux tx39-elf

with these failures removed:

mips64-openbsd  -FAIL: merge notes section (64-bits)
mips64el-openbsd  -FAIL: merge notes section (64-bits)

and relocation data from the affected test case looking correct:

$ as .../binutils/testsuite/binutils-all/note-2-64.s -o tmpdir/bintest.o
$ objcopy --merge-notes tmpdir/bintest.o tmpdir/copy.o
$ readelf -r tmpdir/bintest.o

Relocation section '' at offset 0x3e8 contains 3 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000010  000900000012 R_MIPS_64         0000000000000100 note1.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
000000000070  000a00000012 R_MIPS_64         0000000000000104 note2.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
0000000000d0  000c00000012 R_MIPS_64         0000000000000108 note3.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
$ readelf -r tmpdir/copy.o

Relocation section '' at offset 0x390 contains 2 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000010  000900000012 R_MIPS_64         0000000000000100 note1.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
000000000070  000a00000012 R_MIPS_64         0000000000000104 note2.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE

so functionally the change looks good to me.  You could have posted these 
`readelf' dumps along your submission as a proof of correctness (well, for 
the test case at least).

 Please repost an updated change, or if you have any questions or comments 
before you start making modifications, then feel free to ask.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]