1487622 - Enabling the clang plugin impact codegen

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Mike Hommey [:glandium]]

I thought the clang plugin was supposed to only be doing static analysis, but it looks like it's also changing codegen to some extent.

It would be good to figure out what and why.

Comment 1

[Mike Hommey [:glandium]]

Err, missed the most important information: see how bug 1487330 was backed out because of this:
https://treeherder.mozilla.org/logviewer.html#?job_id=196782297&repo=mozilla-inbound

Comment 2

[Mike Hommey [:glandium]]

FWIW, I relanded bug 1487330 with a change to disable the clang plugin (which wasn't enabled before the change). This should be reproducible on try by removing `unset CLANG_PLUGIN` in build/unix/mozconfig.asan.

Comment 3

[Mike Hommey [:glandium]]

Okay, so even emptying Check.inc triggers it. WTF.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc387ac5ddc69f7ba8f3cd76ad7490c81c9bf204

Comment 4

[Mike Hommey [:glandium]]

Turns out that what causes the difference is enabling the plugin annotations. That is, this change (https://hg.mozilla.org/try/diff/f62a264f8915/mfbt/Attributes.h) is enough to cause the problem *without* the plugin being enabled at all.

Comment 5

[Mike Hommey [:glandium]]

So the good news is that the leak actually exists without the plugin annotations, but the codegen differences that the plugin annotations trigger change its signature such that the whitelist doesn't match it.

Comment 6

[Mike Hommey [:glandium]]

I was able to make the plugin work while removing the annotations. Ideally, we'd rely on a landed version of https://reviews.llvm.org/D31338 in the long term, using plain attributes instead of annotations, instead of forcefully removing annotations at an early stage from the plugin.

My WIP is https://hg.mozilla.org/try/rev/484e3dd69d71b7d301c7dcfddd24acfbecfede31 and it's only a first step wrt what I want to do in the end, but it's a proof that this can work.

However, I do have a problem with it, in that the MOZ_CAN_RUN_SCRIPT that is commented in https://hg.mozilla.org/try/rev/484e3dd69d71b7d301c7dcfddd24acfbecfede31#l6.13 is never passed to CustomAttributesMatcher::run. I tried various things, but none worked out (that's why there are weird things with addMatcher, that was one particular attempt). Somehow, we're not using this pattern in actual code, so just commenting in the test allowed the build to go through.

I haven't found a matcher that makes things work for that particular lambda-attached decl. Andi, do you have any ideas?

Comment 7

[Mike Hommey [:glandium]]

I got it to work by adding a matcher on lambdaExpr, and in the callback, call getCallOperator for that lambda, and do the same as what CustomAttributesMatcher::run does with the result of getCallOperator. http://clang.llvm.org/docs/LibASTMatchersReference.html doesn't show a traverval matcher for getCallOperator.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1487622 - Refactor the clang plugin wrt attributes

- We forcefully remove annotations from the AST so that they don't end
up impacting codegen.
- We change the API such that we use identifiers instead of strings,
reducing the chances of typo errors.

Comment 9

[Andi [:andi]]

Comment on attachment 9007955 [details]
Bug 1487622 - Refactor the clang plugin wrt attributes

Andi-Bogdan Postelnicu [:andi] has approved the revision.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3118f787e33
Refactor the clang plugin wrt attributes r=andi

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e3118f787e33