Re: [PATCH v2] BFD: Fix the bug of R_LARCH_AGLIN caused by discard section

[Fangrui Song <i@maskray.me>]

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?