This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH V2 3/3] bfd: fix the deletion of relocs in mips64
Jose,
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
elfxx-mips.c.
Also avoid namespace pollution and give it a target prefix, i.e.
`mips_elf_'.
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 '.rela.gnu.build.attributes' 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 '.rela.gnu.build.attributes' 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.
Maciej