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

Message ID 3202e357-bc24-6119-9883-6ee7d85d20f6@suse.com
State New
Headers show
Series
  • [RFC] gas/ELF: don't accumulate .type settings
Related show

Commit Message

Jan Beulich June 28, 2019, 1:09 p.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.

Questions:

Is a diagnostic needed (perhaps unless changing to @notype)?

What to do with @common? It getting set involves more than just setting
STT_OBJECT. Should type changes on common symbols be disallowed?

Is anyone aware of any other caveats here?

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

gas/
2019-06-28  Jan Beulich  <jbeulich@suse.com>

	* config/obj-elf.c (obj_elf_type): New local variable "mask".
	Change "type" to flagword. Determine what flags to remove before
	setting the new ones.
	* testsuite/gas/elf/type-2.e,
	testsuite/gas/elf/type-2.s: New.
	* testsuite/gas/elf/elf.exp: Run new test.

Comments

Nick Clifton July 1, 2019, 12:48 p.m. | #1
Hi Jan,

> Make the behavior predictable:


I think that this would be a great idea.

> Is a diagnostic needed (perhaps unless changing to @notype)?


Yes.  If the type changes are significant then the user should
be informed.  I also like the idea of not warning on changes to
@notype.  Presumably this is to allow a deliberate change of 
type to happen.  We should document this feature however.


> What to do with @common? It getting set involves more than just setting

> STT_OBJECT. Should type changes on common symbols be disallowed?


Hmm, I am kind of leaning towards allowing changes from common to
something else, but not the other way around.  (Assuming that the
new object is at least the same size as the common object).  But
given what you have said, and having a look at the code myself, I
agree that it might be safer just to disallow any changes involving
common symbols.


> Is anyone aware of any other caveats here?


No - but one thing I have wanted to add to the assembler for some
time now is the ability to set the type to an arbitrary value.  
Possibly restricted to the range of STT_LOOS .. STT_HIPROC.  I have
tried a couple of times, but could not find a clean solution.  I am
mentioning it now just as a suggestion that maybe the masking code
you are adding should skip symbols with target specific semantics.

Cheers
  Nick
Jan Beulich July 1, 2019, 2:26 p.m. | #2
Hi Nick,

On 01.07.2019 14:48, Nick Clifton wrote:
>> Make the behavior predictable:

> 

> I think that this would be a great idea.

> 

>> Is a diagnostic needed (perhaps unless changing to @notype)?

> 

> Yes.  If the type changes are significant then the user should

> be informed.  I also like the idea of not warning on changes to

> @notype.  Presumably this is to allow a deliberate change of

> type to happen.


Yes.

>  We should document this feature however.


Will do.

>> What to do with @common? It getting set involves more than just setting

>> STT_OBJECT. Should type changes on common symbols be disallowed?

> 

> Hmm, I am kind of leaning towards allowing changes from common to

> something else, but not the other way around.  (Assuming that the

> new object is at least the same size as the common object).  But

> given what you have said, and having a look at the code myself, I

> agree that it might be safer just to disallow any changes involving

> common symbols.

> 

> 

>> Is anyone aware of any other caveats here?

> 

> No - but one thing I have wanted to add to the assembler for some

> time now is the ability to set the type to an arbitrary value.

> Possibly restricted to the range of STT_LOOS .. STT_HIPROC.  I have

> tried a couple of times, but could not find a clean solution.  I am

> mentioning it now just as a suggestion that maybe the masking code

> you are adding should skip symbols with target specific semantics.


Hmm, and how would I recognize "target specific semantics" in
obj_elf_type()? I'm afraid I'm not familiar enough with the BFD
internals ...

Jan
Nick Clifton July 2, 2019, 9:59 a.m. | #3
Hi Jan,

>> No - but one thing I have wanted to add to the assembler for some

>> time now is the ability to set the type to an arbitrary value.

>> Possibly restricted to the range of STT_LOOS .. STT_HIPROC.  I have

>> tried a couple of times, but could not find a clean solution.  I am

>> mentioning it now just as a suggestion that maybe the masking code

>> you are adding should skip symbols with target specific semantics.

> 

> Hmm, and how would I recognize "target specific semantics" in

> obj_elf_type()? I'm afraid I'm not familiar enough with the BFD

> internals ...


Well yes, that is the problem.  There is no direct 1:1 mapping between 
ELF symbol types and bfd symbol flags, which is why I have been having
problems adding the new types.

I think that the best solution here would be to include a new assembler
backend function, eg "md_elf_symbol_type_change(old,new)", which backends
could define if they need to determine for themselves whether a type
change is OK.  If the function is defined and returns say "TRUE", then
your new code would not complain about the type change, otherwise it
would carry on and perform its other checks before issuing a warning
message.

By default of course the macro would be undefined, but if it turns out
later on that certain targets use special symbol types then they can
define the macro and do what they need to do.

Cheers
  Nick

Patch

--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -1962,7 +1962,7 @@  static void
  obj_elf_type (int ignore ATTRIBUTE_UNUSED)
  {
    char c;
-  int type;
+  flagword type, mask;
    const char *type_name;
    symbolS *sym;
    elf_symbol_type *elfsym;
@@ -2069,6 +2069,12 @@  obj_elf_type (int ignore ATTRIBUTE_UNUSE
    if (*input_line_pointer == '"')
      ++input_line_pointer;
  
+  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;
+  elfsym->symbol.flags &= ~mask;
    elfsym->symbol.flags |= type;
  
    demand_empty_rest_of_line ();
--- 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" "" "" "-s" "| grep \"0 *\\\[FIONT\\\]\""
      }
  
      run_dump_test "section6"
--- /dev/null
+++ b/gas/testsuite/gas/elf/type-2.e
@@ -0,0 +1,9 @@ 
+ +.+: 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 +TLS +LOCAL +DEFAULT +. test7
+ +.+: 0+7 +0 +IFUNC +LOCAL +DEFAULT +. test8
--- /dev/null
+++ b/gas/testsuite/gas/elf/type-2.s
@@ -0,0 +1,41 @@ 
+	.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,%tls_object
+        .type   test7,%object
+test7:
+	.byte	0x0
+
+        .type   test8,%gnu_indirect_function
+        .type   test8,%function
+test8:
+	.byte	0x0