Re: [google] fix global vars incorrectly marked as read-only in LIPO mode (issue4798045)

[Rong Xu <xur at google dot com>]

this is a good point. ipa_discover_readonly_nonaddressable_vars() is
called in two passes. whole-program (whole program visibility
analysis) and static-var. The one in whole-program is ok here as it is
 bundled together with the analysis. the invocation in static-var can
go wrong.

should we add a check for COMDAT flag also? like
if ((vnode->needed || (L_IPO_COMP_MODE && DECL_COMDAT (node->decl)))
    && varpool_node_externally_visible_p) (

Thanks,

-Rong
On Thu, Jul 21, 2011 at 11:59 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, Jul 21, 2011 at 11:32 AM, Rong Xu <xur@google.com> wrote:
>> On Thu, Jul 21, 2011 at 11:09 AM, ?<davidxl@google.com> wrote:
>>>
>>> http://codereview.appspot.com/4798045/diff/1/ipa.c
>>> File ipa.c (right):
>>>
>>> http://codereview.appspot.com/4798045/diff/1/ipa.c#newcode1034
>>> ipa.c:1034: {
>>> Has varpool node linking happened at this point? If not, the new code
>>> here is not excersised.
>>
>> This functions is called in multiple places: local pass and
>> whole_program pass. varpool_node_link is b/w these two passes. the
>> varpool node attribute is set before the use in
>> ipa_discover_readonly_nonaddressable_vars()
>
> So the code relies on the second visibility pass to get things right
> -- if that pass is disabled things can go wrong (if the default
> visibility value when vnode is created is true, LIPO mode will get
> pessemitic result; if the default is false, LIPO mode will still get
> wrong result if only local visiblity pass is run).
>
> A better fix might be simply do not check 'needed' bit for comdat
> varnode in LIPO mode and always run the visibiity checker:
>
> if ((vnode->needed || L_IPO_COMP_MODE)
> ? && varpool_node_externally_visible_p (...
> ?)
>
>
> David
>
>
>>
>>>
>>> http://codereview.appspot.com/4798045/diff/1/ipa.c#newcode1041
>>> ipa.c:1041: vnode->externally_visible = false;
>>> Better to add a simple loop after varpool_node_linking to merge the
>>> attribute in l-ipo.c assuming the linking happens after local visibility
>>> pass.
>>
>> We can merge in the varpool_node_link, but we still need to change
>> here as the attribute will be overwritten here in the whole_program
>> pass.
>>
>>>
>>> http://codereview.appspot.com/4798045/
>>>
>>
>