[cfe-dev] Possible bug in check::RegionChanges
Artem Dergachev via cfe-dev
cfe-dev at lists.llvm.org
Tue Sep 20 08:53:31 PDT 2016
Aha, so i was wondering what's so expensive about computing the lists of
changed regions if they need to be computed for pointer escape anyway.
That explains: when the doc was written, there used to be no pointer
escape callback at all.
From now, we may:
(1) Remove the callback completely and lose nothing.
(2) Try to gain performance by fixing this callback.
(3) Try to gain even more performance by adding
wantsPointerEscapeUpdate() callback.
Because performance is good for us, it'd probably be great if someone
(me(?)) tries to measure the actual performance impact of approaches
(1)-(3).
That said, from reading the code i'm still not convinced that there's
much performance gain. Lists of changed regions are filled automatically
alongside with traversing the store, does anybody know what's the hard
part here anyway? Or was it a premature optimization from the very
beginning?
On 9/20/16 3:00 AM, Devin Coughlin via cfe-dev wrote:
>
>> On Sep 19, 2016, at 10:33 AM, Krzysztof Wiśniewski via cfe-dev
>> <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>>
>> Hello,
>>
>> I've been working on new checker that registers for region change
>> update. Together with Artem Dergachev, we noticed that the function
>> /wantsRegionChangeUpdate /never seems to be called. The heavy work
>> necessary to fill data structures for /checkRegionChanges /is being
>> done regardless of it in /ProgramState::invalidateRegionsImpl./ The
>> documentation for /checkRegionChanges/ says the following:
>> "Note that this callback will not be invoked unless
>> wantsRegionChangeUpdate returns true"
>> and for /wantsRegionChangeUpdate:/
>> "Since it is not necessarily cheap to compute which regions are being
>> changed, this allows the analyzer core to skip the more expensive
>> /checkRegionChanges/ when no checkers are tracking any state." /
>> /
>>
>> My question is whether someone can confirm that this is indeed an
>> invalid behavior? For me it looks like it used to work differently
>> and at some point someone changed the logic rendering
>> /wantsRegionChangeUpdate /useless.
>
> Interesting. It looks like Anna removed the call in to
> wantsRegionChangeUpdate() in r170625 when adding
> the checkPointerEscape callback. It also looks to me like the code
> path in ProgramState::invalidateRegionsImpl that doesn’t call
> processRegionChanges() may be completely dead.
>
>> If this indeed is a bug, I suggest we might wish to consult
>> /CheckerManager / in /ProgramState::invalidateRegionsImpl/ //whether
>> there is some one checker registered for an update and only then
>> prepare the input data in order to improve performance.
>
> I think we would still need to call invalidateRegions() on the store
> manager to actually invalidate the transitively reachable regions, so
> any savings would be in not filling in the ‘out’ parameters
> TopLevelInvalidated and Invalidated. I’m not sure skipping this will
> actually result in any significant performance gains. You could try it
> and see — but I wonder if the better thing to do here might be to
> remove the wantsRegionChangeUpdate callback from the checker API.
>
> Devin
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
More information about the cfe-dev
mailing list