[PATCH 0/3] further correct x86'es last-insn tracking

[PATCH 0/3] further correct x86'es last-insn tracking

[Jan Beulich]

..., again involving corrections to common code as well.

1: x86: don't needlessly override .bss
2: ELF: reliably invoke md_elf_section_change_hook()
3: x86: last-insn recording should be per-subsection

Jan

[PATCH 1/3] x86: don't needlessly override .bss

[Jan Beulich]

ELF, COFF, and Mach-O all have custom handlers for .bss. Don't override
those; install a handler only for a.out.
---
v2: New.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -181,7 +181,7 @@ static const reg_entry *build_modrm_byte
 static void output_insn (const struct last_insn *);
 static void output_imm (fragS *, offsetT);
 static void output_disp (fragS *, offsetT);
-#ifndef I386COFF
+#ifdef OBJ_AOUT
 static void s_bss (int);
 #endif
 #if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
@@ -1201,9 +1201,10 @@ const pseudo_typeS md_pseudo_table[] =
   {"align", s_align_ptwo, 0},
 #endif
   {"arch", set_cpu_arch, 0},
-#ifndef I386COFF
+#ifdef OBJ_AOUT
   {"bss", s_bss, 0},
-#else
+#endif
+#ifdef I386COFF
   {"lcomm", pe_lcomm, 1},
 #endif
   {"ffloat", float_cons, 'f'},
@@ -15475,17 +15476,13 @@ md_pcrel_from (fixS *fixP)
   return fixP->fx_size + fixP->fx_where + fixP->fx_frag->fr_address;
 }
 
-#ifndef I386COFF
+#ifdef OBJ_AOUT
 
 static void
 s_bss (int ignore ATTRIBUTE_UNUSED)
 {
   int temp;
 
-#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
-  if (IS_ELF)
-    obj_elf_section_change_hook ();
-#endif
   temp = get_absolute_expression ();
   subseg_set (bss_section, (subsegT) temp);
   demand_empty_rest_of_line ();

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

[Jan Beulich]

... 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?

--- 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)

[PATCH 3/3] x86: last-insn recording should be per-subsection

[Jan Beulich]

Otherwise intermediate subsection switches result in inconsistent
behavior. Leverage ELF's section change hook to switch state as
necessary, limiting overhead to the bare minimum when subsections aren't
used.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -15920,6 +15920,39 @@ i386_elf_section_type (const char *str,
   return -1;
 }
 
+void
+i386_elf_section_change_hook (void)
+{
+  struct i386_segment_info *info = &seg_info(now_seg)->tc_segment_info_data;
+  struct i386_segment_info *curr, *prev;
+
+  if (info->subseg == now_subseg)
+    return;
+
+  /* Find the (or make a) list entry to save state into.  */
+  for (prev = info; (curr = prev->next) != NULL; prev = curr)
+    if (curr->subseg == info->subseg)
+      break;
+  if (!curr)
+    {
+      curr = XNEW (struct i386_segment_info);
+      curr->subseg = info->subseg;
+      curr->next = NULL;
+      prev->next = curr;
+    }
+  curr->last_insn = info->last_insn;
+
+  /* Find the list entry to load state from.  */
+  for (curr = info->next; curr; curr = curr->next)
+    if (curr->subseg == now_subseg)
+      break;
+  if (curr)
+    info->last_insn = curr->last_insn;
+  else
+    memset (&info->last_insn, 0, sizeof (info->last_insn));
+  info->subseg = now_subseg;
+}
+
 #ifdef TE_SOLARIS
 void
 i386_solaris_fix_up_eh_frame (segT sec)
--- a/gas/config/tc-i386.h
+++ b/gas/config/tc-i386.h
@@ -294,6 +294,8 @@ struct i386_segment_info {
 	last_insn_prefix
       } kind;
   } last_insn;
+  subsegT subseg;
+  struct i386_segment_info *next;
 };
 
 #define TC_SEGMENT_INFO_TYPE struct i386_segment_info
@@ -395,6 +397,9 @@ extern void tc_x86_frame_initial_instruc
 #define md_elf_section_type(str,len) i386_elf_section_type (str, len)
 extern int i386_elf_section_type (const char *, size_t);
 
+#define md_elf_section_change_hook i386_elf_section_change_hook
+extern void i386_elf_section_change_hook (void);
+
 #ifdef TE_SOLARIS
 #define md_fix_up_eh_frame(sec) i386_solaris_fix_up_eh_frame (sec)
 extern void i386_solaris_fix_up_eh_frame (segT);
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -745,6 +745,8 @@ if [gas_32_check] then {
 	run_dump_test "nop-6"
 	run_dump_test "unique"
 
+	run_dump_test "lfence-subsect"
+
 	run_dump_test "property-1"
 
 	if {[istarget "*-*-linux*"]} then {
--- /dev/null
+++ b/gas/testsuite/gas/i386/lfence-subsect.d
@@ -0,0 +1,18 @@
+#as: -mlfence-before-indirect-branch=all
+#warning_output: lfence-section.e
+#objdump: -dw
+#name: -mlfence-before-indirect-branch=all w/ subsection switches
+
+.*: +file format .*
+
+Disassembly of section .text:
+
+0+ <_start>:
+ +[a-f0-9]+:	f3 ff d0             	repz call \*%eax
+ +[a-f0-9]+:	f3 c3                	repz ret
+ +[a-f0-9]+:	cc                   	int3
+ +[a-f0-9]+:	cc                   	int3
+ +[a-f0-9]+:	cc                   	int3
+
+0+8 <aux1>:
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/i386/lfence-subsect.s
@@ -0,0 +1,19 @@
+	.text
+_start:
+	rep
+
+	.subsection 2
+aux1:
+	nop
+
+	.previous
+	call *%eax
+	rep
+
+	.pushsection .text, 2
+aux2:
+	nop
+
+	.popsection
+	ret
+	.p2align 2, 0xcc

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

[Nick Clifton]

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

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

[Jan Beulich]

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