[RFC] Fix for pr64706 testcase faulre

Hi,
this patch started as an attempt to fix pr64706 that kept me walking in loops
for some time (I failed to make a hack that would make the testcase work in all
basic settings of -O0/2 -flto set on the both files independently. All GCC
releases crashes on some).

The testcase has ODR violation that breaks comdat groups and kind of works with
non-lto by accident, but it triggers longer problem that we do not handle
aliases correctly.

In ELF an alias is just additional symbol at a given position in code segment,
while for GCC we do not have code segment. Function bodies are assigned to
delcarations and aliases are linked to those declarations. This makes difference
in the following testcase:

===== b.c:

__attribute__((weak))
__attribute__((noinline))

int a()
{
  return 1;
}

__attribute__((noinline))
static int b() __attribute__((alias("a")));
void
test()
{
  if (b()!=1)
    __builtin_abort ();
}

===== b.c:

__attribute__((noinline))
int a();

int a()
{
  return 2;
}
extern void test ();
int
main()
{
  if (a() != 2)
    __builtin_abort ();
  test();
  return 0;
}

Here LTO linking will replace weak symbol a() by definition from b.c and
rediect static alias b in a.c to this definition which is wrong.

This patch works towards fixing it by making lto-symtab to notice that there is
previaling alias (a.c:b in my testcase) of a prevailed symbol (a.c:a in my
testcase that got previaled by b.c:a).  In this case the body of prevailed
symbol (a.c:a) must be saved because it is still reachable by calling b.c:a.
This is done by turning the prevailed symbol to a static symbol.

This has some fallout in streaming because we need to stream in summaries of
such prevailed symbols which is not hard to do and is purpose of
stream_in_summary_p.

Now however we have a problem if tree sharing decided to share delcaration of
a.c:a and b.c:a (which it does in my testcase) because we have one decl
and two bodies.  I use the code form profile merging to patch around but this
code is not fully correct (it works in profile merging only because the
body is discarded immediatly after).

The code creates new static decl, say a.static and moves the body of a.c to it.
This however breaks because when streaming in body of a.static we will prevail
all references to a.c:a to b.c:a.  This is OK, for example when streaming in
statement calling a(). It is wrong when streaming in _CONTEXT pointers.

So in turn my patch mostly works, but sometimes I get errors like
/aux/hubicka/trunk-git/gcc/testsuite/gcc.dg/lto/alias-resolution-1_0.c:9:5: error: label context is not the current function declaration

I do not see way around this with our current infrastructure.  One way would be
to read body of a.c:a and clone it to a.static. This is bit wasteful but it is
also problem in case body of b.c:a is already read to memory.

I would like to do this w/o need to stream in the body, because such situation
happens quite commonly local aliases: Until we are able to prove that all
referencs to a local aliase of a symbol are gone we need to assume that body
needs to be preserved and those are potentially quite frequent.

So basically only solution I think of is to make lto-stream-in live with the
fact that function decl was changed.  So we can stream original decl to the
beggining of the function section and during stream in do rewritting of
DECL_CONTEXT similar way as tree-inline would do.  We need to be careful about
keeping debug info in shape too (we are not producing inline copy, we are
merely unsharing the main decl).

What do you think?

Note that I was considering case of simply not sharing decls with
aliases at stream time.  This does not work because by localizing the
decl one will not redirect references to the symbol correctly.

Other option would be to always produce a static symbol for every
symbol with an alias.  This again can be IMO only done by cloning it and
would be relatively expensive because at the stream out time we do have
considerable quantity of aliases especially with -fpic.

Honza

gcc/ChangeLog:

2020-03-26  Jan Hubicka  <hubicka@ucw.cz>

	* cgraph.c (cgraph_node::make_local): Add force parameter.
	* cgraph.h (symtab_node::clone_referring_skip_aliases): New
	member function.
	(symtab_node::stream_in_summary_p): Likewise.
	* ipa-fnsummary.c (inline_read_section): Use
	symtab_node::stream_in_summary_p.
	* ipa-prop.c (ipa_read_node_info): Likewise.
	* symtab.c (symtab_node::clone_referring): Fix formating.
	(symtab_node::clone_referring_skip_aliases): New member function.
	(symtab_node::dump_referring): Fix formating
	(symtab_node::stream_in_summary_p): New member function.

gcc/lto/ChangeLog:

2020-03-26  Jan Hubicka  <hubicka@ucw.cz>

	* lto-symtab.c (lto_symtab_symbol_p): New function.
	(has_prevailing_alias_p): New function.
	(lto_symtab_replace_node): Break out from ...; support alias
	interposition
	(lto_cgraph_replace_node): ... here.
	(lto_varpool_replace_node): Use the common code.

gcc/testsuite/ChangeLog:

2020-03-26  Jan Hubicka  <hubicka@ucw.cz>

	* gcc.dg/lto/alias-resolution-1_0.c: New test.
	* gcc.dg/lto/alias-resolution-1_1.c: New test.

On Thu, 26 Mar 2020, Jan Hubicka wrote:

> Hi,
> this patch started as an attempt to fix pr64706 that kept me walking in loops
> for some time (I failed to make a hack that would make the testcase work in all
> basic settings of -O0/2 -flto set on the both files independently. All GCC
> releases crashes on some).
> 
> The testcase has ODR violation that breaks comdat groups and kind of works with
> non-lto by accident, but it triggers longer problem that we do not handle
> aliases correctly.
> 
> In ELF an alias is just additional symbol at a given position in code segment,
> while for GCC we do not have code segment. Function bodies are assigned to
> delcarations and aliases are linked to those declarations. This makes difference
> in the following testcase:
> 
> ===== b.c:
> 
> __attribute__((weak))
> __attribute__((noinline))
> 
> int a()
> {
>   return 1;
> }
> 
> __attribute__((noinline))
> static int b() __attribute__((alias("a")));
> void
> test()
> {
>   if (b()!=1)
>     __builtin_abort ();
> }
> 
> ===== b.c:
> 
> __attribute__((noinline))
> int a();
> 
> int a()
> {
>   return 2;
> }
> extern void test ();
> int
> main()
> {
>   if (a() != 2)
>     __builtin_abort ();
>   test();
>   return 0;
> }
> 
> Here LTO linking will replace weak symbol a() by definition from b.c and
> rediect static alias b in a.c to this definition which is wrong.

Is it?  What does a non-weak local alias to a weak function mean?

> This patch works towards fixing it by making lto-symtab to notice that there is
> previaling alias (a.c:b in my testcase) of a prevailed symbol (a.c:a in my
> testcase that got previaled by b.c:a).  In this case the body of prevailed
> symbol (a.c:a) must be saved because it is still reachable by calling b.c:a.
> This is done by turning the prevailed symbol to a static symbol.
> 
> This has some fallout in streaming because we need to stream in summaries of
> such prevailed symbols which is not hard to do and is purpose of
> stream_in_summary_p.
> 
> Now however we have a problem if tree sharing decided to share delcaration of
> a.c:a and b.c:a (which it does in my testcase) because we have one decl
> and two bodies.  I use the code form profile merging to patch around but this
> code is not fully correct (it works in profile merging only because the
> body is discarded immediatly after).
> 
> The code creates new static decl, say a.static and moves the body of a.c to it.
> This however breaks because when streaming in body of a.static we will prevail
> all references to a.c:a to b.c:a.  This is OK, for example when streaming in
> statement calling a(). It is wrong when streaming in _CONTEXT pointers.
> 
> So in turn my patch mostly works, but sometimes I get errors like
> /aux/hubicka/trunk-git/gcc/testsuite/gcc.dg/lto/alias-resolution-1_0.c:9:5: error: label context is not the current function declaration
> 
> I do not see way around this with our current infrastructure.  One way would be
> to read body of a.c:a and clone it to a.static. This is bit wasteful but it is
> also problem in case body of b.c:a is already read to memory.

Why don't we represent the alias at the cgraph level?  That is,
why do decls come into play at all here?  b prevails and it has a
reference to a.c:a so we need to keep (and emit) that.  The only issue
would be that we'd end up with two 'a's so we have to notice that
and fixup somehow.

Isn't this in the end a similar issue as with extern inline bodies
vs. the offline copy body?

> I would like to do this w/o need to stream in the body, because such situation
> happens quite commonly local aliases: Until we are able to prove that all
> referencs to a local aliase of a symbol are gone we need to assume that body
> needs to be preserved and those are potentially quite frequent.
> 
> So basically only solution I think of is to make lto-stream-in live with the
> fact that function decl was changed.  So we can stream original decl to the
> beggining of the function section and during stream in do rewritting of
> DECL_CONTEXT similar way as tree-inline would do.  We need to be careful about
> keeping debug info in shape too (we are not producing inline copy, we are
> merely unsharing the main decl).
> 
> What do you think?

I think the not prevailed but still needed cgraph node for 'a' should
have its decl cleared ;)

> Note that I was considering case of simply not sharing decls with
> aliases at stream time.  This does not work because by localizing the
> decl one will not redirect references to the symbol correctly.

But all remaining references should be via 'b', not via 'a'.

> Other option would be to always produce a static symbol for every
> symbol with an alias.  This again can be IMO only done by cloning it and
> would be relatively expensive because at the stream out time we do have
> considerable quantity of aliases especially with -fpic.
> 
> Honza
> 
> gcc/ChangeLog:
> 
> 2020-03-26  Jan Hubicka  <hubicka@ucw.cz>
> 
> 	* cgraph.c (cgraph_node::make_local): Add force parameter.
> 	* cgraph.h (symtab_node::clone_referring_skip_aliases): New
> 	member function.
> 	(symtab_node::stream_in_summary_p): Likewise.
> 	* ipa-fnsummary.c (inline_read_section): Use
> 	symtab_node::stream_in_summary_p.
> 	* ipa-prop.c (ipa_read_node_info): Likewise.
> 	* symtab.c (symtab_node::clone_referring): Fix formating.
> 	(symtab_node::clone_referring_skip_aliases): New member function.
> 	(symtab_node::dump_referring): Fix formating
> 	(symtab_node::stream_in_summary_p): New member function.
> 
> gcc/lto/ChangeLog:
> 
> 2020-03-26  Jan Hubicka  <hubicka@ucw.cz>
> 
> 	* lto-symtab.c (lto_symtab_symbol_p): New function.
> 	(has_prevailing_alias_p): New function.
> 	(lto_symtab_replace_node): Break out from ...; support alias
> 	interposition
> 	(lto_cgraph_replace_node): ... here.
> 	(lto_varpool_replace_node): Use the common code.
> 
> gcc/testsuite/ChangeLog:
> 
> 2020-03-26  Jan Hubicka  <hubicka@ucw.cz>
> 
> 	* gcc.dg/lto/alias-resolution-1_0.c: New test.
> 	* gcc.dg/lto/alias-resolution-1_1.c: New test.
> 
> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> index 6b780f80eb3..1f1319ed5fe 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -2465,9 +2465,10 @@ cgraph_node::call_for_symbol_thunks_and_aliases (bool (*callback)
>  /* Worker to bring NODE local.  */
>  
>  bool
> -cgraph_node::make_local (cgraph_node *node, void *)
> +cgraph_node::make_local (cgraph_node *node, void *force)
>  {
> -  gcc_checking_assert (node->can_be_local_p ());
> +  if (!force)
> +    gcc_checking_assert (node->can_be_local_p ());
>    if (DECL_COMDAT (node->decl) || DECL_EXTERNAL (node->decl))
>      {
>        node->make_decl_local ();
> @@ -2486,12 +2487,14 @@ cgraph_node::make_local (cgraph_node *node, void *)
>    return false;
>  }
>  
> -/* Bring cgraph node local.  */
> +/* Bring cgraph node local.  If FORCE is true skip checking if symbol can
> +   become local.  */
>  
>  void
> -cgraph_node::make_local (void)
> +cgraph_node::make_local (bool force)
>  {
> -  call_for_symbol_thunks_and_aliases (cgraph_node::make_local, NULL, true);
> +  call_for_symbol_thunks_and_aliases (cgraph_node::make_local,
> +				      (void *)(size_t)force, true);
>  }
>  
>  /* Worker to set nothrow flag.  */
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index 43de3b4a8ac..983cbfe8cd8 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -189,6 +189,9 @@ public:
>       with callgraph edges associated with them.  */
>    void clone_referring (symtab_node *node);
>  
> +  /* Same as clone_referring but skip aliases.  */
> +  void clone_referring_skip_aliases (symtab_node *node);
> +
>    /* Clone reference REF to this symtab_node and set its stmt to STMT.  */
>    ipa_ref *clone_reference (ipa_ref *ref, gimple *stmt);
>  
> @@ -340,6 +343,9 @@ public:
>       in question prevails in the linking to save some memory usage.  */
>    bool prevailing_p (void);
>  
> +  /* Return true if the summary should be streamed in.  */
> +  bool stream_in_summary_p (void);
> +
>    /* Return true if NODE binds to current definition in final executable
>       when referenced from REF.  If REF is NULL return conservative value
>       for any reference.  */
> @@ -1163,8 +1169,9 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
>    /* cgraph_node is no longer nested function; update cgraph accordingly.  */
>    void unnest (void);
>  
> -  /* Bring cgraph node local.  */
> -  void make_local (void);
> +  /* Bring cgraph node local.  If FORCE is true skip checking if symbol can
> +     be local.  */
> +  void make_local (bool force = false);
>  
>    /* Likewise indicate that a node is having address taken.  */
>    void mark_address_taken (void);
> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
> index b411bc4d660..a79e75dd001 100644
> --- a/gcc/ipa-fnsummary.c
> +++ b/gcc/ipa-fnsummary.c
> @@ -4176,10 +4176,10 @@ inline_read_section (struct lto_file_decl_data *file_data, const char *data,
>        encoder = file_data->symtab_node_encoder;
>        node = dyn_cast<cgraph_node *> (lto_symtab_encoder_deref (encoder,
>  								index));
> -      info = node->prevailing_p () ? ipa_fn_summaries->get_create (node) : NULL;
> -      params_summary = node->prevailing_p () ? IPA_NODE_REF (node) : NULL;
> -      size_info = node->prevailing_p ()
> -		  ? ipa_size_summaries->get_create (node) : NULL;
> +      bool stream_in = node->stream_in_summary_p ();
> +      info = stream_in ? ipa_fn_summaries->get_create (node) : NULL;
> +      params_summary = stream_in ? IPA_NODE_REF (node) : NULL;
> +      size_info = stream_in ? ipa_size_summaries->get_create (node) : NULL;
>  
>        int stack_size = streamer_read_uhwi (&ib);
>        int size = streamer_read_uhwi (&ib);
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index a77130ded39..4035d903d33 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -4937,7 +4937,7 @@ ipa_read_node_info (class lto_input_block *ib, struct cgraph_node *node,
>    int k;
>    struct cgraph_edge *e;
>    struct bitpack_d bp;
> -  bool prevails = node->prevailing_p ();
> +  bool prevails = node->stream_in_summary_p ();
>    class ipa_node_params *info = prevails
>  				? IPA_NODE_REF_GET_CREATE (node) : NULL;
>  
> diff --git a/gcc/lto/lto-symtab.c b/gcc/lto/lto-symtab.c
> index d5e38beb4b6..3f4ece360d7 100644
> --- a/gcc/lto/lto-symtab.c
> +++ b/gcc/lto/lto-symtab.c
> @@ -36,6 +36,150 @@ along with GCC; see the file COPYING3.  If not see
>  #include "stringpool.h"
>  #include "attribs.h"
>  
> +/* Return true, if the symbol E should be resolved by lto-symtab.
> +   Those are all external symbols and all real symbols that are not static (we
> +   handle renaming of static later in partitioning).  */
> +
> +static bool
> +lto_symtab_symbol_p (symtab_node *e)
> +{
> +  if (!TREE_PUBLIC (e->decl) && !DECL_EXTERNAL (e->decl))
> +    return false;
> +  return e->real_symbol_p ();
> +}
> +
> +/* Return ture if NODE is an prevailing alias or has alias that prevalis.  */
> +
> +static bool
> +has_prevailing_alias_p (struct symtab_node *node)
> +{
> +  ipa_ref *ref;
> +
> +  FOR_EACH_ALIAS (node, ref)
> +    {
> +      symtab_node *alias = ref->referring;
> +
> +      if (lto_symtab_symbol_p (alias))
> +	return alias->prevailing_p ();
> +      else
> +	return true;
> +      if (has_prevailing_alias_p (alias))
> +	return true;
> +    }
> +  return false;
> +}
> +
> +/* Common part of lto_cgraph_replace_node and lto_varpool_replace_node
> +   Retun true if NODE can be removed.  */
> +
> +static bool
> +lto_symtab_replace_node (struct symtab_node *node,
> +			 struct symtab_node *prevailing_node)
> +{
> +  /* Merge node flags.  */
> +  if (node->force_output)
> +    prevailing_node->force_output = true;
> +  if (node->forced_by_abi)
> +    prevailing_node->forced_by_abi = true;
> +  if (node->address_taken)
> +    {
> +      prevailing_node->address_taken = true;
> +      prevailing_node->ultimate_alias_target ()->address_taken = true;
> +    }
> +  /* Redirect incomming references.  */
> +  prevailing_node->clone_referring_skip_aliases (node);
> +
> +  node->set_comdat_group (NULL);
> +  /* Dissolve same comdat group.  In most cases this is not necessary because
> +     all symbols in the group will be previaled by the symbols in prevailing
> +     group, but in the case of ODR violations we do not want to end up with
> +     multiple comdat groups of same name.  */
> +  if (node->same_comdat_group)
> +    {
> +      if (dyn_cast <cgraph_node *> (node))
> +	dyn_cast <cgraph_node *> (node)->calls_comdat_local = false;
> +      for (symtab_node *next = node->same_comdat_group;
> +	   next != node; next = next->same_comdat_group)
> +	{
> +	   if (dyn_cast <cgraph_node *> (next))
> +	     dyn_cast <cgraph_node *> (next)->calls_comdat_local = false;
> +	   next->set_comdat_group (NULL);
> +	}
> +      node->dissolve_same_comdat_group_list ();
> +    }
> +
> +  /* If there is alias that prevails the definition remains reachable.
> +     Turn it to a static symbol.  */
> +  if (has_prevailing_alias_p (node))
> +    {
> +      bool copy = false;
> +
> +      /* See if decl is shared with another symbol definition.  In this case
> +	 we must copy it.  */
> +      for (symtab_node *e = node->previous_sharing_asm_name;
> +	   e && copy; e = e->previous_sharing_asm_name)
> +	if (e->decl == node->decl
> +	    && e->stream_in_summary_p ())
> +	  copy = true;
> +      for (symtab_node *e = node->next_sharing_asm_name;
> +	   e && copy; e = e->next_sharing_asm_name)
> +	if (e->decl == node->decl
> +	    && e->stream_in_summary_p ())
> +	  copy = true;
> +
> +      if (copy)
> +	{
> +	  struct lto_in_decl_state temp;
> +	  struct lto_in_decl_state *state;
> +
> +	  /* We are going to move the decl, we want to remove its file decl
> +	     data and link these with the new decl.  */
> +	  temp.fn_decl = node->decl;
> +	  lto_in_decl_state **slot
> +	    = node->lto_file_data->function_decl_states->find_slot (&temp,
> +								   NO_INSERT);
> +	  state = *slot;
> +	  node->lto_file_data->function_decl_states->clear_slot (slot);
> +	  gcc_assert (state);
> +
> +	  /* Duplicate the decl and be sure it does not link into body
> +	     of DST.  */
> +	  node->decl = copy_node (node->decl);
> +	  node->decl->decl_with_vis.symtab_node = node;
> +	  DECL_STRUCT_FUNCTION (node->decl) = NULL;
> +	  DECL_ARGUMENTS (node->decl) = NULL;
> +	  DECL_INITIAL (node->decl) = NULL;
> +	  DECL_RESULT (node->decl) = NULL;
> +
> +	  /* Associate the decl state with new declaration, so LTO streamer
> +	     can look it up.  */
> +	  state->fn_decl = node->decl;
> +	  slot
> +	    = node->lto_file_data->function_decl_states->find_slot (state,
> +								    INSERT);
> +	  gcc_assert (!*slot);
> +	  *slot = state;
> +	}
> +      node->make_decl_local ();
> +      return false;
> +    }
> +  else
> +    {
> +      ipa_ref *ref;
> +
> +      /* All aliases are going to be removed, but we need to do that only
> +	 after all replacements are completed.  To keep verifier happy turn
> +	 them into forward declarations.  */
> +      FOR_EACH_ALIAS (node, ref)
> +	{
> +	  ref->referring->definition = false;
> +	  ref->referring->alias = false;
> +	  ref->referring->analyzed = false;
> +	}
> +    }
> +  return true;
> +}
> +
>  /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
>     all edges and removing the old node.  */
>  
> @@ -56,16 +200,8 @@ lto_cgraph_replace_node (struct cgraph_node *node,
>  		 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->decl)))));
>      }
>  
> -  /* Merge node flags.  */
> -  if (node->force_output)
> -    prevailing_node->mark_force_output ();
> -  if (node->forced_by_abi)
> -    prevailing_node->forced_by_abi = true;
> -  if (node->address_taken)
> -    {
> -      gcc_assert (!prevailing_node->inlined_to);
> -      prevailing_node->mark_address_taken ();
> -    }
> +  bool remove_p = lto_symtab_replace_node (node, prevailing_node);
> +
>    if (node->definition && prevailing_node->definition
>        && DECL_COMDAT (node->decl) && DECL_COMDAT (prevailing_node->decl))
>      prevailing_node->merged_comdat = true;
> @@ -95,15 +231,16 @@ lto_cgraph_replace_node (struct cgraph_node *node,
>  	  e->call_stmt_cannot_inline_p = 1;
>  	}
>      }
> -  /* Redirect incomming references.  */
> -  prevailing_node->clone_referring (node);
> -  lto_free_function_in_decl_state_for_node (node);
> +  if (remove_p)
> +    {
> +      lto_free_function_in_decl_state_for_node (node);
>  
> -  if (node->decl != prevailing_node->decl)
> -    node->release_body ();
> +      if (node->decl != prevailing_node->decl)
> +	node->release_body ();
>  
> -  /* Finally remove the replaced node.  */
> -  node->remove ();
> +      /* Finally remove the replaced node.  */
> +      node->remove ();
> +    }
>  }
>  
>  /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
> @@ -116,11 +253,7 @@ lto_varpool_replace_node (varpool_node *vnode,
>    gcc_assert (!vnode->definition || prevailing_node->definition);
>    gcc_assert (!vnode->analyzed || prevailing_node->analyzed);
>  
> -  prevailing_node->clone_referring (vnode);
> -  if (vnode->force_output)
> -    prevailing_node->force_output = true;
> -  if (vnode->forced_by_abi)
> -    prevailing_node->forced_by_abi = true;
> +  bool remove_p = lto_symtab_replace_node (vnode, prevailing_node);
>  
>    /* Be sure we can garbage collect the initializer.  */
>    if (DECL_INITIAL (vnode->decl)
> @@ -142,7 +275,7 @@ lto_varpool_replace_node (varpool_node *vnode,
>  	  || vnode->tls_model == TLS_MODEL_NONE
>  	  || vnode->tls_model == TLS_MODEL_EMULATED)
>  	error = true;
> -      /* Linked is silently supporting transitions
> +      /* Linker is silently supporting transitions
>  	 GD -> IE, GD -> LE, LD -> LE, IE -> LE, LD -> IE.
>  	 Do the same transitions and error out on others.  */
>        else if ((prevailing_node->tls_model == TLS_MODEL_REAL
> @@ -166,14 +299,16 @@ lto_varpool_replace_node (varpool_node *vnode,
>        if (error)
>  	{
>  	  error_at (DECL_SOURCE_LOCATION (vnode->decl),
> -		    "%qD is defined with tls model %s", vnode->decl, tls_model_names [vnode->tls_model]);
> +		    "%qD is defined with tls model %s", vnode->decl,
> +		    tls_model_names[vnode->tls_model]);
>  	  inform (DECL_SOURCE_LOCATION (prevailing_node->decl),
>  		  "previously defined here as %s",
> -		  tls_model_names [prevailing_node->tls_model]);
> +		  tls_model_names[prevailing_node->tls_model]);
>  	}
>      }
>    /* Finally remove the replaced node.  */
> -  vnode->remove ();
> +  if (remove_p)
> +    vnode->remove ();
>  }
>  
>  /* Return non-zero if we want to output waring about T1 and T2.
> @@ -408,18 +543,6 @@ lto_symtab_resolve_replaceable_p (symtab_node *e)
>    return false;
>  }
>  
> -/* Return true, if the symbol E should be resolved by lto-symtab.
> -   Those are all external symbols and all real symbols that are not static (we
> -   handle renaming of static later in partitioning).  */
> -
> -static bool
> -lto_symtab_symbol_p (symtab_node *e)
> -{
> -  if (!TREE_PUBLIC (e->decl) && !DECL_EXTERNAL (e->decl))
> -    return false;
> -  return e->real_symbol_p ();
> -}
> -
>  /* Return true if the symtab entry E can be the prevailing one.  */
>  
>  static bool
> diff --git a/gcc/symtab.c b/gcc/symtab.c
> index 3022acfc1ea..4745c2c94ec 100644
> --- a/gcc/symtab.c
> +++ b/gcc/symtab.c
> @@ -685,7 +685,7 @@ symtab_node::clone_referring (symtab_node *node)
>  {
>    ipa_ref *ref = NULL, *ref2 = NULL;
>    int i;
> -  for (i = 0; node->iterate_referring(i, ref); i++)
> +  for (i = 0; node->iterate_referring (i, ref); i++)
>      {
>        bool speculative = ref->speculative;
>        unsigned int stmt_uid = ref->lto_stmt_uid;
> @@ -697,6 +697,27 @@ symtab_node::clone_referring (symtab_node *node)
>      }
>  }
>  
> +/* Clone all referring from symtab NODE to this symtab_node
> +   Skip aliases.  */
> +
> +void
> +symtab_node::clone_referring_skip_aliases (symtab_node *node)
> +{
> +  ipa_ref *ref = NULL, *ref2 = NULL;
> +  int i;
> +  for (i = 0; node->iterate_referring (i, ref); i++)
> +    if (ref->use != IPA_REF_ALIAS)
> +      {
> +	bool speculative = ref->speculative;
> +	unsigned int stmt_uid = ref->lto_stmt_uid;
> +
> +	ref2 = ref->referring->create_reference (this, ref->use, ref->stmt);
> +	ref2->speculative = speculative;
> +	ref2->lto_stmt_uid = stmt_uid;
> +	ref2->speculative_id = ref->speculative_id;
> +      }
> +}
> +
>  /* Clone reference REF to this symtab_node and set its stmt to STMT.  */
>  
>  ipa_ref *
> @@ -813,7 +834,7 @@ symtab_node::dump_referring (FILE *file)
>  {
>    ipa_ref *ref = NULL;
>    int i;
> -  for (i = 0; iterate_referring(i, ref); i++)
> +  for (i = 0; iterate_referring (i, ref); i++)
>      {
>        fprintf (file, "%s (%s)",
>  	       ref->referring->dump_asm_name (),
> @@ -2490,3 +2511,21 @@ symtab_node::output_to_lto_symbol_table_p (void)
>      }
>    return true;
>  }
> +
> +/* Return true if summary should be streamed in.  */
> +bool
> +symtab_node::stream_in_summary_p ()
> +{
> +  ipa_ref *ref;
> +  if (prevailing_p ())
> +    return true;
> +  FOR_EACH_ALIAS (this, ref)
> +    {
> +      symtab_node *alias = ref->referring;
> +
> +      if (!alias->externally_visible
> +	  || alias->stream_in_summary_p ())
> +	return true;
> +    }
> +  return false;
> +}
> diff --git a/gcc/testsuite/gcc.dg/lto/alias-resolution-1_0.c b/gcc/testsuite/gcc.dg/lto/alias-resolution-1_0.c
> new file mode 100644
> index 00000000000..4a8471244f6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/alias-resolution-1_0.c
> @@ -0,0 +1,20 @@
> +/* { dg-require-alias "" } */
> +#define ASMNAME2(prefix, cname) STRING (prefix) cname
> +#define STRING(x)    #x
> +#define ASMNAME(cname)  ASMNAME2 (__USER_LABEL_PREFIX__, cname)
> +__attribute__((weak))
> +__attribute__((noinline))
> +int a() __asm__ (ASMNAME ("a"));
> +
> +int a()
> +{
> +  return 1;
> +}
> +__attribute__((noinline))
> +static int b() __attribute__((alias("a")));
> +void
> +test()
> +{
> +  if (b()!=1)
> +    __builtin_abort ();
> +}
> diff --git a/gcc/testsuite/gcc.dg/lto/alias-resolution-1_1.c b/gcc/testsuite/gcc.dg/lto/alias-resolution-1_1.c
> new file mode 100644
> index 00000000000..f25cfa27bc7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/alias-resolution-1_1.c
> @@ -0,0 +1,19 @@
> +#define ASMNAME(cname)  ASMNAME2 (__USER_LABEL_PREFIX__, cname)
> +#define ASMNAME2(prefix, cname) STRING (prefix) cname
> +#define STRING(x)    #x
> +__attribute__((noinline))
> +int a() __asm__ (ASMNAME ("a"));
> +
> +int a()
> +{
> +  return 2;
> +}
> +extern void test ();
> +int
> +main()
> +{
> +  if (a() != 2)
> +    __builtin_abort ();
> +  test();
> +  return 0;
> +}
>

Hello,

On Thu, 26 Mar 2020, Richard Biener wrote:

> > ===== b.c:
> > 
> > __attribute__((weak))
> > __attribute__((noinline))
> > 
> > int a()
> > {
> >   return 1;
> > }
> > 
> > __attribute__((noinline))
> > static int b() __attribute__((alias("a")));
> > void
> > test()
> > {
> >   if (b()!=1)
> >     __builtin_abort ();
> > }
> > 
> > ===== b.c:
> > 
> > __attribute__((noinline))
> > int a();
> > 
> > int a()
> > {
> >   return 2;
> > }
> > extern void test ();
> > int
> > main()
> > {
> >   if (a() != 2)
> >     __builtin_abort ();
> >   test();
> >   return 0;
> > }
> > 
> > Here LTO linking will replace weak symbol a() by definition from b.c and
> > rediect static alias b in a.c to this definition which is wrong.
> 
> Is it?  What does a non-weak local alias to a weak function mean?

I think we should continue to try to model ELF semantics re weak and 
aliases.  If so, then yes, LTO gets it wrong and the above testcase should 
not abort.  Weak doesn't enter the picture for creating aliases (the 
aliases is created with the declaration, and we require an available 
definition, and the alias is henceforth bound to _that_ very definition).  
I.e. the 'a.c:b' name should continue to refer to the same code snippet 
identified by the a.c:a name, not be redirected to the overriding b.c:a.

I'm wondering about the amount of code necessary to fix this.  I think 
that points towards a wrong level of representation somewhere, and perhaps 
_that_ should be changed instead of trying to work around it.

For instance, right now aliases are different from non-aliases, whereas in 
reality there's no difference: there are simply names referring to code or 
data snippets, and it so happens that for some snippets there are multiple 
names, and it further just so happens that some names for a code snippet 
become overridden/removed by other names for other code snippets, which 
doesn't invalidate the fact that there still is another name for the first 
snippet.

If it were modelled like that in cgraph/lto all the rest would naturally 
follow.  (Of course you would need tracking of when some code/data 
snippets can't be referred to by name anymore (and hence are useless), 
because all names for them went away, or none of the existing names is 
used anymore, but I don't think that materially would change much in our 
internal data structures).


Ciao,
Michael.

> > Is it?  What does a non-weak local alias to a weak function mean?
> 
> I think we should continue to try to model ELF semantics re weak and 
> aliases.  If so, then yes, LTO gets it wrong and the above testcase should 
> not abort.  Weak doesn't enter the picture for creating aliases (the 
> aliases is created with the declaration, and we require an available 
> definition, and the alias is henceforth bound to _that_ very definition).  
> I.e. the 'a.c:b' name should continue to refer to the same code snippet 
> identified by the a.c:a name, not be redirected to the overriding b.c:a.
> 
> I'm wondering about the amount of code necessary to fix this.  I think 
> that points towards a wrong level of representation somewhere, and perhaps 
> _that_ should be changed instead of trying to work around it.
> 
> For instance, right now aliases are different from non-aliases, whereas in 
> reality there's no difference: there are simply names referring to code or 
> data snippets, and it so happens that for some snippets there are multiple 
> names, and it further just so happens that some names for a code snippet 
> become overridden/removed by other names for other code snippets, which 
> doesn't invalidate the fact that there still is another name for the first 
> snippet.
> 
> If it were modelled like that in cgraph/lto all the rest would naturally 
> follow.  (Of course you would need tracking of when some code/data 
> snippets can't be referred to by name anymore (and hence are useless), 
> because all names for them went away, or none of the existing names is 
> used anymore, but I don't think that materially would change much in our 
> internal data structures).

Yep, the trouble is caused by fact that GCC representation is not quite
what linker does. I.e. we bind function bodies with their declarations
and declarations with symbols.
We are bouit about assumption of 1to1 correspondence between function
bodies and their symbols.
This is not true because one body may have multiple aliases but also
it may define mutliple different symbols (alternative entry points &
labels).

Reorganizing this design is quite some work.

We need to change trees/GIMPLE to use symbols instead of DECLs when
referring to them.  So parameter of CALL_EXPR would not be decl but
symbol associated with it. 

We to move everyting that is property of symbol away from decl into
symbols (i.e. assembler name, visibility, section etc) and thus we would
need to change everything from frontends all the way to RTL.

I am gradually shuffling things in this direction (I plan to move
assembler names to symbols and gradually do that for visibilitie so we
can have cross-module referneces to labels and finally support static
initializers taking addresses of them), but the process is
slow and I think it makes sense to first fix correctness issues with
what we have rahter than waiting for major surgery to be finished.

Honza
> 
> 
> Ciao,
> Michael.

> 
> Why don't we represent the alias at the cgraph level?  That is,
> why do decls come into play at all here?  b prevails and it has a
> reference to a.c:a so we need to keep (and emit) that.  The only issue
> would be that we'd end up with two 'a's so we have to notice that
> and fixup somehow.
> 
> Isn't this in the end a similar issue as with extern inline bodies
> vs. the offline copy body?

The difference is that, for the first time, we need to load two
different bodies for one DECL (because first is reachable by alias in
a.c and other is reahable by the prevailing non-weak declaration).
Function bodies are bound to decls and not symbols and this needs to be done somehow.
> 
> > I would like to do this w/o need to stream in the body, because such situation
> > happens quite commonly local aliases: Until we are able to prove that all
> > referencs to a local aliase of a symbol are gone we need to assume that body
> > needs to be preserved and those are potentially quite frequent.
> > 
> > So basically only solution I think of is to make lto-stream-in live with the
> > fact that function decl was changed.  So we can stream original decl to the
> > beggining of the function section and during stream in do rewritting of
> > DECL_CONTEXT similar way as tree-inline would do.  We need to be careful about
> > keeping debug info in shape too (we are not producing inline copy, we are
> > merely unsharing the main decl).
> > 
> > What do you think?
> 
> I think the not prevailed but still needed cgraph node for 'a' should
> have its decl cleared ;)

I am not sure I understand what you mean here :)
> 
> > Note that I was considering case of simply not sharing decls with
> > aliases at stream time.  This does not work because by localizing the
> > decl one will not redirect references to the symbol correctly.
> 
> But all remaining references should be via 'b', not via 'a'.

Yes, they are, that is why I create a.static and b becomes alias of it.
Moving body of a.c:a to b would also work, but it is essentially the
same problem.  All we would need to do is to pick an "winning" alias,
make it the main definition and make aliases alias of it.
I think this would only introduce furhter problem, since signature of b
is not guaranteed to be same as signature of a.

Honza
> 
> > Other option would be to always produce a static symbol for every
> > symbol with an alias.  This again can be IMO only done by cloning it and
> > would be relatively expensive because at the stream out time we do have
> > considerable quantity of aliases especially with -fpic.
> > 
> > Honza
> > 
> > gcc/ChangeLog:
> > 
> > 2020-03-26  Jan Hubicka  <hubicka@ucw.cz>
> > 
> > 	* cgraph.c (cgraph_node::make_local): Add force parameter.
> > 	* cgraph.h (symtab_node::clone_referring_skip_aliases): New
> > 	member function.
> > 	(symtab_node::stream_in_summary_p): Likewise.
> > 	* ipa-fnsummary.c (inline_read_section): Use
> > 	symtab_node::stream_in_summary_p.
> > 	* ipa-prop.c (ipa_read_node_info): Likewise.
> > 	* symtab.c (symtab_node::clone_referring): Fix formating.
> > 	(symtab_node::clone_referring_skip_aliases): New member function.
> > 	(symtab_node::dump_referring): Fix formating
> > 	(symtab_node::stream_in_summary_p): New member function.
> > 
> > gcc/lto/ChangeLog:
> > 
> > 2020-03-26  Jan Hubicka  <hubicka@ucw.cz>
> > 
> > 	* lto-symtab.c (lto_symtab_symbol_p): New function.
> > 	(has_prevailing_alias_p): New function.
> > 	(lto_symtab_replace_node): Break out from ...; support alias
> > 	interposition
> > 	(lto_cgraph_replace_node): ... here.
> > 	(lto_varpool_replace_node): Use the common code.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 2020-03-26  Jan Hubicka  <hubicka@ucw.cz>
> > 
> > 	* gcc.dg/lto/alias-resolution-1_0.c: New test.
> > 	* gcc.dg/lto/alias-resolution-1_1.c: New test.
> > 
> > diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> > index 6b780f80eb3..1f1319ed5fe 100644
> > --- a/gcc/cgraph.c
> > +++ b/gcc/cgraph.c
> > @@ -2465,9 +2465,10 @@ cgraph_node::call_for_symbol_thunks_and_aliases (bool (*callback)
> >  /* Worker to bring NODE local.  */
> >  
> >  bool
> > -cgraph_node::make_local (cgraph_node *node, void *)
> > +cgraph_node::make_local (cgraph_node *node, void *force)
> >  {
> > -  gcc_checking_assert (node->can_be_local_p ());
> > +  if (!force)
> > +    gcc_checking_assert (node->can_be_local_p ());
> >    if (DECL_COMDAT (node->decl) || DECL_EXTERNAL (node->decl))
> >      {
> >        node->make_decl_local ();
> > @@ -2486,12 +2487,14 @@ cgraph_node::make_local (cgraph_node *node, void *)
> >    return false;
> >  }
> >  
> > -/* Bring cgraph node local.  */
> > +/* Bring cgraph node local.  If FORCE is true skip checking if symbol can
> > +   become local.  */
> >  
> >  void
> > -cgraph_node::make_local (void)
> > +cgraph_node::make_local (bool force)
> >  {
> > -  call_for_symbol_thunks_and_aliases (cgraph_node::make_local, NULL, true);
> > +  call_for_symbol_thunks_and_aliases (cgraph_node::make_local,
> > +				      (void *)(size_t)force, true);
> >  }
> >  
> >  /* Worker to set nothrow flag.  */
> > diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> > index 43de3b4a8ac..983cbfe8cd8 100644
> > --- a/gcc/cgraph.h
> > +++ b/gcc/cgraph.h
> > @@ -189,6 +189,9 @@ public:
> >       with callgraph edges associated with them.  */
> >    void clone_referring (symtab_node *node);
> >  
> > +  /* Same as clone_referring but skip aliases.  */
> > +  void clone_referring_skip_aliases (symtab_node *node);
> > +
> >    /* Clone reference REF to this symtab_node and set its stmt to STMT.  */
> >    ipa_ref *clone_reference (ipa_ref *ref, gimple *stmt);
> >  
> > @@ -340,6 +343,9 @@ public:
> >       in question prevails in the linking to save some memory usage.  */
> >    bool prevailing_p (void);
> >  
> > +  /* Return true if the summary should be streamed in.  */
> > +  bool stream_in_summary_p (void);
> > +
> >    /* Return true if NODE binds to current definition in final executable
> >       when referenced from REF.  If REF is NULL return conservative value
> >       for any reference.  */
> > @@ -1163,8 +1169,9 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
> >    /* cgraph_node is no longer nested function; update cgraph accordingly.  */
> >    void unnest (void);
> >  
> > -  /* Bring cgraph node local.  */
> > -  void make_local (void);
> > +  /* Bring cgraph node local.  If FORCE is true skip checking if symbol can
> > +     be local.  */
> > +  void make_local (bool force = false);
> >  
> >    /* Likewise indicate that a node is having address taken.  */
> >    void mark_address_taken (void);
> > diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
> > index b411bc4d660..a79e75dd001 100644
> > --- a/gcc/ipa-fnsummary.c
> > +++ b/gcc/ipa-fnsummary.c
> > @@ -4176,10 +4176,10 @@ inline_read_section (struct lto_file_decl_data *file_data, const char *data,
> >        encoder = file_data->symtab_node_encoder;
> >        node = dyn_cast<cgraph_node *> (lto_symtab_encoder_deref (encoder,
> >  								index));
> > -      info = node->prevailing_p () ? ipa_fn_summaries->get_create (node) : NULL;
> > -      params_summary = node->prevailing_p () ? IPA_NODE_REF (node) : NULL;
> > -      size_info = node->prevailing_p ()
> > -		  ? ipa_size_summaries->get_create (node) : NULL;
> > +      bool stream_in = node->stream_in_summary_p ();
> > +      info = stream_in ? ipa_fn_summaries->get_create (node) : NULL;
> > +      params_summary = stream_in ? IPA_NODE_REF (node) : NULL;
> > +      size_info = stream_in ? ipa_size_summaries->get_create (node) : NULL;
> >  
> >        int stack_size = streamer_read_uhwi (&ib);
> >        int size = streamer_read_uhwi (&ib);
> > diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> > index a77130ded39..4035d903d33 100644
> > --- a/gcc/ipa-prop.c
> > +++ b/gcc/ipa-prop.c
> > @@ -4937,7 +4937,7 @@ ipa_read_node_info (class lto_input_block *ib, struct cgraph_node *node,
> >    int k;
> >    struct cgraph_edge *e;
> >    struct bitpack_d bp;
> > -  bool prevails = node->prevailing_p ();
> > +  bool prevails = node->stream_in_summary_p ();
> >    class ipa_node_params *info = prevails
> >  				? IPA_NODE_REF_GET_CREATE (node) : NULL;
> >  
> > diff --git a/gcc/lto/lto-symtab.c b/gcc/lto/lto-symtab.c
> > index d5e38beb4b6..3f4ece360d7 100644
> > --- a/gcc/lto/lto-symtab.c
> > +++ b/gcc/lto/lto-symtab.c
> > @@ -36,6 +36,150 @@ along with GCC; see the file COPYING3.  If not see
> >  #include "stringpool.h"
> >  #include "attribs.h"
> >  
> > +/* Return true, if the symbol E should be resolved by lto-symtab.
> > +   Those are all external symbols and all real symbols that are not static (we
> > +   handle renaming of static later in partitioning).  */
> > +
> > +static bool
> > +lto_symtab_symbol_p (symtab_node *e)
> > +{
> > +  if (!TREE_PUBLIC (e->decl) && !DECL_EXTERNAL (e->decl))
> > +    return false;
> > +  return e->real_symbol_p ();
> > +}
> > +
> > +/* Return ture if NODE is an prevailing alias or has alias that prevalis.  */
> > +
> > +static bool
> > +has_prevailing_alias_p (struct symtab_node *node)
> > +{
> > +  ipa_ref *ref;
> > +
> > +  FOR_EACH_ALIAS (node, ref)
> > +    {
> > +      symtab_node *alias = ref->referring;
> > +
> > +      if (lto_symtab_symbol_p (alias))
> > +	return alias->prevailing_p ();
> > +      else
> > +	return true;
> > +      if (has_prevailing_alias_p (alias))
> > +	return true;
> > +    }
> > +  return false;
> > +}
> > +
> > +/* Common part of lto_cgraph_replace_node and lto_varpool_replace_node
> > +   Retun true if NODE can be removed.  */
> > +
> > +static bool
> > +lto_symtab_replace_node (struct symtab_node *node,
> > +			 struct symtab_node *prevailing_node)
> > +{
> > +  /* Merge node flags.  */
> > +  if (node->force_output)
> > +    prevailing_node->force_output = true;
> > +  if (node->forced_by_abi)
> > +    prevailing_node->forced_by_abi = true;
> > +  if (node->address_taken)
> > +    {
> > +      prevailing_node->address_taken = true;
> > +      prevailing_node->ultimate_alias_target ()->address_taken = true;
> > +    }
> > +  /* Redirect incomming references.  */
> > +  prevailing_node->clone_referring_skip_aliases (node);
> > +
> > +  node->set_comdat_group (NULL);
> > +  /* Dissolve same comdat group.  In most cases this is not necessary because
> > +     all symbols in the group will be previaled by the symbols in prevailing
> > +     group, but in the case of ODR violations we do not want to end up with
> > +     multiple comdat groups of same name.  */
> > +  if (node->same_comdat_group)
> > +    {
> > +      if (dyn_cast <cgraph_node *> (node))
> > +	dyn_cast <cgraph_node *> (node)->calls_comdat_local = false;
> > +      for (symtab_node *next = node->same_comdat_group;
> > +	   next != node; next = next->same_comdat_group)
> > +	{
> > +	   if (dyn_cast <cgraph_node *> (next))
> > +	     dyn_cast <cgraph_node *> (next)->calls_comdat_local = false;
> > +	   next->set_comdat_group (NULL);
> > +	}
> > +      node->dissolve_same_comdat_group_list ();
> > +    }
> > +
> > +  /* If there is alias that prevails the definition remains reachable.
> > +     Turn it to a static symbol.  */
> > +  if (has_prevailing_alias_p (node))
> > +    {
> > +      bool copy = false;
> > +
> > +      /* See if decl is shared with another symbol definition.  In this case
> > +	 we must copy it.  */
> > +      for (symtab_node *e = node->previous_sharing_asm_name;
> > +	   e && copy; e = e->previous_sharing_asm_name)
> > +	if (e->decl == node->decl
> > +	    && e->stream_in_summary_p ())
> > +	  copy = true;
> > +      for (symtab_node *e = node->next_sharing_asm_name;
> > +	   e && copy; e = e->next_sharing_asm_name)
> > +	if (e->decl == node->decl
> > +	    && e->stream_in_summary_p ())
> > +	  copy = true;
> > +
> > +      if (copy)
> > +	{
> > +	  struct lto_in_decl_state temp;
> > +	  struct lto_in_decl_state *state;
> > +
> > +	  /* We are going to move the decl, we want to remove its file decl
> > +	     data and link these with the new decl.  */
> > +	  temp.fn_decl = node->decl;
> > +	  lto_in_decl_state **slot
> > +	    = node->lto_file_data->function_decl_states->find_slot (&temp,
> > +								   NO_INSERT);
> > +	  state = *slot;
> > +	  node->lto_file_data->function_decl_states->clear_slot (slot);
> > +	  gcc_assert (state);
> > +
> > +	  /* Duplicate the decl and be sure it does not link into body
> > +	     of DST.  */
> > +	  node->decl = copy_node (node->decl);
> > +	  node->decl->decl_with_vis.symtab_node = node;
> > +	  DECL_STRUCT_FUNCTION (node->decl) = NULL;
> > +	  DECL_ARGUMENTS (node->decl) = NULL;
> > +	  DECL_INITIAL (node->decl) = NULL;
> > +	  DECL_RESULT (node->decl) = NULL;
> > +
> > +	  /* Associate the decl state with new declaration, so LTO streamer
> > +	     can look it up.  */
> > +	  state->fn_decl = node->decl;
> > +	  slot
> > +	    = node->lto_file_data->function_decl_states->find_slot (state,
> > +								    INSERT);
> > +	  gcc_assert (!*slot);
> > +	  *slot = state;
> > +	}
> > +      node->make_decl_local ();
> > +      return false;
> > +    }
> > +  else
> > +    {
> > +      ipa_ref *ref;
> > +
> > +      /* All aliases are going to be removed, but we need to do that only
> > +	 after all replacements are completed.  To keep verifier happy turn
> > +	 them into forward declarations.  */
> > +      FOR_EACH_ALIAS (node, ref)
> > +	{
> > +	  ref->referring->definition = false;
> > +	  ref->referring->alias = false;
> > +	  ref->referring->analyzed = false;
> > +	}
> > +    }
> > +  return true;
> > +}
> > +
> >  /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
> >     all edges and removing the old node.  */
> >  
> > @@ -56,16 +200,8 @@ lto_cgraph_replace_node (struct cgraph_node *node,
> >  		 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->decl)))));
> >      }
> >  
> > -  /* Merge node flags.  */
> > -  if (node->force_output)
> > -    prevailing_node->mark_force_output ();
> > -  if (node->forced_by_abi)
> > -    prevailing_node->forced_by_abi = true;
> > -  if (node->address_taken)
> > -    {
> > -      gcc_assert (!prevailing_node->inlined_to);
> > -      prevailing_node->mark_address_taken ();
> > -    }
> > +  bool remove_p = lto_symtab_replace_node (node, prevailing_node);
> > +
> >    if (node->definition && prevailing_node->definition
> >        && DECL_COMDAT (node->decl) && DECL_COMDAT (prevailing_node->decl))
> >      prevailing_node->merged_comdat = true;
> > @@ -95,15 +231,16 @@ lto_cgraph_replace_node (struct cgraph_node *node,
> >  	  e->call_stmt_cannot_inline_p = 1;
> >  	}
> >      }
> > -  /* Redirect incomming references.  */
> > -  prevailing_node->clone_referring (node);
> > -  lto_free_function_in_decl_state_for_node (node);
> > +  if (remove_p)
> > +    {
> > +      lto_free_function_in_decl_state_for_node (node);
> >  
> > -  if (node->decl != prevailing_node->decl)
> > -    node->release_body ();
> > +      if (node->decl != prevailing_node->decl)
> > +	node->release_body ();
> >  
> > -  /* Finally remove the replaced node.  */
> > -  node->remove ();
> > +      /* Finally remove the replaced node.  */
> > +      node->remove ();
> > +    }
> >  }
> >  
> >  /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
> > @@ -116,11 +253,7 @@ lto_varpool_replace_node (varpool_node *vnode,
> >    gcc_assert (!vnode->definition || prevailing_node->definition);
> >    gcc_assert (!vnode->analyzed || prevailing_node->analyzed);
> >  
> > -  prevailing_node->clone_referring (vnode);
> > -  if (vnode->force_output)
> > -    prevailing_node->force_output = true;
> > -  if (vnode->forced_by_abi)
> > -    prevailing_node->forced_by_abi = true;
> > +  bool remove_p = lto_symtab_replace_node (vnode, prevailing_node);
> >  
> >    /* Be sure we can garbage collect the initializer.  */
> >    if (DECL_INITIAL (vnode->decl)
> > @@ -142,7 +275,7 @@ lto_varpool_replace_node (varpool_node *vnode,
> >  	  || vnode->tls_model == TLS_MODEL_NONE
> >  	  || vnode->tls_model == TLS_MODEL_EMULATED)
> >  	error = true;
> > -      /* Linked is silently supporting transitions
> > +      /* Linker is silently supporting transitions
> >  	 GD -> IE, GD -> LE, LD -> LE, IE -> LE, LD -> IE.
> >  	 Do the same transitions and error out on others.  */
> >        else if ((prevailing_node->tls_model == TLS_MODEL_REAL
> > @@ -166,14 +299,16 @@ lto_varpool_replace_node (varpool_node *vnode,
> >        if (error)
> >  	{
> >  	  error_at (DECL_SOURCE_LOCATION (vnode->decl),
> > -		    "%qD is defined with tls model %s", vnode->decl, tls_model_names [vnode->tls_model]);
> > +		    "%qD is defined with tls model %s", vnode->decl,
> > +		    tls_model_names[vnode->tls_model]);
> >  	  inform (DECL_SOURCE_LOCATION (prevailing_node->decl),
> >  		  "previously defined here as %s",
> > -		  tls_model_names [prevailing_node->tls_model]);
> > +		  tls_model_names[prevailing_node->tls_model]);
> >  	}
> >      }
> >    /* Finally remove the replaced node.  */
> > -  vnode->remove ();
> > +  if (remove_p)
> > +    vnode->remove ();
> >  }
> >  
> >  /* Return non-zero if we want to output waring about T1 and T2.
> > @@ -408,18 +543,6 @@ lto_symtab_resolve_replaceable_p (symtab_node *e)
> >    return false;
> >  }
> >  
> > -/* Return true, if the symbol E should be resolved by lto-symtab.
> > -   Those are all external symbols and all real symbols that are not static (we
> > -   handle renaming of static later in partitioning).  */
> > -
> > -static bool
> > -lto_symtab_symbol_p (symtab_node *e)
> > -{
> > -  if (!TREE_PUBLIC (e->decl) && !DECL_EXTERNAL (e->decl))
> > -    return false;
> > -  return e->real_symbol_p ();
> > -}
> > -
> >  /* Return true if the symtab entry E can be the prevailing one.  */
> >  
> >  static bool
> > diff --git a/gcc/symtab.c b/gcc/symtab.c
> > index 3022acfc1ea..4745c2c94ec 100644
> > --- a/gcc/symtab.c
> > +++ b/gcc/symtab.c
> > @@ -685,7 +685,7 @@ symtab_node::clone_referring (symtab_node *node)
> >  {
> >    ipa_ref *ref = NULL, *ref2 = NULL;
> >    int i;
> > -  for (i = 0; node->iterate_referring(i, ref); i++)
> > +  for (i = 0; node->iterate_referring (i, ref); i++)
> >      {
> >        bool speculative = ref->speculative;
> >        unsigned int stmt_uid = ref->lto_stmt_uid;
> > @@ -697,6 +697,27 @@ symtab_node::clone_referring (symtab_node *node)
> >      }
> >  }
> >  
> > +/* Clone all referring from symtab NODE to this symtab_node
> > +   Skip aliases.  */
> > +
> > +void
> > +symtab_node::clone_referring_skip_aliases (symtab_node *node)
> > +{
> > +  ipa_ref *ref = NULL, *ref2 = NULL;
> > +  int i;
> > +  for (i = 0; node->iterate_referring (i, ref); i++)
> > +    if (ref->use != IPA_REF_ALIAS)
> > +      {
> > +	bool speculative = ref->speculative;
> > +	unsigned int stmt_uid = ref->lto_stmt_uid;
> > +
> > +	ref2 = ref->referring->create_reference (this, ref->use, ref->stmt);
> > +	ref2->speculative = speculative;
> > +	ref2->lto_stmt_uid = stmt_uid;
> > +	ref2->speculative_id = ref->speculative_id;
> > +      }
> > +}
> > +
> >  /* Clone reference REF to this symtab_node and set its stmt to STMT.  */
> >  
> >  ipa_ref *
> > @@ -813,7 +834,7 @@ symtab_node::dump_referring (FILE *file)
> >  {
> >    ipa_ref *ref = NULL;
> >    int i;
> > -  for (i = 0; iterate_referring(i, ref); i++)
> > +  for (i = 0; iterate_referring (i, ref); i++)
> >      {
> >        fprintf (file, "%s (%s)",
> >  	       ref->referring->dump_asm_name (),
> > @@ -2490,3 +2511,21 @@ symtab_node::output_to_lto_symbol_table_p (void)
> >      }
> >    return true;
> >  }
> > +
> > +/* Return true if summary should be streamed in.  */
> > +bool
> > +symtab_node::stream_in_summary_p ()
> > +{
> > +  ipa_ref *ref;
> > +  if (prevailing_p ())
> > +    return true;
> > +  FOR_EACH_ALIAS (this, ref)
> > +    {
> > +      symtab_node *alias = ref->referring;
> > +
> > +      if (!alias->externally_visible
> > +	  || alias->stream_in_summary_p ())
> > +	return true;
> > +    }
> > +  return false;
> > +}
> > diff --git a/gcc/testsuite/gcc.dg/lto/alias-resolution-1_0.c b/gcc/testsuite/gcc.dg/lto/alias-resolution-1_0.c
> > new file mode 100644
> > index 00000000000..4a8471244f6
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/lto/alias-resolution-1_0.c
> > @@ -0,0 +1,20 @@
> > +/* { dg-require-alias "" } */
> > +#define ASMNAME2(prefix, cname) STRING (prefix) cname
> > +#define STRING(x)    #x
> > +#define ASMNAME(cname)  ASMNAME2 (__USER_LABEL_PREFIX__, cname)
> > +__attribute__((weak))
> > +__attribute__((noinline))
> > +int a() __asm__ (ASMNAME ("a"));
> > +
> > +int a()
> > +{
> > +  return 1;
> > +}
> > +__attribute__((noinline))
> > +static int b() __attribute__((alias("a")));
> > +void
> > +test()
> > +{
> > +  if (b()!=1)
> > +    __builtin_abort ();
> > +}
> > diff --git a/gcc/testsuite/gcc.dg/lto/alias-resolution-1_1.c b/gcc/testsuite/gcc.dg/lto/alias-resolution-1_1.c
> > new file mode 100644
> > index 00000000000..f25cfa27bc7
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/lto/alias-resolution-1_1.c
> > @@ -0,0 +1,19 @@
> > +#define ASMNAME(cname)  ASMNAME2 (__USER_LABEL_PREFIX__, cname)
> > +#define ASMNAME2(prefix, cname) STRING (prefix) cname
> > +#define STRING(x)    #x
> > +__attribute__((noinline))
> > +int a() __asm__ (ASMNAME ("a"));
> > +
> > +int a()
> > +{
> > +  return 2;
> > +}
> > +extern void test ();
> > +int
> > +main()
> > +{
> > +  if (a() != 2)
> > +    __builtin_abort ();
> > +  test();
> > +  return 0;
> > +}
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Hello,

On Thu, 26 Mar 2020, Jan Hubicka wrote:

> > I think we should continue to try to model ELF semantics re weak and 
> > aliases.  If so, then yes, LTO gets it wrong and the above testcase should 
> > not abort.  Weak doesn't enter the picture for creating aliases (the 
> > aliases is created with the declaration, and we require an available 
> > definition, and the alias is henceforth bound to _that_ very definition).  
> > I.e. the 'a.c:b' name should continue to refer to the same code snippet 
> > identified by the a.c:a name, not be redirected to the overriding b.c:a.
> > 
> > I'm wondering about the amount of code necessary to fix this.  I think 
> > that points towards a wrong level of representation somewhere, and perhaps 
> > _that_ should be changed instead of trying to work around it.
> > 
> > For instance, right now aliases are different from non-aliases, whereas in 
> > reality there's no difference: there are simply names referring to code or 
> > data snippets, and it so happens that for some snippets there are multiple 
> > names, and it further just so happens that some names for a code snippet 
> > become overridden/removed by other names for other code snippets, which 
> > doesn't invalidate the fact that there still is another name for the first 
> > snippet.
> > 
> > If it were modelled like that in cgraph/lto all the rest would naturally 
> > follow.  (Of course you would need tracking of when some code/data 
> > snippets can't be referred to by name anymore (and hence are useless), 
> > because all names for them went away, or none of the existing names is 
> > used anymore, but I don't think that materially would change much in our 
> > internal data structures).
> 
> Yep, the trouble is caused by fact that GCC representation is not quite 
> what linker does. I.e. we bind function bodies with their declarations 
> and declarations with symbols. We are bouit about assumption of 1to1 
> correspondence between function bodies and their symbols. This is not 
> true because one body may have multiple aliases but also it may define 
> mutliple different symbols (alternative entry points & labels).
> 
> Reorganizing this design is quite some work.
> 
> We need to change trees/GIMPLE to use symbols instead of DECLs when
> referring to them.  So parameter of CALL_EXPR would not be decl but
> symbol associated with it. 
> 
> We to move everyting that is property of symbol away from decl into
> symbols (i.e. assembler name, visibility, section etc) and thus we would
> need to change everything from frontends all the way to RTL.

I'm not sure I agree that it's that complicated.  With the above you 
essentially do away with the need of DECLs at all.  calls would refer to 
your fat symbols, symbols would refer to code/data bodies.  Data 
(e.g. vtables) would also refer to symbols.  Nowhere would be a DECL in 
that chain.  At that point you can just as well call your new symbols 
"DECLs".  The crucial part is merely that the DECLs/fat-symbols can change 
their association with code/data blobs.

(Well, I can see that some properties of code blobs, for instance the API, 
and hence the (function) type, is inherent to the code blob, and therefore 
should be 1to1 associated with it, some there's some argument to be made 
to retain some info of the DECL-as-now to be put tighly to the code blob 
representation).

> I am gradually shuffling things in this direction (I plan to move 
> assembler names to symbols and gradually do that for visibilitie so we 
> can have cross-module referneces to labels and finally support static 
> initializers taking addresses of them), but the process is slow and I 
> think it makes sense to first fix correctness issues with what we have 
> rahter than waiting for major surgery to be finished.

True.  I was just surprised by the size of this change, many diff pluses, 
few minuses :)


Ciao,
Michael.