On Thu, May 9, 2013 at 4:46 PM, Carrot Wei <car...@google.com> wrote:
> On Thu, May 9, 2013 at 12:53 PM, Xinliang David Li <davi...@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 <car...@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 <car...@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)
