1332689 - (st-an-3.9) Remove clang <3.9 specific code from the clang-plugin

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

According to :froydnj, Stylo is going to require clang >= 3.9 for running bindgen. If we have this requirement to build our tree anyway, we can increase the minimum supported clang version of our clang-plugin. This would allow us to remove code like: http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/build/clang-plugin/plugin.h#29-52 and simplify the build setup to not check for things like HAVE_NEW_ASTMATCHER_NAMES.

Comment 1

[(no longer active)]

Does this mean people won't be able to build the tree using Apple clang any more?  That seems bad.  Are we planning to run bindgen as part of the build for everyone?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari from comment #1)
> Does this mean people won't be able to build the tree using Apple clang any
> more?  That seems bad.  Are we planning to run bindgen as part of the build
> for everyone?

https://hg.mozilla.org/integration/mozilla-inbound/rev/210b8d21be06#l1.21 is the new code for stylo IIRC (currently only on inbound).

This would probably be merged somehow with 

http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/build/autoconf/clang-plugin.m4#16-28

Comment 3

[Nathan Froyd [:froydnj]]

The way things currently work is that bindgen can use a completely separate compiler/clang installation than the main CC/CXX.  Once stylo lands and is enabled by default, bindgen will be required, so developers will need a clang-$VERSION installation separate from XCode's clang.  (The idea here is that people shouldn't have to check in generated Rust to the tree; bindgen should be able to generate all the files we need during the build process.)  The plan is to have that clang-$VERSION package installed via |mach bootstrap|, and ideally that package will be installed from the same binaries we use in automation.

Sound reasonable?

Comment 4

[Nathan Froyd [:froydnj]]

I'm assuming that we couldn't use Apple's clang with bindgen even if we wanted to, since I suspect the necessary libraries/headers aren't really provided?

Comment 5

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> The way things currently work is that bindgen can use a completely separate
> compiler/clang installation than the main CC/CXX.  Once stylo lands and is
> enabled by default, bindgen will be required, so developers will need a
> clang-$VERSION installation separate from XCode's clang.  (The idea here is
> that people shouldn't have to check in generated Rust to the tree; bindgen
> should be able to generate all the files we need during the build process.) 
> The plan is to have that clang-$VERSION package installed via |mach
> bootstrap|, and ideally that package will be installed from the same
> binaries we use in automation.
> 
> Sound reasonable?

Yes it sounds awesome.  FWIW I'm essentially writing the code that does the last part in bug 1328454.  :-)

(In reply to Nathan Froyd [:froydnj] from comment #4)
> I'm assuming that we couldn't use Apple's clang with bindgen even if we
> wanted to, since I suspect the necessary libraries/headers aren't really
> provided?

Yes that's correct.  (Similarly you can't use it if you want to enable static analysis.)

Comment 6

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #2)
> (In reply to :Ehsan Akhgari from comment #1)
> > Does this mean people won't be able to build the tree using Apple clang any
> > more?  That seems bad.  Are we planning to run bindgen as part of the build
> > for everyone?
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/210b8d21be06#l1.21 is
> the new code for stylo IIRC (currently only on inbound).

Ugh, it sucks that this wasn't caught at review time.  This code isn't even using the same environment variable name.  Sigh. :(

Comment 7

[(no longer active)]

FWIW I'm bringing up the static analysis builders slowly to clang 3.9.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

andi, you might be interested in working on this :-) - Could be some nice cleanup!

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I accidentally wrote patches for this when diagnosing some sccache-dist issue.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1332689 - Remove clang 3.8 compat code.

This is just drive-by, but it confused me. We require at least clang 4 to build
nowadays.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1332689 - Remove clang 3.9 compat code for the clang-plugin.

We require 4.0 to build since bug 1394825.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88ad4b97fe78
Remove clang 3.8 compat code. r=andi

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98470c1a958c
Remove clang 3.9 compat code for the clang-plugin. r=andi

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/88ad4b97fe78
https://hg.mozilla.org/mozilla-central/rev/98470c1a958c