Re: [PATCH v1 4/6] LoongArch: Remove "elf_seg_map (info->output_bfd) == NULL" relaxation condition

[mengqinggang <mengqinggang@loongson.cn>]

在 2023/11/16 下午3:43, WANG Xuerui 写道:
> On 11/16/23 14:23, mengqinggang wrote:
>> This condition cause .so(shared object) can't be relaxed.
> "Previously the condition prevented shared objects from being relaxed."
>> Without this condition, we need to update program header size and 
>> .eh_frame_hdr
>> size before relaxation.
> "To remove the limitation, we need to ..."
>> ---
>>   bfd/elfnn-loongarch.c        | 24 ++++++++++++++++++++----
>>   ld/emultempl/loongarchelf.em | 18 ++++++++++++++++++
>>   2 files changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
>> index 7436a14441f..d331eefb8ac 100644
>> --- a/bfd/elfnn-loongarch.c
>> +++ b/bfd/elfnn-loongarch.c
>> @@ -3738,7 +3738,7 @@ loongarch_relax_delete_bytes (bfd *abfd,
>>     /* Relax pcalau12i,addi.d => pcaddi.  */
>>   static bool
>> -loongarch_relax_pcala_addi (bfd *abfd, asection *sec,
>> +loongarch_relax_pcala_addi (bfd *abfd, asection *sec, asection 
>> *sym_sec,
>>                  Elf_Internal_Rela *rel_hi, bfd_vma symval,
>>                  struct bfd_link_info *info, bool *again)
>>   {
>> @@ -3747,7 +3747,23 @@ loongarch_relax_pcala_addi (bfd *abfd, 
>> asection *sec,
>>     uint32_t pca = bfd_get (32, abfd, contents + rel_hi->r_offset);
>>     uint32_t add = bfd_get (32, abfd, contents + rel_lo->r_offset);
>>     uint32_t rd = pca & 0x1f;
>> +
>> +  /* Because previous sections' relax, output_offset may increase 
>> and need to
>> +     be updated before relax. But it update after relax in
>> +     size_input_section defaultly, so we manually updating here.  */
>
> This is rather hard to understand...
>
> "This section's output_offset may change after previous section(s) 
> have been relaxed, so it needs to be updated beforehand. 
> size_input_section already took care of updating it after relaxation, 
> so we additionally update once here."
>
> Does this sound okay?
>
>> +  sec->output_offset = sec->output_section->size;
>>     bfd_vma pc = sec_addr (sec) + rel_hi->r_offset;
>> +
>> +  /* If pc and symbol not in the same segment, add/sub segment 
>> alignment.
>> +     Fixme: if there are multiple readonly segments?  */
> Usually FIXME notes are spelled with all-caps to make it easier for 
> the eyes and tools.
>> +  if (!(sym_sec->flags & SEC_READONLY))
>> +    {
>> +      if (symval > pc)
>> +    pc -= info->maxpagesize;
>> +      else if (symval < pc)
>> +    pc += info->maxpagesize;
>> +    }
>> +
>>     const uint32_t addi_d = 0x02c00000;
>>     const uint32_t pcaddi = 0x18000000;
>>   @@ -3889,7 +3905,6 @@ loongarch_elf_relax_section (bfd *abfd, 
>> asection *sec,
>>         || sec->sec_flg0
>>         || (sec->flags & SEC_RELOC) == 0
>>         || sec->reloc_count == 0
>> -      || elf_seg_map (info->output_bfd) == NULL
>>         || (info->disable_target_specific_optimizations
>>         && info->relax_pass == 0)
>>         /* The exp_seg_relro_adjust is enum phase_enum (0x4),
>> @@ -4009,14 +4024,15 @@ loongarch_elf_relax_section (bfd *abfd, 
>> asection *sec,
>>         break;
>>       case R_LARCH_PCALA_HI20:
>>         if (0 == info->relax_pass && (i + 4) <= sec->reloc_count)
>> -        loongarch_relax_pcala_addi (abfd, sec, rel, symval, info, 
>> again);
>> +        loongarch_relax_pcala_addi (abfd, sec, sym_sec, rel, symval,
>> +                    info, again);
>>         break;
>>       case R_LARCH_GOT_PC_HI20:
>>         if (local_got && 0 == info->relax_pass
>>             && (i + 4) <= sec->reloc_count)
>>           {
>>             if (loongarch_relax_pcala_ld (abfd, sec, rel))
>> -        loongarch_relax_pcala_addi (abfd, sec, rel, symval,
>> +        loongarch_relax_pcala_addi (abfd, sec, sym_sec, rel, symval,
>>                           info, again);
>>           }
>>         break;
>> diff --git a/ld/emultempl/loongarchelf.em b/ld/emultempl/loongarchelf.em
>> index 4850feb8767..d81c99da48b 100644
>> --- a/ld/emultempl/loongarchelf.em
>> +++ b/ld/emultempl/loongarchelf.em
>> @@ -62,6 +62,24 @@ gld${EMULATION_NAME}_after_allocation (void)
>>       }
>>       }
>>   +  /* The program header size of executable file may increase.  */
>> +  if (bfd_get_flavour (link_info.output_bfd) == bfd_target_elf_flavour
>> +      && !bfd_link_relocatable (&link_info))
>> +    {
>> +      if (lang_phdr_list == NULL)
>> +        elf_seg_map (link_info.output_bfd) = NULL;
>> +      if (!_bfd_elf_map_sections_to_segments (link_info.output_bfd,
>> +                          &link_info,
>> +                          NULL))
>> +        einfo (_("%F%P: map sections to segments failed: %E\n"));
>> +    }
>> +
>> +  /* Adjust program header size and .eh_frame_hdr size before
>> +     lang_relax_sections. Without it, the vma of data segment may 
>> increase.  */
> Out of curiosity (and unfamiliarity), is it only "increases" and no 
> decreases?


After relaxation, the vma of data segment may decreases. But before 
relaxation,
the vma of data segment should only increase due to the increase in the 
number of
segments and the determination of the .eh_frame_hdr section size.


>> +  lang_do_assignments (lang_allocating_phase_enum);
>> +  lang_reset_memory_regions ();
>> +  lang_size_sections (NULL, true);
>> +
>>     enum phase_enum *phase = &(expld.dataseg.phase);
>>     bfd_elf${ELFSIZE}_loongarch_set_data_segment_info (&link_info, 
>> (int *) phase);
>>     /* gld${EMULATION_NAME}_map_segments (need_layout); */