1316537 - Add 64-bit clang-cl toolchain builds to taskcluster task definitions

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Ting-Yu Chou [:ting] (away)]

In bug 1306650 :froydnj added the task for building 32-bit clang-cl. This one is for 64-bit, because ASan needs 64-bit runtime binary for 64-bit Firefox.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Any idea who would do this?

Comment 2

[Nathan Froyd [:froydnj]]

So our current clang-cl build task builds a 32-bit clang-cl, along with 32-bit runtime libraries.  We really want a 64-bit clang-cl, with 32-bit and 64-bit runtime libraries, because our build machines are (AIUI) 64-bit machines anyway.

I *think* what needs to be done for this bug is to adjust the paths and whatnot in build-clang-windows.sh to use the 64-bit cl.exe etc. rather than the 32-bit cl.exe for building, and possibly adjust how clang-cl itself gets built.  Is that right, dmajor?  How have you been building 64-bit clang-cl?

I can do this, or Ting-Yu can do this, I think.

Comment 3

[(Away)]

Yeah, I think it's just a matter of having the right compiler in your path. The only difference in my local workflow is whether I run cmake/ninja from the "VS2015 x64 Native Tools Command Prompt" or the x86 one.

Comment 4

[(Away)]

One tricky thing is that even though 64-bit clang-cl can build for either target (since you can do -m32), I still end up building a separate 32-bit clang-cl just for its asan libaries. I don't know if there's an easier way around that.

Comment 5

[Ting-Yu Chou [:ting] (away)]

I'll take this.

Comment 6

[Ting-Yu Chou [:ting] (away)]

The other platforms runs static analysis and ASan with only 64 bit version, I am not sure why but do we need both 32-bit and 64-bit for Windows?

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Ting-Yu Chou [:ting] from comment #6)
> The other platforms runs static analysis and ASan with only 64 bit version,
> I am not sure why but do we need both 32-bit and 64-bit for Windows?

I would say because our other major platforms (Linux/OSX) are both 64-bit by default, so we focus on 64-bit ports there.  Whereas 32-bit Windows is our primary Windows platform, so we'd want 32-bit static analysis.  But we're also going to be shipping an official 64-bit Windows port soonish, so we'd want to analyze that as well.

That being said, if there a lot of overlap between our 32-bit and 64-bit Windows code, we could probably get away with doing one or the other.  I don't know how much of our Windows code is specific to one architecture, though.

Comment 8

[(Away)]

If we want to have Windows ASan be a full-fledged entry on treeherder running through the complete set of mochitests etc., then I suspect it's going to have to be 64-bit. We would OOM left and right on 32-bit builds.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to David Major [:dmajor] from comment #8)
> If we want to have Windows ASan be a full-fledged entry on treeherder
> running through the complete set of mochitests etc., then I suspect it's
> going to have to be 64-bit. We would OOM left and right on 32-bit builds.

Indeed.  But mochitests are a ways off at this point, AIUI.  At least, talking with folks on the releng side indicated that running a couple of one-off Windows builds on each push is no big deal.  Running the full test suite, under ASan or otherwise, is a different kettle of fish.

Comment 10

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1316537 part 1 - Add 64-bit clang-cl toolchain build.

Review commit: https://reviewboard.mozilla.org/r/93002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93002/

Comment 11

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1316537 part 2 - Update 64-bit clang build configuration for the changes in bug 1306650.

Review commit: https://reviewboard.mozilla.org/r/93004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93004/

Comment 12

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1316537 part 3 - Fix tar package path conversion for local build.

Review commit: https://reviewboard.mozilla.org/r/93006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93006/

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8810710 [details]
Bug 1316537 part 1 - Add 64-bit clang-cl toolchain build.

https://reviewboard.mozilla.org/r/93002/#review93128

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to David Major [:dmajor] from comment #4)
> One tricky thing is that even though 64-bit clang-cl can build for either
> target (since you can do -m32), I still end up building a separate 32-bit
> clang-cl just for its asan libaries. I don't know if there's an easier way
> around that.

Does the 64-bit clang-cl not build any 32-bit runtime libraries, or does it just not build the ASan ones?

Given the problems with 32-bit ASan test running, I'm not sure if we care about 32-bit ASan builds...fuzzers might, though?

Comment 15

[(no longer active)]

Comment on attachment 8810711 [details]
Bug 1316537 part 2 - Update 64-bit clang build configuration for the changes in bug 1306650.

https://reviewboard.mozilla.org/r/93004/#review93200

Comment 16

[(no longer active)]

Comment on attachment 8810712 [details]
Bug 1316537 part 3 - Fix tar package path conversion for local build.

https://reviewboard.mozilla.org/r/93006/#review93202

Comment 17

[(Away)]

> Does the 64-bit clang-cl not build any 32-bit runtime libraries, or does it
> just not build the ASan ones?

afaik it doesn't build any 32-bit libraries.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f12149f08204
part 1 - Add 64-bit clang-cl toolchain build. r=dustin
https://hg.mozilla.org/integration/autoland/rev/bb4a098b3001
part 2 - Update 64-bit clang build configuration for the changes in bug 1306650. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/0e6e3d0eec49
part 3 - Fix tar package path conversion for local build. r=Ehsan

Comment 19

[Ting-Yu Chou [:ting] (away)]

I'll check to make sure the task does work after landing.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f12149f08204
https://hg.mozilla.org/mozilla-central/rev/bb4a098b3001
https://hg.mozilla.org/mozilla-central/rev/0e6e3d0eec49

Comment 21

[Ting-Yu Chou [:ting] (away)]

Confirmed it works.

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

Comment 22

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.