diff mbox series

[2/3] ELF: reliably invoke md_elf_section_change_hook()

Message ID 00ae907a-8fe1-44b1-9e1f-2c20d33a31a5@suse.com
State New
Headers
Series further correct x86'es last-insn tracking |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed

Commit Message

Jan Beulich Dec. 8, 2023, 7:01 a.m. UTC 
  ... after any (sub)section change. While certain existing target hooks
only look at now_seg, for a few others it looks as if failing to do so
could have caused anomalies if sub-sections were used. In any event a
subsequent x86 change is going to require the sub-section to be properly
in place at the time the hook is invoked.

This primarily means for obj_elf_section() to pass the new subsection
into obj_elf_change_section(), for it to be set right away (ahead of
invoking the hook). While adding the new function parameter, take the
opportunity and change two adjacent boolean ones to "bool".

Also adjust obj_elf_ident() to invoke the hook after all section
changes. (Note that obj_elf_version(), which also changes sections and
then changes them back, has no hook invocation at all so far, so none
are added. Presumably there is a reason for this difference in
behavior.)
---
Considering that no caller outside of obj-elf.c cares about the existing
"push" and the new "new_subsection" arguments, an alternative might be
to drop the "push" one in a prereq patch, thus eliminating the need for
this one to touch various targets. Thoughts anyone?

Comments

Nick Clifton Dec. 8, 2023, 12:07 p.m. UTC | #1 
Hi Jan,

> Considering that no caller outside of obj-elf.c cares about the existing
> "push" and the new "new_subsection" arguments, an alternative might be
> to drop the "push" one in a prereq patch, thus eliminating the need for
> this one to touch various targets. Thoughts anyone?

I like the idea of a simpler patch that does not involve updating any of
the target code.  So if it works then I would suggest that you go for it.

One other, very minor point:

> +  subsegT new_subsection = 0;

There are a lot of places where we use 0 to indicate no-subsection.  I
wonder if the code would be easier to read if we had a constant defined
in subseg.h and used that instead.  eg:

      subsegT new_subsection = NO_SUBSEG;
or:
      subseg_set (comment_section, NO_SUBSEG);

Just a thought.

Cheers
   Nick
Jan Beulich Dec. 8, 2023, 12:25 p.m. UTC | #2 
On 08.12.2023 13:07, Nick Clifton wrote:
>> Considering that no caller outside of obj-elf.c cares about the existing
>> "push" and the new "new_subsection" arguments, an alternative might be
>> to drop the "push" one in a prereq patch, thus eliminating the need for
>> this one to touch various targets. Thoughts anyone?
> 
> I like the idea of a simpler patch that does not involve updating any of
> the target code.

Ftaod, the same targets will need updating nevertheless, just that it's
not going to happen in this (functional) change.

>  So if it works then I would suggest that you go for it.

I don't see why it shouldn't, so I'll see about doing the split.

> One other, very minor point:
> 
>> +  subsegT new_subsection = 0;
> 
> There are a lot of places where we use 0 to indicate no-subsection.  I
> wonder if the code would be easier to read if we had a constant defined
> in subseg.h and used that instead.  eg:
> 
>       subsegT new_subsection = NO_SUBSEG;
> or:
>       subseg_set (comment_section, NO_SUBSEG);
> 
> Just a thought.

I probably could be convinced, but I don't like this. First and foremost
it's not really "no subsection", it really is just the first of potentially
many (yet typically the only one).

Jan
diff mbox series

Patch

--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -546,8 +546,9 @@  obj_elf_change_section (const char *name
 			bfd_vma attr,
 			int entsize,
 			struct elf_section_match *match_p,
-			int linkonce,
-			int push)
+			bool linkonce,
+			bool push,
+			subsegT new_subsection)
 {
   asection *old_sec;
   segT sec;
@@ -585,10 +586,10 @@  obj_elf_change_section (const char *name
   if (old_sec)
     {
       sec = old_sec;
-      subseg_set (sec, 0);
+      subseg_set (sec, new_subsection);
     }
   else
-    sec = subseg_force_new (name, 0);
+    sec = subseg_force_new (name, new_subsection);
 
   bed = get_elf_backend_data (stdoutput);
   ssect = (*bed->get_sec_type_attr) (stdoutput, sec);
@@ -1103,8 +1104,8 @@  obj_elf_section (int push)
   bfd_vma attr;
   bfd_vma gnu_attr;
   int entsize;
-  int linkonce;
-  subsegT new_subsection = -1;
+  bool linkonce;
+  subsegT new_subsection = 0;
   struct elf_section_match match;
   unsigned long linked_to_section_index = -1UL;
 
@@ -1489,7 +1490,7 @@  obj_elf_section (int push)
     }
 
   obj_elf_change_section (name, type, attr, entsize, &match, linkonce,
-			  push);
+			  push, new_subsection);
 
   if (linked_to_section_index != -1UL)
     {
@@ -1497,9 +1498,6 @@  obj_elf_section (int push)
       elf_section_data (now_seg)->this_hdr.sh_link = linked_to_section_index;
       /* FIXME: Should we perform some sanity checking on the section index ?  */
     }
-
-  if (push && new_subsection != -1)
-    subseg_set (now_seg, new_subsection);
 }
 
 /* Change to the .bss section.  */
@@ -2519,9 +2517,17 @@  obj_elf_ident (int ignore ATTRIBUTE_UNUS
       *p = 0;
     }
   else
-    subseg_set (comment_section, 0);
+    {
+      subseg_set (comment_section, 0);
+#ifdef md_elf_section_change_hook
+      md_elf_section_change_hook ();
+#endif
+    }
   stringer (8 + 1);
   subseg_set (old_section, old_subsection);
+#ifdef md_elf_section_change_hook
+  md_elf_section_change_hook ();
+#endif
 }
 
 #ifdef INIT_STAB_SECTION
--- a/gas/config/obj-elf.h
+++ b/gas/config/obj-elf.h
@@ -198,7 +198,7 @@  extern void obj_elf_data (int);
 extern void obj_elf_text (int);
 extern void obj_elf_change_section
   (const char *, unsigned int, bfd_vma, int, struct elf_section_match *,
-   int, int);
+   bool, bool, subsegT);
 extern void obj_elf_vtable_inherit (int);
 extern void obj_elf_vtable_entry (int);
 extern struct fix * obj_elf_get_vtable_inherit (void);
--- a/gas/config/tc-arm.c
+++ b/gas/config/tc-arm.c
@@ -27775,7 +27775,7 @@  start_unwind_section (const segT text_se
     }
 
   obj_elf_change_section (sec_name, type, flags, 0, &match,
-			  linkonce, 0);
+			  linkonce, false, 0);
 
   /* Set the section link for index tables.  */
   if (idx)
--- a/gas/config/tc-ia64.c
+++ b/gas/config/tc-ia64.c
@@ -1139,7 +1139,7 @@  obj_elf_vms_common (int ignore ATTRIBUTE
   obj_elf_change_section
     (sec_name, SHT_NOBITS,
      SHF_ALLOC | SHF_WRITE | SHF_IA_64_VMS_OVERLAID | SHF_IA_64_VMS_GLOBAL,
-     0, NULL, 1, 0);
+     0, NULL, true, false, 0);
 
   S_SET_VALUE (symbolP, 0);
   S_SET_SIZE (symbolP, size);
--- a/gas/config/tc-microblaze.c
+++ b/gas/config/tc-microblaze.c
@@ -150,7 +150,7 @@  microblaze_s_data (int ignore ATTRIBUTE_
 {
 #ifdef OBJ_ELF
   obj_elf_change_section (".data", SHT_PROGBITS, SHF_ALLOC+SHF_WRITE,
-			  0, 0, 0, 0);
+			  0, 0, false, false, 0);
 #else
   s_data (ignore);
 #endif
@@ -163,7 +163,7 @@  microblaze_s_sdata (int ignore ATTRIBUTE
 {
 #ifdef OBJ_ELF
   obj_elf_change_section (".sdata", SHT_PROGBITS, SHF_ALLOC+SHF_WRITE,
-			  0, 0, 0, 0);
+			  0, 0, false, false, 0);
 #else
   s_data (ignore);
 #endif
@@ -282,7 +282,7 @@  microblaze_s_rdata (int localvar)
     {
       /* rodata.  */
       obj_elf_change_section (".rodata", SHT_PROGBITS, SHF_ALLOC,
-			      0, 0, 0, 0);
+			      0, 0, false, false, 0);
       if (rodata_segment == 0)
 	rodata_segment = subseg_new (".rodata", 0);
     }
@@ -290,7 +290,7 @@  microblaze_s_rdata (int localvar)
     {
       /* 1 .sdata2.  */
       obj_elf_change_section (".sdata2", SHT_PROGBITS, SHF_ALLOC,
-			      0, 0, 0, 0);
+			      0, 0, false, false, 0);
     }
 #else
   s_data (ignore);
@@ -303,12 +303,12 @@  microblaze_s_bss (int localvar)
 #ifdef OBJ_ELF
   if (localvar == 0) /* bss.  */
     obj_elf_change_section (".bss", SHT_NOBITS, SHF_ALLOC+SHF_WRITE,
-			    0, 0, 0, 0);
+			    0, 0, false, false, 0);
   else if (localvar == 1)
     {
       /* sbss.  */
       obj_elf_change_section (".sbss", SHT_NOBITS, SHF_ALLOC+SHF_WRITE,
-			      0, 0, 0, 0);
+			      0, 0, false, false, 0);
       if (sbss_segment == 0)
 	sbss_segment = subseg_new (".sbss", 0);
     }
--- a/gas/config/tc-mips.c
+++ b/gas/config/tc-mips.c
@@ -16435,7 +16435,7 @@  s_change_section (int ignore ATTRIBUTE_U
     section_type = SHT_PROGBITS;
 
   obj_elf_change_section (section_name, section_type, section_flag,
-			  section_entry_size, 0, 0, 0);
+			  section_entry_size, 0, false, false, 0);
 }
 
 void
--- a/gas/config/tc-msp430.c
+++ b/gas/config/tc-msp430.c
@@ -622,7 +622,7 @@  msp430_profiler (int dummy ATTRIBUTE_UNU
   subseg = now_subseg;
 
   /* Now go to .profiler section.  */
-  obj_elf_change_section (".profiler", SHT_PROGBITS, 0, 0, 0, 0, 0);
+  obj_elf_change_section (".profiler", SHT_PROGBITS, 0, 0, 0, false, false, 0);
 
   /* Save flags.  */
   emit_expr (& exp, 2);
--- a/gas/config/tc-rx.c
+++ b/gas/config/tc-rx.c
@@ -488,7 +488,7 @@  parse_rx_section (char * name)
       else
 	type = SHT_NOBITS;
 
-      obj_elf_change_section (name, type, attr, 0, NULL, false, false);
+      obj_elf_change_section (name, type, attr, 0, NULL, false, false, 0);
     }
   else /* Try not to redefine a section, especially B_1.  */
     {
@@ -503,7 +503,7 @@  parse_rx_section (char * name)
 	| ((flags & SEC_STRINGS) ? SHF_STRINGS : 0)
 	| ((flags & SEC_THREAD_LOCAL) ? SHF_TLS : 0);
 
-      obj_elf_change_section (name, type, attr, 0, NULL, false, false);
+      obj_elf_change_section (name, type, attr, 0, NULL, false, false, 0);
     }
 
   bfd_set_section_alignment (now_seg, align);
--- a/gas/config/tc-tic6x.c
+++ b/gas/config/tc-tic6x.c
@@ -4662,7 +4662,7 @@  tic6x_start_unwind_section (const segT t
     }
 
   obj_elf_change_section (sec_name, type, flags, 0, &match,
-			  linkonce, 0);
+			  linkonce, false, 0);
 
   /* Set the section link for index tables.  */
   if (idx)