[v2] gas/ELF: don't accumulate .type settings

Message ID 2eff1f48-450e-15e5-48b6-189f138af186@suse.com
State New
Headers show
Series
  • [v2] gas/ELF: don't accumulate .type settings
Related show

Commit Message

Jan Beulich July 3, 2019, 8:05 a.m.
Recently a patch was submitted for a Xen Project test harness binary to
override the compiler specified @object to @func (see [1]). In a reply I
suggested we shouldn't make ourselves dependent on currently unspecified
behavior of gas here: It accumulates all requests, and then
bfd/elf.c:swap_out_syms(), in an apparently ad hoc manner, prioritizes
certain flags over others.

Make the behavior predictable: Generally the last .type is what counts.
Exceptions are directives which set multiple bits (TLS, IFUNC, and
UNIQUE): Subsequent directives requesting just the more generic bit
(i.e. FUNC following IFUNC) won't clear the more specific one.  Warn
about incompatible changes, except from/to STT_NOTYPE.

Also add a new target hook, which hppa wants to use right away afaict.

In the course of adding the warning I ran into two ld testsuite
failures.  I can only assume that it was a copy-and-paste mistake that
lead to the same symbol having its type set twice.

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01980.html

gas/
2019-07-03  Jan Beulich  <jbeulich@suse.com>

	* config/obj-elf.c (obj_elf_type): Check for conflicts between
	old and new types.
	* config/tc-hppa.h (md_elf_symbol_type_change): New.
	* doc/as.texi: Mention warning behavior for the ELF flavor of
	.type.
	* testsuite/gas/elf/type-2.e, testsuite/gas/elf/type-2.l,
	testsuite/gas/elf/type-2.s: New.
	* testsuite/gas/elf/elf.exp: Run new test.

ld/
2019-07-03  Jan Beulich  <jbeulich@suse.com>

	* testsuite/ld-elf/group9.s: Correct argument of .type.

Comments

Nick Clifton July 3, 2019, 2:15 p.m. | #1
Hi Jan,

> gas/

> 2019-07-03  Jan Beulich  <jbeulich@suse.com>

> 

> 	* config/obj-elf.c (obj_elf_type): Check for conflicts between

> 	old and new types.

> 	* config/tc-hppa.h (md_elf_symbol_type_change): New.

> 	* doc/as.texi: Mention warning behavior for the ELF flavor of

> 	.type.

> 	* testsuite/gas/elf/type-2.e, testsuite/gas/elf/type-2.l,

> 	testsuite/gas/elf/type-2.s: New.

> 	* testsuite/gas/elf/elf.exp: Run new test.

> 

> ld/

> 2019-07-03  Jan Beulich  <jbeulich@suse.com>

> 

> 	* testsuite/ld-elf/group9.s: Correct argument of .type.

 
Approved - please apply - and thanks for doing this.

Cheers
  Nick
Alan Modra July 8, 2019, 5:42 a.m. | #2
On Wed, Jul 03, 2019 at 08:05:48AM +0000, Jan Beulich wrote:
> 	* config/obj-elf.c (obj_elf_type): Check for conflicts between

> 	old and new types.

> 	* config/tc-hppa.h (md_elf_symbol_type_change): New.

> 	* doc/as.texi: Mention warning behavior for the ELF flavor of

> 	.type.

> 	* testsuite/gas/elf/type-2.e, testsuite/gas/elf/type-2.l,

> 	testsuite/gas/elf/type-2.s: New.

> 	* testsuite/gas/elf/elf.exp: Run new test.


aarch64_be-linux-gnu_ilp32  +FAIL: elf type-2 list
aarch64-linux  +FAIL: elf type-2 list
nds32be-elf  +FAIL: elf type-2 list
nds32le-linux  +FAIL: elf type-2 list
rl78-elf  +FAIL: elf type-2 list

Plus a large number of mips-sgi-irix6 tests, all due to "symbol ..
already has its type set" warnings.

-- 
Alan Modra
Australia Development Lab, IBM
Jan Beulich July 8, 2019, 6:53 a.m. | #3
On 08.07.2019 07:42, Alan Modra wrote:
> On Wed, Jul 03, 2019 at 08:05:48AM +0000, Jan Beulich wrote:

>> 	* config/obj-elf.c (obj_elf_type): Check for conflicts between

>> 	old and new types.

>> 	* config/tc-hppa.h (md_elf_symbol_type_change): New.

>> 	* doc/as.texi: Mention warning behavior for the ELF flavor of

>> 	.type.

>> 	* testsuite/gas/elf/type-2.e, testsuite/gas/elf/type-2.l,

>> 	testsuite/gas/elf/type-2.s: New.

>> 	* testsuite/gas/elf/elf.exp: Run new test.

> 

> aarch64_be-linux-gnu_ilp32  +FAIL: elf type-2 list

> aarch64-linux  +FAIL: elf type-2 list

> nds32be-elf  +FAIL: elf type-2 list

> nds32le-linux  +FAIL: elf type-2 list

> rl78-elf  +FAIL: elf type-2 list

> 

> Plus a large number of mips-sgi-irix6 tests, all due to "symbol ..

> already has its type set" warnings.


Well, I can see about looking into these, but not before some time
next week. Until then all I can suggest is for someone to revert
the commit if the failures need to be taken care of quickly. I have
to admit though that it is quite unobvious to me why the test would
behave differently for different targets (target specific maintainer
input [or action] might be helpful here), _except_ ones like hppa
which truly customize the type setting. And afaics it's just hppa
which has been making use of md_elf_symbol_type(), and which hence
had to be extended to also make use of the new hook.

Jan
Alan Modra July 9, 2019, 5:05 a.m. | #4
On Mon, Jul 08, 2019 at 06:53:01AM +0000, Jan Beulich wrote:
> On 08.07.2019 07:42, Alan Modra wrote:

> > On Wed, Jul 03, 2019 at 08:05:48AM +0000, Jan Beulich wrote:

> >> 	* config/obj-elf.c (obj_elf_type): Check for conflicts between

> >> 	old and new types.

> >> 	* config/tc-hppa.h (md_elf_symbol_type_change): New.

> >> 	* doc/as.texi: Mention warning behavior for the ELF flavor of

> >> 	.type.

> >> 	* testsuite/gas/elf/type-2.e, testsuite/gas/elf/type-2.l,

> >> 	testsuite/gas/elf/type-2.s: New.

> >> 	* testsuite/gas/elf/elf.exp: Run new test.

> > 

> > aarch64_be-linux-gnu_ilp32  +FAIL: elf type-2 list

> > aarch64-linux  +FAIL: elf type-2 list

> > nds32be-elf  +FAIL: elf type-2 list

> > nds32le-linux  +FAIL: elf type-2 list

> > rl78-elf  +FAIL: elf type-2 list

> > 

> > Plus a large number of mips-sgi-irix6 tests, all due to "symbol ..

> > already has its type set" warnings.

> 

> Well, I can see about looking into these, but not before some time

> next week. Until then all I can suggest is for someone to revert

> the commit if the failures need to be taken care of quickly.


I don't think there is any hurry, but this time I'll fix the fails,
particularly since they involve some MIPS tidies.

> I have

> to admit though that it is quite unobvious to me why the test would

> behave differently for different targets


Exactly why general ELF changes need to be tested on multiple targets.
Lots of things are not obvious.  (And I hope I haven't tripped over
some irix peculiarity with this patch..)

----
git commit f2d4ba38f5 caused many failures for mips-sgi-irix targets,
and added a new test that failed for aarch64, nds32, and rl78.
The mips failures are due to BSF_OBJECT being set in many cases for
symbols by the mips .global/.globl directive.  This patch removes that
code and instead sets BSF_OBJECT in a target frob_symbol function,
also moving the mips hacks in elf_frob_symbol to the new function.

Note that common symbols are handled fine in elf.c:swap_out_syms
without needing to set BSF_OBJECT, so that old code can disappear.

	* config/obj-elf.c (elf_frob_symbol): Remove mips hacks.
	* config/tc-mips.h (tc_frob_symbol): Define.
	(mips_frob_symbol): Declare.
	* config/tc-mips.c (s_mips_globl): Don't set BSF_OBJECT for irix.
	(mips_frob_symbol): Fudge symbols for irix here.
	* testsuite/gas/elf/type-2.e: Allow random target symbols.

diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
index 224d4c29e6..af35feeec2 100644
--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -2369,23 +2369,6 @@ elf_frob_symbol (symbolS *symp, int *puntp)
 	as_bad (_("symbol `%s' can not be both weak and common"),
 		S_GET_NAME (symp));
     }
-
-#ifdef TC_MIPS
-  /* The Irix 5 and 6 assemblers set the type of any common symbol and
-     any undefined non-function symbol to STT_OBJECT.  We try to be
-     compatible, since newer Irix 5 and 6 linkers care.  However, we
-     only set undefined symbols to be STT_OBJECT if we are on Irix,
-     because that is the only time gcc will generate the necessary
-     .global directives to mark functions.  */
-
-  if (S_IS_COMMON (symp))
-    symbol_get_bfdsym (symp)->flags |= BSF_OBJECT;
-
-  if (strstr (TARGET_OS, "irix") != NULL
-      && ! S_IS_DEFINED (symp)
-      && (symbol_get_bfdsym (symp)->flags & BSF_FUNCTION) == 0)
-    symbol_get_bfdsym (symp)->flags |= BSF_OBJECT;
-#endif
 }
 
 struct group_list
diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
index 671d74aab7..b7b4b6989a 100644
--- a/gas/config/tc-mips.c
+++ b/gas/config/tc-mips.c
@@ -16461,7 +16461,6 @@ s_mips_globl (int x ATTRIBUTE_UNUSED)
   char *name;
   int c;
   symbolS *symbolP;
-  flagword flag;
 
   do
     {
@@ -16472,14 +16471,6 @@ s_mips_globl (int x ATTRIBUTE_UNUSED)
       *input_line_pointer = c;
       SKIP_WHITESPACE_AFTER_NAME ();
 
-#ifdef TE_IRIX
-      /* On Irix 5, every global symbol that is not explicitly labelled as
-         being a function is apparently labelled as being an object.  */
-      flag = BSF_OBJECT;
-#else
-      flag = BSF_NO_FLAGS;
-#endif
-
       if (!is_end_of_line[(unsigned char) *input_line_pointer]
 	  && (*input_line_pointer != ','))
 	{
@@ -16493,11 +16484,9 @@ s_mips_globl (int x ATTRIBUTE_UNUSED)
 	  (void) restore_line_pointer (c);
 
 	  if (sec != NULL && (sec->flags & SEC_CODE) != 0)
-	    flag = BSF_FUNCTION;
+	    symbol_get_bfdsym (symbolP)->flags |= BSF_FUNCTION;
 	}
 
-      symbol_get_bfdsym (symbolP)->flags |= flag;
-
       c = *input_line_pointer;
       if (c == ',')
 	{
@@ -16512,6 +16501,23 @@ s_mips_globl (int x ATTRIBUTE_UNUSED)
   demand_empty_rest_of_line ();
 }
 
+#ifdef TE_IRIX
+/* The Irix 5 and 6 assemblers set the type of any common symbol and
+   any undefined non-function symbol to STT_OBJECT.  We try to be
+   compatible, since newer Irix 5 and 6 linkers care.  */
+
+void
+mips_frob_symbol (symbolS *symp ATTRIBUTE_UNUSED)
+{
+  /* This late in assembly we can set BSF_OBJECT indiscriminately
+     and let elf.c:swap_out_syms sort out the symbol type.  */
+  flagword *flags = &symbol_get_bfdsym (symp)->flags;
+  if ((*flags & (BSF_GLOBAL | BSF_WEAK)) != 0
+      || !S_IS_DEFINED (symp))
+    *flags |= BSF_OBJECT;
+}
+#endif
+
 static void
 s_option (int x ATTRIBUTE_UNUSED)
 {
diff --git a/gas/config/tc-mips.h b/gas/config/tc-mips.h
index 615269dd37..a928c8a49c 100644
--- a/gas/config/tc-mips.h
+++ b/gas/config/tc-mips.h
@@ -126,6 +126,11 @@ extern void mips_frob_file (void);
 extern void mips_frob_file_after_relocs (void);
 #endif
 
+#ifdef TE_IRIX
+#define tc_frob_symbol(sym, punt) mips_frob_symbol (sym)
+extern void mips_frob_symbol (symbolS *);
+#endif
+
 #define tc_fix_adjustable(fixp) mips_fix_adjustable (fixp)
 extern int mips_fix_adjustable (struct fix *);
 
diff --git a/gas/testsuite/gas/elf/type-2.e b/gas/testsuite/gas/elf/type-2.e
index 684727ccf4..57f21068cc 100644
--- a/gas/testsuite/gas/elf/type-2.e
+++ b/gas/testsuite/gas/elf/type-2.e
@@ -1,10 +1,20 @@
  +.+: 0+0 +0 +NOTYPE +LOCAL +DEFAULT +UND *
+#...
  +.+: 0+0 +0 +OBJECT +LOCAL +DEFAULT +. test1
+#...
  +.+: 0+1 +0 +FUNC +LOCAL +DEFAULT +. test2
+#...
  +.+: 0+2 +0 +NOTYPE +LOCAL +DEFAULT +. test3
+#...
  +.+: 0+3 +0 +NOTYPE +LOCAL +DEFAULT +. test4
+#...
  +.+: 0+4 +0 +NOTYPE +LOCAL +DEFAULT +. test5
+#...
  +.+: 0+5 +0 +NOTYPE +LOCAL +DEFAULT +. test6
+#...
  +.+: 0+6 +0 +OBJECT +LOCAL +DEFAULT +. test7
+#...
  +.+: 0+7 +0 +TLS +LOCAL +DEFAULT +. test8
+#...
  +.+: 0+8 +0 +IFUNC +LOCAL +DEFAULT +. test9
+#pass

-- 
Alan Modra
Australia Development Lab, IBM
Jan Beulich July 9, 2019, 11:41 a.m. | #5
On 09.07.2019 07:05, Alan Modra wrote:
> On Mon, Jul 08, 2019 at 06:53:01AM +0000, Jan Beulich wrote:

>> Well, I can see about looking into these, but not before some time

>> next week. Until then all I can suggest is for someone to revert

>> the commit if the failures need to be taken care of quickly.

> 

> I don't think there is any hurry, but this time I'll fix the fails,

> particularly since they involve some MIPS tidies.


Thanks a lot, Alan! I wouldn't have know what to do about those
MIPS issues anyway, i.e. if I had to be dealing with this, I guess
I wouldn't have come farther than ...

> --- a/gas/testsuite/gas/elf/type-2.e

> +++ b/gas/testsuite/gas/elf/type-2.e

> @@ -1,10 +1,20 @@

>    +.+: 0+0 +0 +NOTYPE +LOCAL +DEFAULT +UND *

> +#...

>    +.+: 0+0 +0 +OBJECT +LOCAL +DEFAULT +. test1

> +#...

>    +.+: 0+1 +0 +FUNC +LOCAL +DEFAULT +. test2

> +#...

>    +.+: 0+2 +0 +NOTYPE +LOCAL +DEFAULT +. test3

> +#...

>    +.+: 0+3 +0 +NOTYPE +LOCAL +DEFAULT +. test4

> +#...

>    +.+: 0+4 +0 +NOTYPE +LOCAL +DEFAULT +. test5

> +#...

>    +.+: 0+5 +0 +NOTYPE +LOCAL +DEFAULT +. test6

> +#...

>    +.+: 0+6 +0 +OBJECT +LOCAL +DEFAULT +. test7

> +#...

>    +.+: 0+7 +0 +TLS +LOCAL +DEFAULT +. test8

> +#...

>    +.+: 0+8 +0 +IFUNC +LOCAL +DEFAULT +. test9

> +#pass


... just this part.

And yes, I'm aware that I should have done even wider testing.
I sort of hoped that by having had to fiddle with (and hence
test) hppa (alongside x86) I'd be good enough. I'll know better
next time, apologies.

Jan
Maciej W. Rozycki July 9, 2019, 12:30 p.m. | #6
On Tue, 9 Jul 2019, Jan Beulich wrote:

> And yes, I'm aware that I should have done even wider testing.

> I sort of hoped that by having had to fiddle with (and hence

> test) hppa (alongside x86) I'd be good enough. I'll know better

> next time, apologies.


 FWIW running regression testing with reasonably recent hardware across 
(most of) our target base takes machine processing in the order of maybe 1 
hour in terms of elapsed time and does not require any additional tools 
beyond a native toolchain, so I think it is worth making it a regular 
habit when making any changes to binutils outside strictly target-specific 
code.

 Then if there are obscure target-specific issues you cannot figure out 
what to do about, you can ask for assistance at the mailing list before a 
change is committed that breaks things.  Not breaking things is important 
for bisecting other problems.

 Alan, thanks for stepping in and taking care of this time; my time is 
limited these days for any MIPS maintenance (and I had a network outage 
from yesterday till shortly before now too).

  Maciej

Patch

--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -2069,7 +2069,38 @@  obj_elf_type (int ignore ATTRIBUTE_UNUSE
    if (*input_line_pointer == '"')
      ++input_line_pointer;
  
-  elfsym->symbol.flags |= type;
+#ifdef md_elf_symbol_type_change
+  if (!md_elf_symbol_type_change (sym, elfsym, type))
+#endif
+    {
+      flagword mask = BSF_FUNCTION | BSF_OBJECT;
+
+      if (type != BSF_FUNCTION)
+	mask |= BSF_GNU_INDIRECT_FUNCTION;
+      if (type != BSF_OBJECT)
+	{
+	  mask |= BSF_GNU_UNIQUE | BSF_THREAD_LOCAL;
+
+	  if (S_IS_COMMON (sym))
+	    {
+	      as_bad (_("cannot change type of common symbol '%s'"),
+		      S_GET_NAME (sym));
+	      mask = type = 0;
+	    }
+	}
+
+      /* Don't warn when changing to STT_NOTYPE.  */
+      if (type)
+	{
+	  flagword new = (elfsym->symbol.flags & ~mask) | type;
+
+	  if (new != (elfsym->symbol.flags | type))
+	    as_warn (_("symbol '%s' already has its type set"), S_GET_NAME (sym));
+	  elfsym->symbol.flags = new;
+	}
+      else
+	elfsym->symbol.flags &= ~mask;
+    }
  
    demand_empty_rest_of_line ();
  }
--- a/gas/config/tc-hppa.h
+++ b/gas/config/tc-hppa.h
@@ -177,6 +177,14 @@  int hppa_fix_adjustable (struct fix *);
         ), BSF_FUNCTION)							\
     : -1)
  
+/* Handle type change from .type pseudo: Zap STT_PARISC_MILLI when
+   switching to a non-function type.  */
+#define md_elf_symbol_type_change(sym, elf, type)			\
+  ((type) != BSF_FUNCTION						\
+   && (((elf)->internal_elf_sym.st_info = 				\
+	ELF_ST_INFO (ELF_ST_BIND ((elf)->internal_elf_sym.st_info),	\
+		     STT_NOTYPE)), 0))
+
  #define tc_frob_symbol(sym,punt) \
    { \
      if ((S_GET_SEGMENT (sym) == bfd_und_section_ptr \
--- a/gas/doc/as.texi
+++ b/gas/doc/as.texi
@@ -7215,6 +7215,10 @@  systems).
  
  @end table
  
+Changing between incompatible types other than from/to STT_NOTYPE will
+result in a diagnostic.  An intermediate change to STT_NOTYPE will silence
+this.
+
  Note: Some targets support extra types in addition to those listed above.
  
  @end ifset
--- a/gas/testsuite/gas/elf/elf.exp
+++ b/gas/testsuite/gas/elf/elf.exp
@@ -230,6 +230,7 @@  if { [is_elf_format] } then {
      } else {
  	run_dump_test ifunc-1
  	run_elf_list_test "type" "" "" "-s" "| grep \"1 *\\\[FIONTCU\\\]\""
+	run_elf_list_test "type-2" "" "--warn" "-s" "| grep \"0 *\\\[FIONT\\\]\""
      }
  
      run_dump_test "section6"
--- /dev/null
+++ b/gas/testsuite/gas/elf/type-2.e
@@ -0,0 +1,10 @@ 
+ +.+: 0+0 +0 +NOTYPE +LOCAL +DEFAULT +UND *
+ +.+: 0+0 +0 +OBJECT +LOCAL +DEFAULT +. test1
+ +.+: 0+1 +0 +FUNC +LOCAL +DEFAULT +. test2
+ +.+: 0+2 +0 +NOTYPE +LOCAL +DEFAULT +. test3
+ +.+: 0+3 +0 +NOTYPE +LOCAL +DEFAULT +. test4
+ +.+: 0+4 +0 +NOTYPE +LOCAL +DEFAULT +. test5
+ +.+: 0+5 +0 +NOTYPE +LOCAL +DEFAULT +. test6
+ +.+: 0+6 +0 +OBJECT +LOCAL +DEFAULT +. test7
+ +.+: 0+7 +0 +TLS +LOCAL +DEFAULT +. test8
+ +.+: 0+8 +0 +IFUNC +LOCAL +DEFAULT +. test9
--- /dev/null
+++ b/gas/testsuite/gas/elf/type-2.l
@@ -0,0 +1,3 @@ 
+.*: Assembler messages:
+.*:4: Warning: .*
+.*:9: Warning: .*
--- /dev/null
+++ b/gas/testsuite/gas/elf/type-2.s
@@ -0,0 +1,49 @@ 
+	.text
+
+	.type	test1,%function
+	.type	test1,%object
+test1:
+	.byte	0x0
+
+	.type	test2,%object
+	.type	test2,%function
+test2:
+	.byte	0x0
+
+	.type	test3,%object
+	.type	test3,%notype
+test3:
+	.byte	0x0
+
+	.type	test4,%function
+	.type	test4,%notype
+test4:
+	.byte	0x0
+
+	.type	test5,%tls_object
+	.type	test5,%notype
+test5:
+	.byte	0x0
+
+	.type	test6,%gnu_indirect_function
+	.type	test6,%notype
+test6:
+	.byte	0x0
+
+	.type	test7,%function
+	.type	test7,%notype
+	.type	test7,%object
+test7:
+	.byte	0x0
+
+	.type	test8,%object
+	.type	test8,%tls_object
+	.type	test8,%object
+test8:
+	.byte	0x0
+
+	.type	test9,%function
+	.type	test9,%gnu_indirect_function
+	.type	test9,%function
+test9:
+	.byte	0x0
--- a/ld/testsuite/ld-elf/group9.s
+++ b/ld/testsuite/ld-elf/group9.s
@@ -5,7 +5,7 @@  foo:
  	.byte 0
  	.section	.data.foo,"axG",%progbits,foo,comdat
  	.globl foo.data
-	.type	foo,%object
+	.type	foo.data,%object
  foo.data:
  	.byte 0
  	.section	.text.bar,"axG",%progbits,bar,comdat