[RFC] lto: keep 'force_output' on extern symbols

Hello,

We have a somewhat long-recognized problem with LTO vs. symbols referenced
in inline asm statements. For extended asms, the solution is easy: just
mention the symbol in constraints. For toplevel asms, this is trickier, as
they cannot have constraints.

I think a good solution for toplevel asms would be to give users a way to
add '__attribute__((used))' to symbols referenced in toplevel asms, e.g.:

    #include <foo.h>

    __attribute__((used))
    __typeof(foo) foo;

    asm("foo");


If 'foo' has a definition in this translation unit, this already works.
Let's make this work in LTO if 'foo' is defined in some other TU.

The only problem seems to be that the 'force_output' decl flag is reset
prior to LTO streaming just because pre-WPA that node is external. I'm
not sure it's actually necessary.

Does the following patch look reasonable? Bootstrapped/regrested on x86-64.

Thanks.
Alexander

	* ipa-visibility.c (function_and_variable_visibility): Do not reset
	node->force_output.

On Thu, Jun 14, 2018 at 1:19 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> Hello,
>
> We have a somewhat long-recognized problem with LTO vs. symbols referenced
> in inline asm statements. For extended asms, the solution is easy: just
> mention the symbol in constraints. For toplevel asms, this is trickier, as
> they cannot have constraints.
>
> I think a good solution for toplevel asms would be to give users a way to
> add '__attribute__((used))' to symbols referenced in toplevel asms, e.g.:
>
>     #include <foo.h>
>
>     __attribute__((used))
>     __typeof(foo) foo;
>
>     asm("foo");
>
>
> If 'foo' has a definition in this translation unit, this already works.
> Let's make this work in LTO if 'foo' is defined in some other TU.
>
> The only problem seems to be that the 'force_output' decl flag is reset
> prior to LTO streaming just because pre-WPA that node is external. I'm
> not sure it's actually necessary.
>
> Does the following patch look reasonable? Bootstrapped/regrested on x86-64.

Can you make sure to add a testcase?

> Thanks.
> Alexander
>
>         * ipa-visibility.c (function_and_variable_visibility): Do not reset
>         node->force_output.
>
> diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
> index a82852b3ce4..caa9d420c82 100644
> --- a/gcc/ipa-visibility.c
> +++ b/gcc/ipa-visibility.c
> @@ -672,10 +672,7 @@ function_and_variable_visibility (bool whole_program)
>          is finished.  We may end up marking as node external nodes
>          where this flag is meaningless strip it.  */
>        if (DECL_EXTERNAL (node->decl) || !node->definition)
> -       {
> -         node->force_output = 0;
> -         node->forced_by_abi = 0;
> -       }
> +       node->forced_by_abi = 0;
>
>        /* C++ FE on lack of COMDAT support create local COMDAT functions
>          (that ought to be shared but can not due to object format

On Thu, 14 Jun 2018, Richard Biener wrote:
> Can you make sure to add a testcase?

Apologies, I got a bit too excited and forgot that my local testcase had

    void *unused_ref = &foo;

which masked another issue: unreferenced declarations won't even appear
in the symtab, even if they have __attribute__((used)).

I took a shot at fixing that. The following patch makes sure the decl
appears in the symtab and is not deleted by ipa-visibility.

However, it is not added to LTO streamed-out symtab, so WPA does not see it.

If the direction looks fine overall, I'll look into getting over this
streaming hurdle (hints much appreciated).

WIP patch (untested), now with a minimal testcase:

	* c-family/c-attribs.c (handle_used_attribute): Create a symtab node.
	* cgraphunit.c (symtab_node::needed_p): Respect force_output even for
	external nodes.
	* ipa-visibility.c (function_and_variable_visibility): Do not reset
        node->force_output.
	* ipa.c (symbol_table::remove_unreachable_nodes): Do not remove nodes
	that are needed_p.
	
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 73901bdf47c..f28bb529092 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -1052,6 +1052,7 @@ handle_used_attribute (tree *pnode, tree name, tree ARG_UNUSED (args),
       DECL_PRESERVE_P (node) = 1;
       if (VAR_P (node))
 	DECL_READ_P (node) = 1;
+      symtab_node::get_create (node);
     }
   else
     {
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 04b6919be48..3931a79aa3d 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -244,16 +244,16 @@ symtab_node::needed_p (void)
 	(!DECL_ASSEMBLER_NAME_SET_P (decl)
 	 || !TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)));
 
+  /* If the user told us it is used, then it must be so.  */
+  if (force_output)
+    return true;
+
   if (!definition)
     return false;
 
   if (DECL_EXTERNAL (decl))
     return false;
 
-  /* If the user told us it is used, then it must be so.  */
-  if (force_output)
-    return true;
-
   /* ABI forced symbols are needed when they are external.  */
   if (forced_by_abi && TREE_PUBLIC (decl))
     return true;
diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
index 907dc9d0e2b..3e167567b45 100644
--- a/gcc/ipa-visibility.c
+++ b/gcc/ipa-visibility.c
@@ -668,10 +668,7 @@ function_and_variable_visibility (bool whole_program)
 	 is finished.  We may end up marking as node external nodes
 	 where this flag is meaningless strip it.  */
       if (DECL_EXTERNAL (node->decl) || !node->definition)
-	{
-	  node->force_output = 0;
-	  node->forced_by_abi = 0;
-	}
+	node->forced_by_abi = 0;
 
       /* C++ FE on lack of COMDAT support create local COMDAT functions
 	 (that ought to be shared but can not due to object format
diff --git a/gcc/ipa.c b/gcc/ipa.c
index 82fc334ec0b..8e011d00cc4 100644
--- a/gcc/ipa.c
+++ b/gcc/ipa.c
@@ -507,7 +507,7 @@ symbol_table::remove_unreachable_nodes (FILE *file)
       next = next_function (node);
 
       /* If node is not needed at all, remove it.  */
-      if (!node->aux)
+      if (!node->aux && !node->needed_p ())
 	{
 	  if (file)
 	    fprintf (file, " %s", node->dump_name ());
diff --git a/gcc/testsuite/gcc.dg/lto/tlasm_0.c b/gcc/testsuite/gcc.dg/lto/tlasm_0.c
index e69de29bb2d..ba31c01e17c 100644
--- a/gcc/testsuite/gcc.dg/lto/tlasm_0.c
+++ b/gcc/testsuite/gcc.dg/lto/tlasm_0.c
@@ -0,0 +1,19 @@
+/* { dg-lto-do link } */
+/* { dg-require-effective-target gas } */
+
+/* Test that __attribute__((used)) can be used to preserve definitions of
+ * symbols referenced in toplevel asm even when the definition is provided
+ * in another translation unit, i.e. the attribute is not lost before WPA. */
+
+__attribute__ ((used))
+void foo (void);
+
+#if __SIZEOF_POINTER__ == 64
+asm (".quad foo");
+#else
+asm (".long foo");
+#endif
+
+int main ()
+{
+}
diff --git a/gcc/testsuite/gcc.dg/lto/tlasm_1.c b/gcc/testsuite/gcc.dg/lto/tlasm_1.c
index e69de29bb2d..a4b7f5afa52 100644
--- a/gcc/testsuite/gcc.dg/lto/tlasm_1.c
+++ b/gcc/testsuite/gcc.dg/lto/tlasm_1.c
@@ -0,0 +1,3 @@
+void foo (void)
+{
+}

On Thu, 14 Jun 2018, Alexander Monakov wrote:
> However, it is not added to LTO streamed-out symtab, so WPA does not see it.
> 
> If the direction looks fine overall, I'll look into getting over this
> streaming hurdle (hints much appreciated).

Well, going down this rabbit hole is a fine exercise, but the idea seems to
require changes in too many places. There's also the problems that it can
solve only a subset of toplevel-asm-related issues, and won't gracefully work
for variables (we'd warn about ignoring the attribute on an extern decl).

So, let me withdraw this - sorry for the noise.

Alexander