Closed Bug 1316537 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ting, Assigned: ting)

Details

Attachments

(3 files)

Bug 1316537 part 1 - Add 64-bit clang-cl toolchain build.
58 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1316537 part 2 - Update 64-bit clang build configuration for the changes in bug 1306650.
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1316537 part 3 - Fix tar package path conversion for local build.
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details 
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.
Any idea who would do this?
Component: General → Task Configuration
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.
Flags: needinfo?(dmajor)
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.
Flags: needinfo?(dmajor)
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.
I'll take this.
Assignee: nobody → janus926
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?
(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.
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.
(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 on attachment 8810710 [details]
Bug 1316537 part 1 - Add 64-bit clang-cl toolchain build.

https://reviewboard.mozilla.org/r/93002/#review93128
Attachment #8810710 - Flags: review?(dustin) → review+
(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?
Flags: needinfo?(dmajor)
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
Attachment #8810711 - Flags: review?(ehsan) → review+
Comment on attachment 8810712 [details]
Bug 1316537 part 3 - Fix tar package path conversion for local build.

https://reviewboard.mozilla.org/r/93006/#review93202
Attachment #8810712 - Flags: review?(ehsan) → review+
> 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.
Flags: needinfo?(dmajor)
Pushed by tchou@mozilla.com:
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
I'll check to make sure the task does work after landing.
Keywords: leave-open
Confirmed it works.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c76375b44f68a249df35902f9684db00b77d5be7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.