[google] fix global vars incorrectly marked as read-only in LIPO mode

This change is for google-main branch only.

This bug is triggered by the bad interaction b/w LIPO and some new IPA passes introduced in 4.6 (namely visibility analysis): one variable in comdat section is incorrectly marked as read-only by ipa_discover_readonly_nonaddressable_vars(). The folding pass in inliner then propagates it's
initialization value to the use. This variable is in the comdat section. In the primary module, it's not referenced and read/write in auxiliary module.
In the varpool node for the primary module, it's finalized and marked as not needed.
 ==> varpool_externally_visible_p() will not be called
 ==> externally_visible property is not set.
  ==> ipa_discover_readonly_nonaddressable_vars() will ONLY look at address_taken and written bit locally

This only happens in template instantiation (otherwise, the finally bit not set for the decl in the primary module).

This bug seems to be LIPO only as only in LIPO mode, we create varpool entries eagerly.

[davidxl, 2011-07-21 18:09:34]

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.

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.

[xur, 2011-07-21 18:32:20]

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

>
> 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/
>

[davidxl, 2011-07-21 18:59:30]

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/
>>
>

[xur, 2011-07-21 19:27:15]

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/
>>>
>>
>

[davidxl, 2011-07-21 19:35:27]

On Thu, Jul 21, 2011 at 12:27 PM, Rong Xu <xur@google.com> wrote:
> 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

Probably not -- only non referenced comdat vars will be marked as not needed.

David

> 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/
>>>>
>>>
>>
>

[xur, 2011-07-21 19:44:08]

wait a second. this still won't work if we disable the whole-program
pass (like my original change, the visibility analysis change won't
kick in). we also need to change varpool_node_link() to merge the
local attribute.

-Rong

On Thu, Jul 21, 2011 at 12:35 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, Jul 21, 2011 at 12:27 PM, Rong Xu <xur@google.com> wrote:
>> 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
>
> Probably not -- only non referenced comdat vars will be marked as not needed.
>
> David
>
>> 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/
>>>>>
>>>>
>>>
>>
>

[xur, 2011-07-21 20:45:27]

Please review the new patch attached to this email.

thanks,

-Rong

On Thu, Jul 21, 2011 at 12:44 PM, Rong Xu <xur@google.com> wrote:
> wait a second. this still won't work if we disable the whole-program
> pass (like my original change, the visibility analysis change won't
> kick in). we also need to change varpool_node_link() to merge the
> local attribute.
>
> -Rong
>
> On Thu, Jul 21, 2011 at 12:35 PM, Xinliang David Li <davidxl@google.com>
wrote:
>> On Thu, Jul 21, 2011 at 12:27 PM, Rong Xu <xur@google.com> wrote:
>>> 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
>>
>> Probably not -- only non referenced comdat vars will be marked as not needed.
>>
>> David
>>
>>> 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/
>>>>>>
>>>>>
>>>>
>>>
>>
>

[davidxl, 2011-07-21 21:04:33]

Ok.

David

On Thu, Jul 21, 2011 at 1:45 PM, Rong Xu <xur@google.com> wrote:
> Please review the new patch attached to this email.
>
> thanks,
>
> -Rong
>
> On Thu, Jul 21, 2011 at 12:44 PM, Rong Xu <xur@google.com> wrote:
>> wait a second. this still won't work if we disable the whole-program
>> pass (like my original change, the visibility analysis change won't
>> kick in). we also need to change varpool_node_link() to merge the
>> local attribute.
>>
>> -Rong
>>
>> On Thu, Jul 21, 2011 at 12:35 PM, Xinliang David Li <davidxl@google.com>
wrote:
>>> On Thu, Jul 21, 2011 at 12:27 PM, Rong Xu <xur@google.com> wrote:
>>>> 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
>>>
>>> Probably not -- only non referenced comdat vars will be marked as not
needed.
>>>
>>> David
>>>
>>>> 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/
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>