Richard, Your proposed patch fails to bootstrap here on x86_64-apple-darwin14 against the system clang compilers...
g++ -c -g -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-format -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common -DHAVE_CONFIG_H -I. -I. -I../../gcc-5-20150211/gcc -I../../gcc-5-20150211/gcc/. -I../../gcc-5-20150211/gcc/../include -I../../gcc-5-20150211/gcc/../libcpp/include -I/sw/include -I/sw/include -I../../gcc-5-20150211/gcc/../libdecnumber -I../../gcc-5-20150211/gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc-5-20150211/gcc/../libbacktrace -I/sw/include -I/sw/include -o varasm.o -MT varasm.o -MMD -MP -MF ./.deps/varasm.TPo ../../gcc-5-20150211/gcc/varasm.c ... ../../gcc-5-20150211/gcc/varasm.c:6899:9: error: use of undeclared identifier 'resolution_to_local_definition_p' return resolution_to_local_definition_p (vnode->resolution); ^ ../../gcc-5-20150211/gcc/varasm.c:6906:9: error: use of undeclared identifier 'resolution_to_local_definition_p' return resolution_to_local_definition_p (node->resolution); ^ Jack On Tue, Feb 10, 2015 at 4:19 PM, Richard Henderson <r...@redhat.com> wrote: > On 02/07/2015 08:45 AM, H.J. Lu wrote: >> Here is an updated patch. > > This is an annoying comment to differentiate 3 different versions, FYI. > >> @@ -6826,11 +6817,18 @@ default_binds_local_p_1 (const_tree exp, int shlib) >> && (TREE_STATIC (exp) || DECL_EXTERNAL (exp))) >> { >> varpool_node *vnode = varpool_node::get (exp); >> - if (vnode && (resolution_local_p (vnode->resolution) || >> vnode->in_other_partition)) >> - resolved_locally = true; >> - if (vnode >> - && resolution_to_local_definition_p (vnode->resolution)) >> - resolved_to_local_def = true; >> + /* If not building shared library, common or initialized symbols >> + are also resolved locally, regardless they are weak or not if >> + weak_dominate is true. */ >> + if (vnode) >> + { >> + if ((weak_dominate && !shlib && vnode->definition) >> + || vnode->in_other_partition >> + || resolution_local_p (vnode->resolution)) >> + resolved_locally = true; >> + if (resolution_to_local_definition_p (vnode->resolution)) >> + resolved_to_local_def = true; >> + } >> } >> else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp)) >> { > > Not the same test for a function_decl? That's certainly a mistake. > > Indeed, I believe we can now unify these two cases by using the base > symtab_node instead of varpool_node and cgraph_node. > > I'm a bit confused about why we have both resolved_to_local_def vs > resolved_locally. At least within this function, which only cares about > module > locality rather than object file locality. Honza? > >> @@ -6859,9 +6857,15 @@ default_binds_local_p_1 (const_tree exp, int shlib) >> else if (! TREE_PUBLIC (exp)) >> local_p = true; >> /* A variable is local if the user has said explicitly that it will >> - be. */ >> + be and it is resolved or defined locally, not compiling for PIC, >> + not weak or weak_dominate is false. */ >> else if ((DECL_VISIBILITY_SPECIFIED (exp) >> || resolved_to_local_def) >> + && (!weak_dominate >> + || resolved_locally >> + || !flag_pic >> + || !DECL_EXTERNAL (exp) >> + || !DECL_WEAK (exp)) >> && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT) >> local_p = true; > > I think this test is conflating several issues, and as such it's very > confusing. > > As an existing issue, I'm not sure why "specified" visibility is any different > from unspecified visibility. As far as I'm aware, the "specified" bit simply > means that the decl doesn't inherit inherit visibility from the class, or from > the command-line. But once we're this far, the visibility actually applied to > the symbol should be all that matters. > > Unfortunately, this test is 11 years old, from r85167. It came in as part of > the implementation of the visibility attribute for the class. > > I believe the next test should be > > else if (DECL_WEAK (exp) && !resolved_locally) > local_p = false; > > Given the change above, resolved_locally will now be true for (weak && defined > && weak_dominate), and therefore we need not reiterate that test. > > I believe the next test should be dictated by normal non-lto usage like > > extern void bar(void) __attribute__((visibility("hidden"))); > void foo(void) { ... bar(); ...) > > where the user is expecting that the function call make use of a simpler > instruction sequence, probably avoiding an PLT. Therefore > > else if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT) > local_p = true; > > Since we have already excluded undef-weak, we know that any symbol here is > either defined or strong, and non-default visibility must resolve it locally. > >> /* Variables defined outside this object might not be local. */ >> @@ -6881,8 +6885,9 @@ default_binds_local_p_1 (const_tree exp, int shlib) >> else if (shlib) >> local_p = false; >> /* Uninitialized COMMON variable may be unified with symbols >> - resolved from other modules. */ >> + resolved from other modules unless weak_dominate is true. */ >> else if (DECL_COMMON (exp) >> + && !weak_dominate >> && !resolved_locally > > Like the weak test, surely weak_dominate has already been accounted for. > >> @@ -7445,9 +7465,10 @@ default_elf_asm_output_external (FILE *file >> ATTRIBUTE_UNUSED, >> { >> /* We output the name if and only if TREE_SYMBOL_REFERENCED is >> set in order to avoid putting out names that are never really >> - used. */ >> + used. Always output visibility specified in the source. */ >> if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)) >> - && targetm.binds_local_p (decl)) >> + && (DECL_VISIBILITY_SPECIFIED (decl) >> + || targetm.binds_local_p (decl))) >> maybe_assemble_visibility (decl); > > Explain? > > I'm testing the following in the meantime, in order to validate my hypotheses > above. > > > r~ >