Message ID | CAEe8uEC+fxjSXCSpPwbTzH31EX4sk7b2E-fWdu8yJaJPiPDU4w@mail.gmail.com |
---|---|
State | New |
Headers | show |
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)
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)