diff mbox series

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

Message ID 20231116062307.3292483-5-mengqinggang@loongson.cn
State New
Headers
Series Fix some bugs of relaxation |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_binutils_check--master-arm fail Patch failed to apply

Commit Message

mengqinggang Nov. 16, 2023, 6:23 a.m. UTC 
  This condition cause .so(shared object) can't be relaxed.
Without this condition, we need to update program header size and .eh_frame_hdr
size before relaxation.
---
 bfd/elfnn-loongarch.c        | 24 ++++++++++++++++++++----
 ld/emultempl/loongarchelf.em | 18 ++++++++++++++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

Comments

WANG Xuerui Nov. 16, 2023, 7:43 a.m. UTC | #1 
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?
> +  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); */
mengqinggang Nov. 16, 2023, 9:38 a.m. UTC | #2 
在 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); */
diff mbox series

Patch

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.  */
+  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?  */
+  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.  */
+  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); */