Closed Bug 886842 Opened 6 years ago Closed 6 years ago

Add clang trunk builds for ASan

Categories

(Release Engineering :: General, defect)

All
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: decoder, Assigned: rail)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want, Whiteboard: [asan])

Attachments

(4 files, 1 obsolete file)

Initially we tried to get ASan builds working with the Clang 3.3 update in bug 870173. The problem is that some backported patch doesn't work properly and is causing a lot of pain. It might be easier to add Clang trunk builds in addition to the 3.3 builds and use a separate manifest for ASan that points to Clang trunk.

That way, we can also more easily test regular builds on Clang trunk (e.g. test new features, analyzers, etc.).
I've built r184859 locally and it seems to work without any new problems so far (build successful, no crashes in mochitest-1 until it aborted with some timeouts which always happens in my local builds for some reason). Maybe we should start with this on TBPL and see if there are any problems that need to be solved on the Clang side.
Component: Release Engineering → Release Engineering: Automation (General)
QA Contact: catlee
How do we want to make sure we get the proper version of clang-with-asan built?  Should we add an extra option to build-clang.py, or should we add a new build-asan-clang.py that reuses most of the bits?  Or is somebody just going to manually build the necessary clang, hoping that this is a one-off thing?
Flags: needinfo?(choller)
I'm not familar with build-clang.py but rail said it wouldn't be a big problem to get multiple versions of Clang in parallel.

Of course for the actual builds (of Firefox) we would require a separate manifest.
Flags: needinfo?(choller) → needinfo?(rail)
We may also need to have different patches for regular and asan builds. We could move revision and patch information into a separate file (JSON) and use it as a build configuration file for build-clang.py (yay! another packaging format!!!).

Once we want to use different manifests for regular and asan builds, Releng would need to tweak the buildbot side configs/factories.
Flags: needinfo?(rail)
(In reply to Rail Aliiev [:rail] from comment #4)
> We may also need to have different patches for regular and asan builds.

Why is that? If there is a Clang/ASan problem on trunk, then the patch should always go upstream.
(In reply to Christian Holler (:decoder) from comment #5)
> (In reply to Rail Aliiev [:rail] from comment #4)
> > We may also need to have different patches for regular and asan builds.
> 
> Why is that? If there is a Clang/ASan problem on trunk, then the patch
> should always go upstream.

If we want to port/fix something to a stable branch we may want to avoid patching the trunk and patch the stable branch.
What's the next step here, Rail?
Flags: needinfo?(rail)
1) Separate ASan manifests. We can copy the current working manifests.
2) Teach buildbot to use different manifests for ASan builds.
3) modify build-clang.py so it handles more than 1 branch
4) create trunk clang builds

I can look at these this week.
Assignee: nobody → rail
Flags: needinfo?(rail)
To be landed with DONTBUILD.
Attachment #772631 - Flags: review?(ehsan)
Attached patch buildbot-configsSplinter Review
To be landed once the previous patch lands on m-c.
Attachment #772633 - Flags: review?(catlee)
Attachment #772631 - Flags: review?(ehsan) → review+
Comment on attachment 772676 [details] [diff] [review]
[WIP] build-clang.py

* remove all global variables
* use a JSON config file to separate data and logic
* all patches should use -p1 and applicable in source_dir
* tested on mac and linux64
Attachment #772676 - Flags: review?(bhearsum)
Attached patch trunk clang (obsolete) — Splinter Review
Can be tested on try after we land attachment 772633 [details] [diff] [review]
Blocks: 872565
Comment on attachment 772676 [details] [diff] [review]
[WIP] build-clang.py

Review of attachment 772676 [details] [diff] [review]:
-----------------------------------------------------------------

It seems awkward and bad to me that we have clang patches in our tree, but I don't have full context on this so I won't judge that part.

I'm r+'ing this, but please address the comment below. Also, I'm working on the assumption that is well tested, because I don't know much about clang or how to build it...so I can't validate that aspect of this.

::: build/unix/build-clang/clang-3.3.json
@@ +1,5 @@
> +{
> +    "llvm_revision": "183744",
> +    "llvm_repo": "http://llvm.org/svn/llvm-project/llvm/branches/release_33",
> +    "clang_repo": "http://llvm.org/svn/llvm-project/cfe/branches/release_33",
> +    "compiler_repo": "http://llvm.org/svn/llvm-project/compiler-rt/branches/release_33",

Are we pulling from llvm.org at build time on releng machines? If so, we need to either stop doing that, or get it on the whitelist. We should also pull from https or some other secure thing if possible.
Attachment #772676 - Flags: review?(bhearsum) → review+
(In reply to Ben Hearsum [:bhearsum] from comment #15)
> Comment on attachment 772676 [details] [diff] [review]
> [WIP] build-clang.py
> 
> Review of attachment 772676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems awkward and bad to me that we have clang patches in our tree, but I
> don't have full context on this so I won't judge that part.

The script is supposed to formalize the procedure of how we build clang binaries, so anyone can reproduce this.

> ::: build/unix/build-clang/clang-3.3.json
> @@ +1,5 @@
> > +{
> > +    "llvm_revision": "183744",
> > +    "llvm_repo": "http://llvm.org/svn/llvm-project/llvm/branches/release_33",
> > +    "clang_repo": "http://llvm.org/svn/llvm-project/cfe/branches/release_33",
> > +    "compiler_repo": "http://llvm.org/svn/llvm-project/compiler-rt/branches/release_33",
> 
> Are we pulling from llvm.org at build time on releng machines? If so, we
> need to either stop doing that, or get it on the whitelist. 

No, this is a one time operation to build a tarball, a similar to RPM building.

To simplify our life we agreed on a procedure when responsible devs provide us compiled clang binaries, we upload them to the tooltool server, they test them on try, only then a manifest patch is landed on mozilla-central. Rafael used to build the binaries himself (using VMs I believe).


> We should also
> pull from https or some other secure thing if possible.

Good idea. I'll change that.
Attachment #772633 - Flags: review?(catlee) → review+
Attached patch trunk clangSplinter Review
Nathan,

What's the procedure for ASan specific tests? Can you test it on try somehow (AFAIK ASan is m-c only)?
Attachment #773505 - Flags: review?(nfroyd)
Attachment #773263 - Attachment is obsolete: true
Rail, thanks for the patches :)

It is possible to test ASan on try and I will do this now with your manifest. I'll report back with the results. Setting needinfo on myself for this :)
Flags: needinfo?(choller)
Comment on attachment 773505 [details] [diff] [review]
trunk clang

https://tbpl.mozilla.org/?tree=Try&rev=4d2f7b2718c3

Looks good. We still have known failures, but I don't see any new issues, except for the various alloc_dealloc_mismatches. We can investigate them one by one and meanwhile keep that check disabled. (I disabled alloc_dealloc_mismatch warnings in mochitests, but in compiled tests they still trigger sometimes so I still have to pass the proper ASAN_OPTIONS there somehow).

I think we can land this and then continue trying to get the tree green.
Attachment #773505 - Flags: feedback+
Comment on attachment 773505 [details] [diff] [review]
trunk clang

Review of attachment 773505 [details] [diff] [review]:
-----------------------------------------------------------------

I think Christian's feedback is sufficient here.
Attachment #773505 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #22)
> Comment on attachment 773505 [details] [diff] [review]
> trunk clang
> 
> Review of attachment 773505 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think Christian's feedback is sufficient here.

I'll push the patch when buildbot side is ready (after tomorrow's reconfig).
I think there is no need for this needinfo anymore?
Flags: needinfo?(choller)
In production
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Depends on: 1105715
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.