[PATCH, GCC/LTO, ping3] Fix PR69866: LTO with def for weak alias in regular object file

[Thomas Preudhomme]

Hi,

On 09/05/17 23:36, Jan Hubicka wrote:
>> Ping?
> Sorry for late reply
>>> Hi,
>>>
>>> This patch fixes an assert failure when linking one LTOed object file
>>> having a weak alias with a regular object file containing a strong
>>> definition for that same symbol. The patch is twofold:
>>>
>>> + do not add an alias to a partition if it is external
>>> + do not declare (.globl) an alias if it is external
>
> Adding external alises to partitions is important to keep the information
> that two symbols are the same.
> The second part makes sense to me.  What breaks when you drop the first
> change?

Adding aliases to partitions is what cause the ICE referenced in the PR. It 
fails on the following assert:

   gcc_assert (c != SYMBOL_EXTERNAL
               && (c == SYMBOL_DUPLICATE || !symbol_partitioned_p (node)));


Second change came about when doing the first change because the linker was 
complaining about the alias being defined twice (once in the trans object, once 
in the non LTO object).

Best regards,

Thomas

>
> Honza
>>>
>>> ChangeLog entries are as follow:
>>>
>>> *** gcc/lto/ChangeLog ***
>>>
>>> 2017-03-01  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>>>
>>>    PR lto/69866
>>>    * lto/lto-partition.c (add_symbol_to_partition_1): Do not add external
>>>    aliases to partition.
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2017-03-01  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>>>
>>>    PR lto/69866
>>>    * cgraphunit.c (cgraph_node::assemble_thunks_and_aliases): Do not
>>>    declare external aliases.
>>>
>>> *** gcc/testsuite/ChangeLog ***
>>>
>>> 2017-02-28  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>>>
>>>    PR lto/69866
>>>    * gcc.dg/lto/pr69866_0.c: New test.
>>>    * gcc.dg/lto/pr69866_1.c: Likewise.
>>>
>>>
>>> Testing: Testsuite shows no regression when targeting Cortex-M3 with an
>>> arm-none-eabi GCC cross-compiler, neither does it show any regression with
>>> native LTO-bootstrapped x86-64_linux-gnu and aarch64-linux-gnu compilers.
>>>
>>> Is this ok for stage4?
>>>
>>> Best regards,
>>>
>>> Thomas
>>>
>>> On 31/03/17 18:07, Richard Biener wrote:
>>>> On March 31, 2017 5:23:03 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>>>>> On 03/16/2017 08:05 AM, Thomas Preudhomme wrote:
>>>>>> Ping?
>>>>>>
>>>>>> Is this ok for stage4?
>>>>> Given the lack of response from Richi, I'd suggest deferring to stage1.
>>>>
>>>> Honza needs to review this, i habe too little knowledge here.
>>>>
>>>> Richard.
>>>>
>>>>> jeff
>>>>
>
>> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
>> index c82a88a599ca61b068dd9783d2a6158163809b37..580500ff922b8546d33119261a2455235edbf16d 100644
>> --- a/gcc/cgraphunit.c
>> +++ b/gcc/cgraphunit.c
>> @@ -1972,7 +1972,7 @@ cgraph_node::assemble_thunks_and_aliases (void)
>>    FOR_EACH_ALIAS (this, ref)
>>      {
>>        cgraph_node *alias = dyn_cast <cgraph_node *> (ref->referring);
>> -      if (!alias->transparent_alias)
>> +      if (!alias->transparent_alias && !DECL_EXTERNAL (alias->decl))
>>  	{
>>  	  bool saved_written = TREE_ASM_WRITTEN (decl);
>>
>> diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c
>> index e27d0d1690c1fcfb39e2fac03ce0f4154031fc7c..f44fd435ed075a27e373bdfdf0464eb06e1731ef 100644
>> --- a/gcc/lto/lto-partition.c
>> +++ b/gcc/lto/lto-partition.c
>> @@ -178,7 +178,8 @@ add_symbol_to_partition_1 (ltrans_partition part, symtab_node *node)
>>    /* Add all aliases associated with the symbol.  */
>>
>>    FOR_EACH_ALIAS (node, ref)
>> -    if (!ref->referring->transparent_alias)
>> +    if (!ref->referring->transparent_alias
>> +	&& ref->referring->get_partitioning_class () != SYMBOL_EXTERNAL)
>>        add_symbol_to_partition_1 (part, ref->referring);
>>      else
>>        {
>> @@ -189,7 +190,8 @@ add_symbol_to_partition_1 (ltrans_partition part, symtab_node *node)
>>  	  {
>>  	    /* Nested transparent aliases are not permitted.  */
>>  	    gcc_checking_assert (!ref2->referring->transparent_alias);
>> -	    add_symbol_to_partition_1 (part, ref2->referring);
>> +	    if (ref2->referring->get_partitioning_class () != SYMBOL_EXTERNAL)
>> +	      add_symbol_to_partition_1 (part, ref2->referring);
>>  	  }
>>        }
>>
>> diff --git a/gcc/testsuite/gcc.dg/lto/pr69866_0.c b/gcc/testsuite/gcc.dg/lto/pr69866_0.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..f49ef8d4c1da7a21d1bfb5409d647bd18141595b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/lto/pr69866_0.c
>> @@ -0,0 +1,13 @@
>> +/* { dg-lto-do link } */
>> +
>> +int _umh(int i)
>> +{
>> +  return i+1;
>> +}
>> +
>> +int weaks(int i) __attribute__((weak, alias("_umh")));
>> +
>> +int main()
>> +{
>> +  return weaks(10);
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/lto/pr69866_1.c b/gcc/testsuite/gcc.dg/lto/pr69866_1.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..3a14f850eefaffbf659ce4642adef7900330f4ed
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/lto/pr69866_1.c
>> @@ -0,0 +1,6 @@
>> +/* { dg-options { -fno-lto } } */
>> +
>> +int weaks(int i)
>> +{
>> +  return i+1;
>> +}
>