[PATCH v2] BFD: Fix the bug of R_LARCH_AGLIN caused by discard section
Fangrui Song
i@maskray.me
Fri Apr 19 06:16:13 GMT 2024
On Thu, Apr 18, 2024 at 11:04 PM Fangrui Song <i@maskray.me> wrote:
>
> On Fri, Mar 22, 2024 at 1:29 AM mengqinggang <mengqinggang@loongson.cn> wrote:
> >
> > To represent the first and third expression of .align, R_LARCH_ALIGN need to
> > associate with a symbol. We defind a local symbol for R_LARCH_AGLIN.
> > But if the section of the local symbo is discarded, it may result in
> > a undefined symbol error.
> >
> > Instead, we use the section name symbols, and this does not need to
> > add extra symbols.
> >
> > During partial linking (ld -r), if the symbol associated with a relocation is
> > STT_SECTION type, the addend of relocation needs to add the section output
> > offset. We prevent it for R_LARCH_ALIGN.
> >
> > The elf_backend_data.rela_normal only can set all relocations of a target to
> > rela_normal. Add a new function is_rela_normal to elf_backend_data, it can
> > set part of relocations to rela_normal.
> > ---
> > bfd/elf-bfd.h | 4 ++
> > bfd/elflink.c | 5 +-
> > bfd/elfnn-loongarch.c | 16 ++++++
> > bfd/elfxx-target.h | 5 ++
> > gas/config/tc-loongarch.c | 5 +-
> > gas/testsuite/gas/loongarch/relax_align.d | 56 ++++++++-----------
> > .../ld-loongarch-elf/relax-align-discard.lds | 4 ++
> > .../ld-loongarch-elf/relax-align-discard.s | 17 ++++++
> > ld/testsuite/ld-loongarch-elf/relax.exp | 12 ++++
> > 9 files changed, 86 insertions(+), 38 deletions(-)
> > create mode 100644 ld/testsuite/ld-loongarch-elf/relax-align-discard.lds
> > create mode 100644 ld/testsuite/ld-loongarch-elf/relax-align-discard.s
> >
> > diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> > index c5d325435b6..af507b93df5 100644
> > --- a/bfd/elf-bfd.h
> > +++ b/bfd/elf-bfd.h
> > @@ -1721,6 +1721,10 @@ struct elf_backend_data
> > backend relocate_section routine for relocatable linking. */
> > unsigned rela_normal : 1;
> >
> > + /* Whether a relocation is rela_normal. Compared with rela_normal,
> > + is_rela_normal can set part of relocations to rela_normal. */
> > + bool (*is_rela_normal) (Elf_Internal_Rela *);
> > +
> > /* Set if DT_REL/DT_RELA/DT_RELSZ/DT_RELASZ should not include PLT
> > relocations. */
> > unsigned dtrel_excludes_plt : 1;
> > diff --git a/bfd/elflink.c b/bfd/elflink.c
> > index 5a6cb07b2ce..8223db98186 100644
> > --- a/bfd/elflink.c
> > +++ b/bfd/elflink.c
> > @@ -11692,7 +11692,10 @@ elf_link_input_bfd (struct elf_final_link_info *flinfo, bfd *input_bfd)
> > {
> > rel_hash = PTR_ADD (esdo->rela.hashes, esdo->rela.count);
> > rela_hash_list = rel_hash;
> > - rela_normal = bed->rela_normal;
> > + if (bed->is_rela_normal != NULL)
> > + rela_normal = bed->is_rela_normal (irela);
> > + else
> > + rela_normal = bed->rela_normal;
> > }
> >
> > irela->r_offset = _bfd_elf_section_offset (output_bfd,
> > diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> > index c42052f9321..1679aa5da7d 100644
> > --- a/bfd/elfnn-loongarch.c
> > +++ b/bfd/elfnn-loongarch.c
> > @@ -5454,6 +5454,21 @@ elf_loongarch64_hash_symbol (struct elf_link_hash_entry *h)
> > return _bfd_elf_hash_symbol (h);
> > }
> >
> > +/* If a relocation is rela_normal and the symbol associated with the
> > + relocation is STT_SECTION type, the addend of the relocation would add
> > + sec->output_offset when partial linking (ld -r).
> > + See elf_backend_data.rela_normal and elf_link_input_bfd().
> > + The addend of R_LARCH_ALIGN is used to represent the first and third
> > + expression of .align, it should be a constant when linking. */
> > +
> > +static bool
> > +loongarch_elf_is_rela_normal (Elf_Internal_Rela *rel)
> > +{
> > + if (R_LARCH_ALIGN == ELFNN_R_TYPE (rel->r_info))
> > + return false;
> > + return true;
> > +}
> > +
> > #define TARGET_LITTLE_SYM loongarch_elfNN_vec
> > #define TARGET_LITTLE_NAME "elfNN-loongarch"
> > #define ELF_ARCH bfd_arch_loongarch
> > @@ -5489,6 +5504,7 @@ elf_loongarch64_hash_symbol (struct elf_link_hash_entry *h)
> > #define elf_backend_grok_psinfo loongarch_elf_grok_psinfo
> > #define elf_backend_hash_symbol elf_loongarch64_hash_symbol
> > #define bfd_elfNN_bfd_relax_section loongarch_elf_relax_section
> > +#define elf_backend_is_rela_normal loongarch_elf_is_rela_normal
> >
> > #define elf_backend_dtrel_excludes_plt 1
> >
> > diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
> > index 1e6992b5793..6e2d948b69b 100644
> > --- a/bfd/elfxx-target.h
> > +++ b/bfd/elfxx-target.h
> > @@ -709,6 +709,10 @@
> > #define elf_backend_rela_normal 0
> > #endif
> >
> > +#ifndef elf_backend_is_rela_normal
> > +#define elf_backend_is_rela_normal NULL
> > +#endif
> > +
> > #ifndef elf_backend_dtrel_excludes_plt
> > #define elf_backend_dtrel_excludes_plt 0
> > #endif
> > @@ -955,6 +959,7 @@ static const struct elf_backend_data elfNN_bed =
> > elf_backend_default_use_rela_p,
> > elf_backend_rela_plts_and_copies_p,
> > elf_backend_rela_normal,
> > + elf_backend_is_rela_normal,
> > elf_backend_dtrel_excludes_plt,
> > elf_backend_sign_extend_vma,
> > elf_backend_want_got_plt,
> > diff --git a/gas/config/tc-loongarch.c b/gas/config/tc-loongarch.c
> > index 30aefce36fd..6b1a89738ef 100644
> > --- a/gas/config/tc-loongarch.c
> > +++ b/gas/config/tc-loongarch.c
> > @@ -1791,10 +1791,7 @@ loongarch_frag_align_code (int n, int max)
> > if (fragP->fr_subtype != 0 && offset > fragP->fr_subtype). */
> > if (max > 0 && (bfd_vma) max < worst_case_bytes)
> > {
> > - s = symbol_find (".Lla-relax-align");
> > - if (s == NULL)
> > - s = (symbolS *)local_symbol_make (".Lla-relax-align", now_seg,
> > - &zero_address_frag, 0);
> > + s = symbol_find (now_seg->name);
> > ex.X_add_symbol = s;
> > ex.X_op = O_symbol;
> > ex.X_add_number = (max << 8) | n;
> > diff --git a/gas/testsuite/gas/loongarch/relax_align.d b/gas/testsuite/gas/loongarch/relax_align.d
> > index fc1fd032611..6710927be1b 100644
> > --- a/gas/testsuite/gas/loongarch/relax_align.d
> > +++ b/gas/testsuite/gas/loongarch/relax_align.d
> > @@ -7,40 +7,30 @@
> >
> > Disassembly of section .text:
> >
> > -[ ]*0000000000000000 <.Lla-relax-align>:
> > -[ ]+0:[ ]+4c000020[ ]+ret
> > -[ ]+4:[ ]+03400000[ ]+nop
> > -[ ]+4: R_LARCH_ALIGN[ ]+\*ABS\*\+0xc
> > +[ ]*0000000000000000 <.text>:
> > +[ ]+0:[ ]+1a000004[ ]+pcalau12i[ ]+\$a0, 0
> > +[ ]+0: R_LARCH_PCALA_HI20[ ]+L1
> > +[ ]+0: R_LARCH_RELAX[ ]+\*ABS\*
> > +[ ]+4:[ ]+02c00084[ ]+addi.d[ ]+\$a0, \$a0, 0
> > +[ ]+4: R_LARCH_PCALA_LO12[ ]+L1
> > +[ ]+4: R_LARCH_RELAX[ ]+\*ABS\*
> > [ ]+8:[ ]+03400000[ ]+nop
> > +[ ]+8: R_LARCH_ALIGN[ ]+.text\+0x4
> > [ ]+c:[ ]+03400000[ ]+nop
> > -[ ]+10:[ ]+4c000020[ ]+ret
> > -[ ]+14:[ ]+03400000[ ]+nop
> > -[ ]+14: R_LARCH_ALIGN[ ]+\*ABS\*\+0xc
> > -[ ]+18:[ ]+03400000[ ]+nop
> > +[ ]+10:[ ]+03400000[ ]+nop
> > +[ ]+14:[ ]+1a000004[ ]+pcalau12i[ ]+\$a0, 0
> > +[ ]+14: R_LARCH_PCALA_HI20[ ]+L1
> > +[ ]+14: R_LARCH_RELAX[ ]+\*ABS\*
> > +[ ]+18:[ ]+02c00084[ ]+addi.d[ ]+\$a0, \$a0, 0
> > +[ ]+18: R_LARCH_PCALA_LO12[ ]+L1
> > +[ ]+18: R_LARCH_RELAX[ ]+\*ABS\*
> > [ ]+1c:[ ]+03400000[ ]+nop
> > -[ ]+20:[ ]+4c000020[ ]+ret
> > +[ ]+1c: R_LARCH_ALIGN[ ]+.text\+0x404
> > +[ ]+20:[ ]+03400000[ ]+nop
> > [ ]+24:[ ]+03400000[ ]+nop
> > -[ ]+24: R_LARCH_ALIGN[ ]+.Lla-relax-align\+0x104
> > -[ ]+28:[ ]+03400000[ ]+nop
> > -[ ]+2c:[ ]+03400000[ ]+nop
> > -[ ]+30:[ ]+4c000020[ ]+ret
> > -[ ]+34:[ ]+03400000[ ]+nop
> > -[ ]+34: R_LARCH_ALIGN[ ]+.Lla-relax-align\+0xb04
> > -[ ]+38:[ ]+03400000[ ]+nop
> > -[ ]+3c:[ ]+03400000[ ]+nop
> > -[ ]+40:[ ]+4c000020[ ]+ret
> > -[ ]+44:[ ]+03400000[ ]+nop
> > -[ ]+44: R_LARCH_ALIGN[ ]+\*ABS\*\+0xc
> > -[ ]+48:[ ]+03400000[ ]+nop
> > -[ ]+4c:[ ]+03400000[ ]+nop
> > -[ ]+50:[ ]+4c000020[ ]+ret
> > -[ ]+54:[ ]+03400000[ ]+nop
> > -[ ]+54: R_LARCH_ALIGN[ ]+\*ABS\*\+0xc
> > -[ ]+58:[ ]+03400000[ ]+nop
> > -[ ]+5c:[ ]+03400000[ ]+nop
> > -[ ]+60:[ ]+4c000020[ ]+ret
> > -[ ]+64:[ ]+03400000[ ]+nop
> > -[ ]+64: R_LARCH_ALIGN[ ]+\*ABS\*\+0xc
> > -[ ]+68:[ ]+03400000[ ]+nop
> > -[ ]+6c:[ ]+03400000[ ]+nop
> > -[ ]+70:[ ]+4c000020[ ]+ret
> > +[ ]+28:[ ]+1a000004[ ]+pcalau12i[ ]+\$a0, 0
> > +[ ]+28: R_LARCH_PCALA_HI20[ ]+L1
> > +[ ]+28: R_LARCH_RELAX[ ]+\*ABS\*
> > +[ ]+2c:[ ]+02c00084[ ]+addi.d[ ]+\$a0, \$a0, 0
> > +[ ]+2c: R_LARCH_PCALA_LO12[ ]+L1
> > +[ ]+2c: R_LARCH_RELAX[ ]+\*ABS\*
> > diff --git a/ld/testsuite/ld-loongarch-elf/relax-align-discard.lds b/ld/testsuite/ld-loongarch-elf/relax-align-discard.lds
> > new file mode 100644
> > index 00000000000..4a81323d926
> > --- /dev/null
> > +++ b/ld/testsuite/ld-loongarch-elf/relax-align-discard.lds
> > @@ -0,0 +1,4 @@
> > +SECTIONS
> > +{
> > + /DISCARD/ : { *(.another.*) }
> > +}
> > diff --git a/ld/testsuite/ld-loongarch-elf/relax-align-discard.s b/ld/testsuite/ld-loongarch-elf/relax-align-discard.s
> > new file mode 100644
> > index 00000000000..b65d63f370f
> > --- /dev/null
> > +++ b/ld/testsuite/ld-loongarch-elf/relax-align-discard.s
> > @@ -0,0 +1,17 @@
> > +# Use the section name symbol for R_LARCH_ALIGN to avoid discard section problem
> > +.section ".another.text", "ax"
> > +.cfi_startproc
> > +break 0
> > +.cfi_def_cfa_offset 16
> > +.p2align 5
> > +break 1
> > +.cfi_endproc
> > +
> > +.text
> > +.cfi_startproc
> > +break 0
> > +.cfi_def_cfa_offset 16
> > +.p2align 5
> > +break 1
> > +.cfi_endproc
> > +
> > diff --git a/ld/testsuite/ld-loongarch-elf/relax.exp b/ld/testsuite/ld-loongarch-elf/relax.exp
> > index 7d95a9ca41d..ed71fb45b46 100644
> > --- a/ld/testsuite/ld-loongarch-elf/relax.exp
> > +++ b/ld/testsuite/ld-loongarch-elf/relax.exp
> > @@ -295,6 +295,18 @@ if [istarget loongarch64-*-*] {
> > "relax-align" \
> > ] \
> > ]
> > +
> > + run_ld_link_tests \
> > + [list \
> > + [list \
> > + "loongarch relax align discard" \
> > + "-e 0x0 -T relax-align-discard.lds -r" "" \
> > + "" \
> > + {relax-align-discard.s} \
> > + {} \
> > + "relax-align-discard" \
> > + ] \
> > + ]
> > }
> >
> > set objdump_flags "-s -j .data"
> > --
> > 2.36.0
>
>
> I just saw this was pushed as commit daeda14191c1710ce967259a47ef4e0a3fb6eebf.
>
> The addition of the generic elf_backend_is_rela_normal flag seems like
> something a global maintainer should take a closer look at.
> In particular, I'm curious if Alan, the author of the "rela_normal"
> commit (b491616acb5462a3694160ffef6413c160fed10a), has any thoughts on
> this.
>
> The idea appears to be
> (https://github.com/loongson/la-abi-specs/blob/release/laelf.adoc#:~:text=R_LARCH_ALIGN)
>
> .text
> break 1
> .p2align 4, , 8 // R_LARCH_ALIGN .text+0x0804
> break 8
>
> In a relocatable link, the addend associated with the STT_SECTION
> symbol is kept unchanged.
>
> > But if the section of the local symbo is discarded, it may result in a undefined symbol error.
>
> How does this happen when the R_LARCH_ALIGN relocation references
> another local symbol instead of .text ?
I should make it clear that I think this R_LARCH_ALIGN referencing
STT_SECTION with addend align+256*align_limit representation is
questionable.
Why do you break the regular semantics of STT_SECTION relocatable linking?
Can an absolute symbol be used instead?
More information about the Binutils
mailing list