[GOOGLE] Check conditions before calling varpool_node

This patch fixed google bug entry 6124850.

The usage of varpool_node has some restrictions on the corresponding var decl.
In LIPO mode function notice_global_symbol may call varpool_node with a decl
that doesn't satisfy these restrictions since the function notice_global_symbol
can be directly or indirectly called from anywhere. So we need to check if
a decl can be safely passed into varpoo_node before calling it.

Tested by ./buildit with targets x86-64 and power64 without regression.

OK for google branches?

thanks
Carrot


2013-05-09  Guozhi Wei  <carrot@google.com>

        varasm.c (notice_global_symbol): Check conditions before calling
        varpool_node.

This is not correct. current_module_id is used only in FE parsing.

The real question is why the decl is created, neither static nor external?

David

On Thu, May 9, 2013 at 11:39 AM, Carrot Wei <carrot@google.com> wrote:
> This patch fixed google bug entry 6124850.
>
> The usage of varpool_node has some restrictions on the corresponding var decl.
> In LIPO mode function notice_global_symbol may call varpool_node with a decl
> that doesn't satisfy these restrictions since the function notice_global_symbol
> can be directly or indirectly called from anywhere. So we need to check if
> a decl can be safely passed into varpoo_node before calling it.
>
> Tested by ./buildit with targets x86-64 and power64 without regression.
>
> OK for google branches?
>
> thanks
> Carrot
>
>
> 2013-05-09  Guozhi Wei  <carrot@google.com>
>
>         varasm.c (notice_global_symbol): Check conditions before calling
>         varpool_node.
>
>
> Index: varasm.c
> ===================================================================
> --- varasm.c (revision 198726)
> +++ varasm.c (working copy)
> @@ -1515,13 +1515,29 @@
>        || !MEM_P (DECL_RTL (decl)))
>      return;
>
> -  if (L_IPO_COMP_MODE
> -      && ((TREE_CODE (decl) == FUNCTION_DECL
> -           && cgraph_is_auxiliary (decl))
> -          || (TREE_CODE (decl) == VAR_DECL
> -              && varpool_is_auxiliary (varpool_node (decl)))))
> -    return;
> +  if (L_IPO_COMP_MODE)
> +    {
> +      if (TREE_CODE (decl) == FUNCTION_DECL && cgraph_is_auxiliary (decl))
> + return;
>
> +      if (TREE_CODE (decl) == VAR_DECL)
> + {
> +  /* Varpool_node can only accept var decl with flags
> +     (TREE_STATIC (decl) || DECL_EXTERNAL (decl))
> +     For decl without these flags, we need to
> +     check if it is auxiliary manually.  */
> +  if (!(TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
> +    {
> +      /* If a new varpool_node can be created,
> +         the module id is current_module_id.  */
> +      if (current_module_id != primary_module_id)
> + return;
> +    }
> +  else if (varpool_is_auxiliary (varpool_node (decl)))
> +    return;
> + }
> +    }
> +
>    /* We win when global object is found, but it is useful to know about weak
>       symbol as well so we can produce nicer unique names.  */
>    if (DECL_WEAK (decl) || DECL_ONE_ONLY (decl) || flag_shlib)

On Thu, May 9, 2013 at 12:53 PM, Xinliang David Li <davidxl@google.com> wrote:
> This is not correct. current_module_id is used only in FE parsing.
>
Suppose the var decl has correct flags and varpool_node can accept it,
a new varpool_node will be created for it, the module_id for the new node
is set to current_module_id. And in function varpool_is_auxiliary the new
node's module_id is compared with primary_module_id. So this code
exactly simulate the behavior of varpool_is_auxiliary (varpool_node (decl)).

> The real question is why the decl is created, neither static nor external?
>
The decl is created in function dw2_output_indirect_constant_1,
it has the following contents

(gdb) p debug_tree (decl)
 <var_decl 0xf6300540 DW.ref.__gxx_personality_v0
    type <pointer_type 0xf60907e0
        type <void_type 0xf6090780 void type_6 VOID
            align 8 symtab 0 alias set -1 canonical type 0xf6090780
            pointer_to_this <pointer_type 0xf60907e0>>
        public unsigned type_6 DI
        size <integer_cst 0xf5fe0640 constant 64>
        unit size <integer_cst 0xf5fe0660 constant 8>
        align 64 symtab 0 alias set 3 canonical type 0xf60907e0
        pointer_to_this <pointer_type 0xf6092940> reference_to_this
<reference_type 0xf66714a0>>
    readonly asm_written public unsigned ignored weak DI file (null)
line 0 col 0 size <integer_cst 0xf5fe0640 64> unit size <integer_cst
0xf5fe0660 8>
    align 64 initial <var_decl 0xf6300540 DW.ref.__gxx_personality_v0>
    (mem/f/c:DI (symbol_ref/i:DI ("DW.ref.__gxx_personality_v0")
[flags 0x2] <var_decl 0xf6300540 DW.ref.__gxx_personality_v0>) [3
DW.ref.__gxx_personality_v0+0 S8 A64])>

Function dw2_output_indirect_constant_1 creates a new decl with property of
either PUBLIC or STATIC. Is a PUBLIC but not STATIC var decl legal?

The call chain of this failure is
dw2_output_indirect_constant_1 -> assemble_variable ->
notice_global_symbol -> varpool_node

The last call notice_global_symbol -> varpool_node is added by lipo, before that
these functions can't call into varpool_node. So could it because the original
implementation of these functions didn't consider the restrictions of
varpool_node
since it couldn't be called from there?

thanks
Carrot

> David
>
> On Thu, May 9, 2013 at 11:39 AM, Carrot Wei <carrot@google.com> wrote:
>> This patch fixed google bug entry 6124850.
>>
>> The usage of varpool_node has some restrictions on the corresponding var decl.
>> In LIPO mode function notice_global_symbol may call varpool_node with a decl
>> that doesn't satisfy these restrictions since the function notice_global_symbol
>> can be directly or indirectly called from anywhere. So we need to check if
>> a decl can be safely passed into varpoo_node before calling it.
>>
>> Tested by ./buildit with targets x86-64 and power64 without regression.
>>
>> OK for google branches?
>>
>> thanks
>> Carrot
>>
>>
>> 2013-05-09  Guozhi Wei  <carrot@google.com>
>>
>>         varasm.c (notice_global_symbol): Check conditions before calling
>>         varpool_node.
>>
>>
>> Index: varasm.c
>> ===================================================================
>> --- varasm.c (revision 198726)
>> +++ varasm.c (working copy)
>> @@ -1515,13 +1515,29 @@
>>        || !MEM_P (DECL_RTL (decl)))
>>      return;
>>
>> -  if (L_IPO_COMP_MODE
>> -      && ((TREE_CODE (decl) == FUNCTION_DECL
>> -           && cgraph_is_auxiliary (decl))
>> -          || (TREE_CODE (decl) == VAR_DECL
>> -              && varpool_is_auxiliary (varpool_node (decl)))))
>> -    return;
>> +  if (L_IPO_COMP_MODE)
>> +    {
>> +      if (TREE_CODE (decl) == FUNCTION_DECL && cgraph_is_auxiliary (decl))
>> + return;
>>
>> +      if (TREE_CODE (decl) == VAR_DECL)
>> + {
>> +  /* Varpool_node can only accept var decl with flags
>> +     (TREE_STATIC (decl) || DECL_EXTERNAL (decl))
>> +     For decl without these flags, we need to
>> +     check if it is auxiliary manually.  */
>> +  if (!(TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
>> +    {
>> +      /* If a new varpool_node can be created,
>> +         the module id is current_module_id.  */
>> +      if (current_module_id != primary_module_id)
>> + return;
>> +    }
>> +  else if (varpool_is_auxiliary (varpool_node (decl)))
>> +    return;
>> + }
>> +    }
>> +
>>    /* We win when global object is found, but it is useful to know about weak
>>       symbol as well so we can produce nicer unique names.  */
>>    if (DECL_WEAK (decl) || DECL_ONE_ONLY (decl) || flag_shlib)

On Thu, May 9, 2013 at 4:46 PM, Carrot Wei <carrot@google.com> wrote:
> On Thu, May 9, 2013 at 12:53 PM, Xinliang David Li <davidxl@google.com> wrote:
>> This is not correct. current_module_id is used only in FE parsing.
>>
> Suppose the var decl has correct flags and varpool_node can accept it,
> a new varpool_node will be created for it, the module_id for the new node
> is set to current_module_id. And in function varpool_is_auxiliary the new
> node's module_id is compared with primary_module_id. So this code
> exactly simulate the behavior of varpool_is_auxiliary (varpool_node (decl)).

yes -- that is implementation detail. Ideally, all post-parsing decls
should directly use 'primary_module_id'.

>
>> The real question is why the decl is created, neither static nor external?
>>
> The decl is created in function dw2_output_indirect_constant_1,
> it has the following contents
>
> (gdb) p debug_tree (decl)
>  <var_decl 0xf6300540 DW.ref.__gxx_personality_v0
>     type <pointer_type 0xf60907e0
>         type <void_type 0xf6090780 void type_6 VOID
>             align 8 symtab 0 alias set -1 canonical type 0xf6090780
>             pointer_to_this <pointer_type 0xf60907e0>>
>         public unsigned type_6 DI
>         size <integer_cst 0xf5fe0640 constant 64>
>         unit size <integer_cst 0xf5fe0660 constant 8>
>         align 64 symtab 0 alias set 3 canonical type 0xf60907e0
>         pointer_to_this <pointer_type 0xf6092940> reference_to_this
> <reference_type 0xf66714a0>>
>     readonly asm_written public unsigned ignored weak DI file (null)
> line 0 col 0 size <integer_cst 0xf5fe0640 64> unit size <integer_cst
> 0xf5fe0660 8>
>     align 64 initial <var_decl 0xf6300540 DW.ref.__gxx_personality_v0>
>     (mem/f/c:DI (symbol_ref/i:DI ("DW.ref.__gxx_personality_v0")
> [flags 0x2] <var_decl 0xf6300540 DW.ref.__gxx_personality_v0>) [3
> DW.ref.__gxx_personality_v0+0 S8 A64])>
>
> Function dw2_output_indirect_constant_1 creates a new decl with property of
> either PUBLIC or STATIC. Is a PUBLIC but not STATIC var decl legal?
>
> The call chain of this failure is
> dw2_output_indirect_constant_1 -> assemble_variable ->
> notice_global_symbol -> varpool_node
>
> The last call notice_global_symbol -> varpool_node is added by lipo, before that
> these functions can't call into varpool_node. So could it because the original
> implementation of these functions didn't consider the restrictions of
> varpool_node
> since it couldn't be called from there?

I think the code :

 if (TREE_PUBLIC (id))
    {
      TREE_PUBLIC (decl) = 1;
      make_decl_one_only (decl, DECL_ASSEMBLER_NAME (decl));
      if (USE_LINKONCE_INDIRECT)
DECL_VISIBILITY (decl) = VISIBILITY_HIDDEN;
    }
  else
    TREE_STATIC (decl) = 1;


is wrong. The TREE_STATIC should be in the fall through path. Try
submit a patch to trunk to solicit comments.

thanks,

David

>
> thanks
> Carrot
>
>> David
>>
>> On Thu, May 9, 2013 at 11:39 AM, Carrot Wei <carrot@google.com> wrote:
>>> This patch fixed google bug entry 6124850.
>>>
>>> The usage of varpool_node has some restrictions on the corresponding var decl.
>>> In LIPO mode function notice_global_symbol may call varpool_node with a decl
>>> that doesn't satisfy these restrictions since the function notice_global_symbol
>>> can be directly or indirectly called from anywhere. So we need to check if
>>> a decl can be safely passed into varpoo_node before calling it.
>>>
>>> Tested by ./buildit with targets x86-64 and power64 without regression.
>>>
>>> OK for google branches?
>>>
>>> thanks
>>> Carrot
>>>
>>>
>>> 2013-05-09  Guozhi Wei  <carrot@google.com>
>>>
>>>         varasm.c (notice_global_symbol): Check conditions before calling
>>>         varpool_node.
>>>
>>>
>>> Index: varasm.c
>>> ===================================================================
>>> --- varasm.c (revision 198726)
>>> +++ varasm.c (working copy)
>>> @@ -1515,13 +1515,29 @@
>>>        || !MEM_P (DECL_RTL (decl)))
>>>      return;
>>>
>>> -  if (L_IPO_COMP_MODE
>>> -      && ((TREE_CODE (decl) == FUNCTION_DECL
>>> -           && cgraph_is_auxiliary (decl))
>>> -          || (TREE_CODE (decl) == VAR_DECL
>>> -              && varpool_is_auxiliary (varpool_node (decl)))))
>>> -    return;
>>> +  if (L_IPO_COMP_MODE)
>>> +    {
>>> +      if (TREE_CODE (decl) == FUNCTION_DECL && cgraph_is_auxiliary (decl))
>>> + return;
>>>
>>> +      if (TREE_CODE (decl) == VAR_DECL)
>>> + {
>>> +  /* Varpool_node can only accept var decl with flags
>>> +     (TREE_STATIC (decl) || DECL_EXTERNAL (decl))
>>> +     For decl without these flags, we need to
>>> +     check if it is auxiliary manually.  */
>>> +  if (!(TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
>>> +    {
>>> +      /* If a new varpool_node can be created,
>>> +         the module id is current_module_id.  */
>>> +      if (current_module_id != primary_module_id)
>>> + return;
>>> +    }
>>> +  else if (varpool_is_auxiliary (varpool_node (decl)))
>>> +    return;
>>> + }
>>> +    }
>>> +
>>>    /* We win when global object is found, but it is useful to know about weak
>>>       symbol as well so we can produce nicer unique names.  */
>>>    if (DECL_WEAK (decl) || DECL_ONE_ONLY (decl) || flag_shlib)