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)).
> 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 <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)
