PR rtl-optimization/32219: optimizer causees wrong code in pic/hidden/weak symbol checking

On 02/19/2015 09:08 AM, Alex Velenko wrote:
> Your suggestion seem to fix gcc.target/arm/long-calls-1.c, but has to be
> thoroughly tested.

Before you do complete testing, please also delete the TREE_STATIC test.
That bit should never be relevant to functions, as it indicates not that
it is in the compilation unit, but that it has static (as opposed to
automatic) storage duration.  Thus it is only relevant to variables.


r~

On 19/02/15 17:26, Richard Henderson wrote:
> On 02/19/2015 09:08 AM, Alex Velenko wrote:
>> Your suggestion seem to fix gcc.target/arm/long-calls-1.c, but has to be
>> thoroughly tested.
>
> Before you do complete testing, please also delete the TREE_STATIC test.
> That bit should never be relevant to functions, as it indicates not that
> it is in the compilation unit, but that it has static (as opposed to
> automatic) storage duration.  Thus it is only relevant to variables.
>
>
> r~
>
>
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 7bf5b4d..777230e 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -6392,14 +6392,8 @@ arm_set_default_type_attributes (tree type)
>   static bool
>   arm_function_in_section_p (tree decl, section *section)
>   {
> -  /* We can only be certain about functions defined in the same
> -     compilation unit.  */
> -  if (!TREE_STATIC (decl))
> -    return false;
> -
> -  /* Make sure that SYMBOL always binds to the definition in this
> -     compilation unit.  */
> -  if (!targetm.binds_local_p (decl))
> +  /* We can only be certain about the prevailing symbol definition.  */
> +  if (!decl_binds_to_current_def_p (decl))
>       return false;
>
>     /* If DECL_SECTION_NAME is set, assume it is trustworthy.  */
>
>

Hi,

Did a bootstrap and a full regression run on arm-none-linux-gnueabihf,
No new regressions found. Some previously failing tests in libstdc++ 
started to fail differently, for example:

< ERROR: 22_locale/num_get/get/wchar_t/2.cc: can't read 
"additional_sources": no such variable for " dg-do 22 run { xfail 
lax_strtof\
p } "
< UNRESOLVED: 22_locale/num_get/get/wchar_t/2.cc: can't read 
"additional_sources": no such variable for " dg-do 22 run { xfail lax_s\
trtofp } "
---
 > ERROR: 22_locale/num_get/get/wchar_t/2.cc: can't read 
"et_cache(uclibc,value)": no such element in array for " dg-do 22 run { 
xfai\
l lax_strtofp } "
 > UNRESOLVED: 22_locale/num_get/get/wchar_t/2.cc: can't read 
"et_cache(uclibc,value)": no such element in array for " dg-do 22 run {\
  xfail lax_strtofp } "


But I think it is okay.

Kind regards,
Alex

On 03/03/15 15:58, Alex Velenko wrote:
> On 19/02/15 17:26, Richard Henderson wrote:
>> On 02/19/2015 09:08 AM, Alex Velenko wrote:
>>> Your suggestion seem to fix gcc.target/arm/long-calls-1.c, but has 
>>> to be
>>> thoroughly tested.
>>
>> Before you do complete testing, please also delete the TREE_STATIC test.
>> That bit should never be relevant to functions, as it indicates not that
>> it is in the compilation unit, but that it has static (as opposed to
>> automatic) storage duration.  Thus it is only relevant to variables.
>>
>>
>> r~
>>
>>
>>
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index 7bf5b4d..777230e 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -6392,14 +6392,8 @@ arm_set_default_type_attributes (tree type)
>>   static bool
>>   arm_function_in_section_p (tree decl, section *section)
>>   {
>> -  /* We can only be certain about functions defined in the same
>> -     compilation unit.  */
>> -  if (!TREE_STATIC (decl))
>> -    return false;
>> -
>> -  /* Make sure that SYMBOL always binds to the definition in this
>> -     compilation unit.  */
>> -  if (!targetm.binds_local_p (decl))
>> +  /* We can only be certain about the prevailing symbol definition.  */
>> +  if (!decl_binds_to_current_def_p (decl))
>>       return false;
>>
>>     /* If DECL_SECTION_NAME is set, assume it is trustworthy. */
>>
>>
>
> Hi,
>
> Did a bootstrap and a full regression run on arm-none-linux-gnueabihf,
> No new regressions found. Some previously failing tests in libstdc++ 
> started to fail differently, for example:
>
> < ERROR: 22_locale/num_get/get/wchar_t/2.cc: can't read 
> "additional_sources": no such variable for " dg-do 22 run { xfail 
> lax_strtof\
> p } "
> < UNRESOLVED: 22_locale/num_get/get/wchar_t/2.cc: can't read 
> "additional_sources": no such variable for " dg-do 22 run { xfail lax_s\
> trtofp } "
> ---
> > ERROR: 22_locale/num_get/get/wchar_t/2.cc: can't read 
> "et_cache(uclibc,value)": no such element in array for " dg-do 22 run 
> { xfai\
> l lax_strtofp } "
> > UNRESOLVED: 22_locale/num_get/get/wchar_t/2.cc: can't read 
> "et_cache(uclibc,value)": no such element in array for " dg-do 22 run {\
>  xfail lax_strtofp } "
>
>
> But I think it is okay.
>
> Kind regards,
> Alex
>
>

Hi,
Ping. Could someone, please approve Richard's patch?
This issue needs fixing.
Kind regards,
Alex

>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>> index 7bf5b4d..777230e 100644
>>> --- a/gcc/config/arm/arm.c
>>> +++ b/gcc/config/arm/arm.c
>>> @@ -6392,14 +6392,8 @@ arm_set_default_type_attributes (tree type)
>>>   static bool
>>>   arm_function_in_section_p (tree decl, section *section)
>>>   {
>>> -  /* We can only be certain about functions defined in the same
>>> -     compilation unit.  */
>>> -  if (!TREE_STATIC (decl))
>>> -    return false;
>>> -
>>> -  /* Make sure that SYMBOL always binds to the definition in this
>>> -     compilation unit.  */
>>> -  if (!targetm.binds_local_p (decl))
>>> +  /* We can only be certain about the prevailing symbol definition.  */
>>> +  if (!decl_binds_to_current_def_p (decl))
>>>       return false;
>>>
>>>     /* If DECL_SECTION_NAME is set, assume it is trustworthy. */
>>>
>>>


Sorry to have missed this - I've also been traveling recently which has 
made it harder with patch traffic - this is OK if no regressions.

Please apply with an appropriate Changelog.

regressions
Ramana

On 05/03/15 15:28, Ramana Radhakrishnan wrote:
>>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>>> index 7bf5b4d..777230e 100644
>>>> --- a/gcc/config/arm/arm.c
>>>> +++ b/gcc/config/arm/arm.c
>>>> @@ -6392,14 +6392,8 @@ arm_set_default_type_attributes (tree type)
>>>>    static bool
>>>>    arm_function_in_section_p (tree decl, section *section)
>>>>    {
>>>> -  /* We can only be certain about functions defined in the same
>>>> -     compilation unit.  */
>>>> -  if (!TREE_STATIC (decl))
>>>> -    return false;
>>>> -
>>>> -  /* Make sure that SYMBOL always binds to the definition in this
>>>> -     compilation unit.  */
>>>> -  if (!targetm.binds_local_p (decl))
>>>> +  /* We can only be certain about the prevailing symbol definition.  */
>>>> +  if (!decl_binds_to_current_def_p (decl))
>>>>        return false;
>>>>
>>>>      /* If DECL_SECTION_NAME is set, assume it is trustworthy. */
>>>>
>>>>
>
> Sorry to have missed this - I've also been traveling recently which has
> made it harder with patch traffic - this is OK if no regressions.
>
> Please apply with an appropriate Changelog.
>
> regressions
> Ramana
>
>
Hi,
Committed as r221220 and fixed ChangeLog entry in r221234.
Sorry for claiming the patch for myself.
Kind regards,
Alex