__start/__stop symbols not always defined

[Michael Matz <matz at suse dot de>]

Hi,

so, some followup from last years fiddling with __start/__stop symbols.  
(Also see 
http://lists.gnu.org/archive/html/bug-binutils/2017-08/msg00195.html , as 
the respective bug report fell right into the bugzilla crash and database 
loss).

Pacemakers logging facility is unfortunately still not happy with how we 
solved this now: while __start/__stop symbols are nicely exported 
(protected vis) when they are created, they aren't always created when 
they were created before.

Before binutils 2.29 those symbols were created if there were 
any "references" in the linker hash table.  That meant that either a 
reference from a regular .o file, or (and that's the thing) e.g. 
a definition in some dynamic object included in the link, would cause the 
symbols to be created.

That meant that the definition of __start___verbose in libqb.so (which got 
there because of explicit references to __start__verbose in its objects) 
also caused these symbols to appear in all libraries merely linked against 
it (when they had a __verbose section), even if their objects didn't 
contain any references to them.  dlopen and dlsym are then used by the 
logging facility to actually get at the symbols and hence section content.

Now the rewrites of last year ultimately caused these symbol to go from 
default to protected visibility, but also to only create them when there 
are explicit references in any of the regular objects:

  if (h != NULL
      && (h->root.type == bfd_link_hash_undefined
          || h->root.type == bfd_link_hash_undefweak
          || (h->ref_regular && !h->def_regular)))
    {
      ... create start/stop ...

The end-result is that while libqb.so still contains __start___verbose in 
its symbol table, the depending libraries and application don't anymore.  
Hence dlsym doesn't find anything --> logging doesn't work.

The reason I didn't create a bug report for this is that I myself aren't 
totally sure if the new behaviour is better or not.  It's quite probable 
that the old behaviour was only an accident, but it meant that accessing 
the symbols dia dlsym only was possible.

It is (and was) also documented that these symbols are PROVIDEd, and hence 
only defined if referenced.

OTOH, the old behaviour also somewhat makes sense: if the user created a 
section with a C-representable name he presumably wants to access its 
content somehow, and without regular references in some .o file dlsym is 
the only possibility, which doesn't work if the symbols aren't created.

So, there's a case for always creating the __start/__stop symbols when an 
orphan section is included, even if there are no regular references.

The pacemaker guys meanwhile resort to work arounds where they use a 
linker script to always define these __start/__stop symbols.  Indeed I 
can't really see a way to somehow force references to those symbols to 
appear without either wasting code or data space (e.g.
"void *__unused_global=&__start___verbose"), which also has the issue of 
where to put these references (they must be forced from a header file 
which is included by multiple C files, so there must be more magic that 
such reference at least appears only once in the final app/library).

So, hmm, I'm somewhat leaning towards saying that we should not only 
PROVIDE these symbols but properly define them always (when not already 
defined in regular objects).  Similar to below patch.

Thoughts?


Ciao,
Michael.
diff --git a/bfd/elflink.c b/bfd/elflink.c
index e3751fa..a18587c 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -14335,10 +14335,19 @@ bfd_elf_define_start_stop (struct bfd_link_info *info,
 
   h = elf_link_hash_lookup (elf_hash_table (info), symbol,
 			    FALSE, FALSE, TRUE);
+  if (!h && symbol[0] != '.')
+    {
+      bfd_elf_record_link_assignment (info->output_bfd, info, symbol,
+				      FALSE, FALSE);
+      h = elf_link_hash_lookup (elf_hash_table (info), symbol,
+				FALSE, FALSE, TRUE);
+    }
+
   if (h != NULL
       && (h->root.type == bfd_link_hash_undefined
+	  || h->root.type == bfd_link_hash_new
 	  || h->root.type == bfd_link_hash_undefweak
-	  || (h->ref_regular && !h->def_regular)))
+	  || !h->def_regular))
     {
       h->root.type = bfd_link_hash_defined;
       h->root.u.def.section = sec;
@@ -14347,6 +14356,7 @@ bfd_elf_define_start_stop (struct bfd_link_info *info,
       h->def_dynamic = 0;
       h->start_stop = 1;
       h->u2.start_stop_section = sec;
+      bfd_elf_link_record_dynamic_symbol (info, h);
       if (symbol[0] == '.')
 	{
 	  /* .startof. and .sizeof. symbols are local.  */