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

[Jinyang He <hejinyang@loongson.cn>]

On 2024-04-19 14:16, Fangrui Song wrote:
> 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?
Here just some my thoughts about ABS symbol. It cause more symbol cost
in the "*.o" files. For ABS symbols, if several "*.o" files are linked
together, there will be several extra symbols. Llvm works OK by creating
ABS symbol (I tried, but forgot the details), but GNU AS is not. Because
it applies its ABS value to addend (, Qinggang has investigated before.).