Require libclang 4.0+ for stylo.

RESOLVED FIXED in Firefox 67

Status

defect
P3
normal
RESOLVED FIXED
2 years ago
20 days ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox67 fixed)

Details

(Whiteboard: [stylo])

Attachments

(1 attachment)

That would make easier to land bug 1393230, and remove other stylo hacks.

clang 4.0 is already stable (and has been for a while) and it is pretty much everywhere.
Hi Ralph, do you know how easy / hard / doable would this be?
Flags: needinfo?(giles)
There is talk of enabling Stylo by default in Nightly in the next few days. If Stylo becomes the Firefox default, then requiring Clang 4.0 for Stylo effectively means bumping Firefox's minimum Clang version from 3.9 to 4.0. I suppose we could still support Clang 3.9 with no Stylo. But we'd have to change the "base toolchains" builds to disable Stylo. And once we drop no Stylo as a supported build configuration, that means we've bumped the minimum Clang dependency to 4.0.
(In reply to Gregory Szorc [:gps] from comment #2)
> There is talk of enabling Stylo by default in Nightly in the next few days.
> If Stylo becomes the Firefox default, then requiring Clang 4.0 for Stylo
> effectively means bumping Firefox's minimum Clang version from 3.9 to 4.0. I
> suppose we could still support Clang 3.9 with no Stylo. But we'd have to
> change the "base toolchains" builds to disable Stylo. And once we drop no
> Stylo as a supported build configuration, that means we've bumped the
> minimum Clang dependency to 4.0.

Sure, I just supposed it'd be worth to have a bug on file. Please feel free to punt on this until you think it's suitable.

I think it'd be pretty nice for Stylo because that means we can remove a bunch of hacks, but of course I don't know whether that outweights bumping the minimum clang dependency, that's why I asked :)
I don't think it's particularly hard, assuming there are no regressions. Nathan did the necessary work to our build_clang script, I think in bug 1377214.

We already effectively require libclang 4.0.1 for bindgen on macOS because earlier versions crash there. We just allow compiling the C++ side with an earlier version.
Flags: needinfo?(giles)
It seems kinda late in the Nightly 57 cycle to bump compiler version if we don't need a particular clang bug fix.
Priority: -- → P4
Whiteboard: [stylo]
status-firefox57=wontfix unless someone thinks this bug should block 57
(In reply to Gregory Szorc [:gps] from comment #2)
> There is talk of enabling Stylo by default in Nightly in the next few days.
> If Stylo becomes the Firefox default, then requiring Clang 4.0 for Stylo
> effectively means bumping Firefox's minimum Clang version from 3.9 to 4.0. I
> suppose we could still support Clang 3.9 with no Stylo. But we'd have to
> change the "base toolchains" builds to disable Stylo. And once we drop no
> Stylo as a supported build configuration, that means we've bumped the
> minimum Clang dependency to 4.0.

We will likely remove Gecko's old style system in Nightly 59 (stylo-everywhere bug 1395112). We can then update from libclang 3.9.0 to something newer like 4.0.
Depends on: stylo-everywhere
Priority: P4 → P3

Updated

Last year
Product: Core → Firefox Build System
Assignee

Comment 8

5 months ago

Mike, we still build the base toolchains job with clang 3.9 only for bindgen (since we use GCC for the C++ code IIUC). I don't see anything else using linux64-clang-3.9, is that right?

If so, can we update it to, let's say, clang 5.0? (4.0 would be ok too I guess). I'm happy to figure out the way to tweak the json file in build/build-clang/clang-3.9-linux64.json if you cannot / don't want to do it.

Flags: needinfo?(mh+mozilla)

I actually want to setup builds with the minimum clang we claim to support which is 3.9 at the moment (independently of bindgen). It may or may not be worthwhile to update the minimum clang version we claim to support, but we should at least check out which one(s) work.

Flags: needinfo?(mh+mozilla)
Assignee

Comment 10

5 months ago

Huh, as it turns out, this is blocking some changes I want to do to the style system. We have already so many hacks to workaround libclang 3.9 bugs that are fixed in 4.0 that I took a stab at this.

With 3.9:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1122db9ff531e4c626d9048e27d88e8cc8bc511

With 4.0:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f530666dc2308502b9d3257ce1220d1ff34751e4 (not done yet, but has finished linking libxul, so it did build).

Blocks: 1523071
Assignee

Updated

5 months ago
Assignee: nobody → emilio
Assignee

Comment 11

5 months ago

Note that we only use this for libclang at the moment, since
base-toolchains is built with gcc.

libclang 3.9 has a bug that makes bindgen unable to distinguish some typedefs
from the underlying type, which matters for bug 1523071.

We have had quite a few workarounds for this bug and I don't really want to add
more, since in this case it is non-trivial. I think requiring libclang 4.0+ is
reasonable at this point.

Assignee

Updated

5 months ago
Depends on: 1525760

Comment 12

5 months ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6a025216838
Update minimum clang version to 4.0. r=glandium

Comment 13

5 months ago
bugherder
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee

Updated

4 months ago
Depends on: 1530956
Assignee

Updated

3 months ago
See Also: → 1528848
Blocks: 1557547
You need to log in before you can comment on or make changes to this bug.