win: Make oilpan plugin work better with delayed template parsing.

win: Make oilpan plugin work better with delayed template parsing.

On Windows, clang-cl only parses template methods that are
referenced (-fdelayed-template-parsing), so members traced from
unreferenced template methods weren't seen by the plugin, which
caused it to warn about them.

Explicitly parse delayed template methods that look like they
are trace methods and that aren't in system headers to work around
this. This is based on Kim Grasman's patch for the IWYU project:
https://code.google.com/p/include-what-you-use/source/detail?r=566

This fixes 3 of the 4 failing oilpan plugin tests on Windows.

BUG=486571
NOPRESUBMIT=true

Committed: https://crrev.com/28c1b1099fb09782841d891ca43e83e71cd8dfc6
Cr-Commit-Position: refs/heads/master@{#330247}

[Nico, 2015-05-15 22:36:10]

thakis@chromium.org changed reviewers:
+ hans@chromium.org

[Nico, 2015-05-15 22:36:10]

(rnk: FYI since you suggested this approach to Kim 9 months ago)

[hans, 2015-05-15 22:43:56]

lgtm

Could we have a test for this that works on all platforms, by putting
-fdelayed-template parsing in its .flags file?

[Reid Kleckner, 2015-05-15 22:44:31]

rnk@chromium.org changed reviewers:
+ rnk@chromium.org

[Reid Kleckner, 2015-05-15 22:44:32]

Seems reasonable. RAV is a pretty heavy class that isn't really meant to be
instantiated many times per program, but by all means, go for it. Clang is the
one that should have a lighter-weight, perhaps non-templated AST traversal
mechanism.

https://codereview.chromium.org/1140443005/diff/1/tools/clang/blink_gc_plugin...
File tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp (right):

https://codereview.chromium.org/1140443005/diff/1/tools/clang/blink_gc_plugin...
tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp:1088:
GetLateParsedFunctionDecls(decl);
Traversing the whole AST might be expensive. Maybe you only want to do this if
DelayedTemplateParsing is true, and then put
'assert(GetLateParsedFunctionDecls(decl).empty())' in the else block?

[Nico, 2015-05-15 22:57:26]

Added a targeted test that tests this when running tests on non-win too.

https://codereview.chromium.org/1140443005/diff/1/tools/clang/blink_gc_plugin...
File tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp (right):

https://codereview.chromium.org/1140443005/diff/1/tools/clang/blink_gc_plugin...
tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp:1088:
GetLateParsedFunctionDecls(decl);
On 2015/05/15 22:44:32, Reid Kleckner wrote:
> Traversing the whole AST might be expensive. Maybe you only want to do this if
> DelayedTemplateParsing is true, and then put
> 'assert(GetLateParsedFunctionDecls(decl).empty())' in the else block?

We always build with asserts enabled, so that wouldn't be any faster. I made
this early-return if DelayedTemplateParsing isn't set in LangOpts instead.

[Nico, 2015-05-15 22:59:27]

The CQ bit was checked by thakis@chromium.org

[Nico, 2015-05-15 22:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from hans@chromium.org
Link to the patchset: https://codereview.chromium.org/1140443005/#ps20001
(title: "comments")

[commit-bot: I haz the power, 2015-05-15 22:59:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140443005/20001

[Nico, 2015-05-15 23:03:57]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2015-05-15 23:11:44]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-05-15 23:14:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140443005/20001

[commit-bot: I haz the power, 2015-05-15 23:26:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-15 23:26:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Nico, 2015-05-16 00:26:30]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-05-16 00:28:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140443005/20001

[commit-bot: I haz the power, 2015-05-16 00:31:41]

Committed patchset #2 (id:20001)

[sof, 2015-05-16 05:45:18]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-05-16 05:45:19]

Not clear from the CL, have you tested this on the Blink codebase with Oilpan
enabled?

When experimenting with something very similar to this locally, I ran into
various Parser asserts (still alive TemplateIdAnnotations or some such.)

[Nico, 2015-05-16 05:50:05]

On 2015/05/16 05:45:19, sof wrote:
> Not clear from the CL, have you tested this on the Blink codebase with Oilpan
> enabled?
> 
> When experimenting with something very similar to this locally, I ran into
> various Parser asserts (still alive TemplateIdAnnotations or some such.)

Me too; see bug :-P

[Nico, 2015-05-16 06:15:11]

(I'll debug that tomorrow)
On May 15, 2015 10:50 PM, <thakis@chromium.org> wrote:

> On 2015/05/16 05:45:19, sof wrote:
>
>> Not clear from the CL, have you tested this on the Blink codebase with
>> Oilpan
>> enabled?
>>
>
>  When experimenting with something very similar to this locally, I ran into
>> various Parser asserts (still alive TemplateIdAnnotations or some such.)
>>
>
> Me too; see bug :-P
>
> https://codereview.chromium.org/1140443005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sof, 2015-05-16 06:16:14]

On 2015/05/16 05:50:05, Nico wrote:
> On 2015/05/16 05:45:19, sof wrote:
> > Not clear from the CL, have you tested this on the Blink codebase with
Oilpan
> > enabled?
> > 
> > When experimenting with something very similar to this locally, I ran into
> > various Parser asserts (still alive TemplateIdAnnotations or some such.)
> 
> Me too; see bug :-P

ok, if you need help to check/verify/look into stuff, i'm happy (to try) to lend
a hand.

I couldn't see any additional cleanups being done for something like this in the
IWYU sources.

[commit-bot: I haz the power, 2015-05-18 11:29:51]

Patchset 2 (id:??) landed as
https://crrev.com/28c1b1099fb09782841d891ca43e83e71cd8dfc6
Cr-Commit-Position: refs/heads/master@{#330247}