This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] PR rtl-optimization/32219: optimizer causes wrong code in pic/hidden/weak symbol checking


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 <rth@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~
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]