Closed Bug 1181255 Opened 6 years ago Closed 4 years ago

Get TSan builds and tests running in automation

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want)

Attachments

(11 files, 2 obsolete files)

40 bytes, text/x-review-board-request
chmanchester
: review+
Details
40 bytes, text/x-review-board-request
glandium
: review+
Details
40 bytes, text/x-review-board-request
jlund
: review+
Details
17.77 KB, patch
jlund
: review+
Details | Diff | Splinter Review
12.44 KB, patch
jlund
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
camd
: review+
Details | Review
689 bytes, patch
kmoir
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
emorley
: review+
Details | Review
1.29 KB, patch
kmoir
: review+
Details | Diff | Splinter Review
832 bytes, patch
glandium
: review-
Details | Diff | Splinter Review
1.63 KB, patch
glandium
: review+
Details | Diff | Splinter Review
I'd like to start with try.
Keywords: sec-want
Bug 1181255 - Turn off tsan checking during packaging. r=glandium
Attachment #8633646 - Flags: review?(mh+mozilla)
Bug 1181255 - Mozconfigs for tsan builds. r=glandium
Attachment #8633647 - Flags: review?(mh+mozilla)
I did this essentially by pattern matching and looking at asan and cc jobs, please let me know if I overlooked something obvious.
Attachment #8633677 - Flags: review?(jlund)
Bug 1181255 - Add configs for (linux64 opt) tsan builds. r=jlund
Attachment #8633678 - Flags: review?(jlund)
Ah, I think I want to turn off try_by_default for now.
Attachment #8633680 - Flags: review?(jlund)
Attachment #8633677 - Attachment is obsolete: true
Attachment #8633677 - Flags: review?(jlund)
Comment on attachment 8633646 [details]
MozReview Request: Bug 1181255 - Turn off tsan checking during packaging. r=glandium

https://reviewboard.mozilla.org/r/13229/#review11893

Ship It!
Attachment #8633646 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8633647 [details]
MozReview Request: Bug 1181255 - Mozconfigs for tsan builds. r=glandium

https://reviewboard.mozilla.org/r/13231/#review11895

::: build/unix/mozconfig.tsan:26
(Diff revision 1)
> +export MOZ_DEBUG_SYMBOLS=1
> +ac_add_options --enable-debug-symbols

I know these two are in mozconfig.asan, but they shouldn't be necessary (as in, 1. they both do the same thing and 2. that's the default)

::: build/unix/mozconfig.tsan:11
(Diff revision 1)
> +export TSANFLAGS="-fsanitize=thread -fPIC -pie"

-fPIC is the default.
-pie is better not given to everything, so use ac_add_options --enable-pie instead.

::: build/unix/mozconfig.tsan:32
(Diff revision 1)
> +ac_add_options --disable-debug

--disable-debug is the default.

::: build/unix/mozconfig.tsan:31
(Diff revision 1)
> +ac_add_options --enable-optimize="-O2 -gline-tables-only"

Please add a comment as to why you add -gline-tables-only. Also, it would be better *not* to pass this through --enable-optimize, and thus not pass -O2 as well. You can use --enable-debug-symbols=-gline-tables-only instead.

::: browser/config/mozconfigs/linux64/opt-tsan:6
(Diff revision 1)
> +export MOZ_PACKAGE_JSSHELL=1

Do you plan to use that js shell? If not, why package it?

::: build/unix/mozconfig.tsan:18
(Diff revision 1)
> +ac_add_options --disable-sandbox

I'd say that's unfortunate. It would be good to document why sandbox and tsan don't work together.
Attachment #8633647 - Flags: review?(mh+mozilla)
Comment on attachment 8633646 [details]
MozReview Request: Bug 1181255 - Turn off tsan checking during packaging. r=glandium

Bug 1181255 - Turn off tsan checking during packaging. r=glandium
Attachment #8633646 - Flags: review+ → review?(mh+mozilla)
Comment on attachment 8633647 [details]
MozReview Request: Bug 1181255 - Mozconfigs for tsan builds. r=glandium

Bug 1181255 - Mozconfigs for tsan builds. r=glandium
Attachment #8633647 - Flags: review?(mh+mozilla)
Attachment #8633646 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8633647 [details]
MozReview Request: Bug 1181255 - Mozconfigs for tsan builds. r=glandium

https://reviewboard.mozilla.org/r/13231/#review11923

Ship It!
Attachment #8633647 - Flags: review?(mh+mozilla) → review+
Attachment #8633683 - Flags: review?(jlund) → review+
Attachment #8633678 - Flags: review?(jlund) → review+
Comment on attachment 8633678 [details]
MozReview Request: Bug 1181255 - Add configs for (linux64 opt) tsan builds. r=jlund

https://reviewboard.mozilla.org/r/13235/#review11931

Ship It!
Comment on attachment 8633680 [details] [diff] [review]
Schedule tsan builds and tests on try

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

wow. well done at a blind attempt of this. There might be some unneeded items: legacy stuff for buildbot based factories before mozharness and mentions of talos configs even though we have talos disabled. In saying that, this patch is no different than asan/code-coverage (victim of this too) and it would be tedious to go through one by one removing all unused items especially as we are moving off buildbot anyway.

I believe this will add builds+tests for tsan on try if you explicitly select that in the trychooser syntax (i.e. try-by-default == False). This won't run on any other branches.

I think all that's left is:

- a cloud-tools patch to tell our instance manager to recognize the new builders and spin up instances accordingly
- trychooser patch so that this shows up as an option in: http://trychooser.pub.build.mozilla.org/

I did the cloud-tools one already (see next comment)
Attachment #8633680 - Flags: review?(jlund) → review+
(In reply to Mike Hommey [:glandium] from comment #9)
> ::: browser/config/mozconfigs/linux64/opt-tsan:6
> (Diff revision 1)
> > +export MOZ_PACKAGE_JSSHELL=1
> 
> Do you plan to use that js shell? If not, why package it?

If you're going to run jit-tests you need this nowadays. We should probably just make MOZ_PACKAGE_JSSHELL the default because of that.
Attachment #8633885 - Flags: review?(kmoir) → review+
Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/287738dd8bbdee503dc1e6e7ce5ae247f230c13e
Bug 1181255 - Make treeherder aware of a tsan build type.

https://github.com/mozilla/treeherder/commit/1d934685c1982e20c5c251422d92536f5477b020
Merge pull request #757 from chmanchester/tsan-builds

Bug 1181255 - Make treeherder aware of a tsan build type.
Attachment #8633714 - Flags: review?(cdawson) → review+
Attached file PR with fixup
Attachment #8635018 - Flags: review?(emorley)
Comment on attachment 8635018 [details] [review]
PR with fixup

r=me with comment
Attachment #8635018 - Flags: review?(emorley) → review+
Follow up for buildbot configs (sign off from jlund in #releng): https://hg.mozilla.org/build/buildbot-configs/rev/546c7ea19f2b
Comment on attachment 8636761 [details] [diff] [review]
Follow up to fix slave platform in mozilla/config.py

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

feel free to land when 1186188 is resolved. I wasn't able to verify this will completely solve our issue as the max_builder assert was blocking my tests from finishing but logically, this is what we need.
Attachment #8636761 - Flags: review?(jlund) → review+
(In reply to Jordan Lund (:jlund) from comment #32)
> Comment on attachment 8636761 [details] [diff] [review]
> Follow up to fix slave platform in mozilla/config.py
> 
> Review of attachment 8636761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> feel free to land when 1186188 is resolved. I wasn't able to verify this
> will completely solve our issue as the max_builder assert was blocking my
> tests from finishing but logically, this is what we need.

Bug 1186280 got us back under the limit:
https://hg.mozilla.org/build/buildbot-configs/rev/84310ed35dc0
https://bugzilla.mozilla.org/attachment.cgi?id=8636761&action=edit was backed out because of:
INFO  - ValueError: builder Ubuntu TSAN VM 12.04 x64 try opt test mochitest-1 reuses builddir try_ubuntu64_vm_test-mochitest-1

the previous patch was telling buildbot not to bother creating a new slave_type for tsan (which would require a big patch and duplicate code). Instead, use the existing ubuntu64_vm just like code-coverage does.

this will work fine except, when we go to create builddirs on the slave, the builddir derives from the slave_type + platform name, which would cause duplicate builddirs as there is already a ubuntu64_vm mochitest-1. To compensate, we are using build_dir_prefix to manually set the builddir and telling the scheduler there is magic (scheduler_slave_platform_identifier)

builderlist diff: http://people.mozilla.org/~jlund/tsan_test_builderlist.diff
dumpmaster diff: http://people.mozilla.org/~jlund/tsan_test_dump_master.diff
Attachment #8636761 - Attachment is obsolete: true
Attachment #8637455 - Flags: review?(kmoir)
Attachment #8637455 - Flags: review?(kmoir) → review+
Comment on attachment 8637455 [details] [diff] [review]
150822_tsan_tests_fix_builderdir_dupe-bbot-cfgs.patch

thanks!

remote:   https://hg.mozilla.org/build/buildbot-configs/rev/3e6a23a3e0f0
Comment on attachment 8638159 [details] [diff] [review]
Keep building with gtk2 for tsan

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

Please make them use gtk3. All the other builds are going to be switched soon (bug 1186748).
Attachment #8638159 - Flags: review?(mh+mozilla) → review-
Attachment #8640038 - Flags: review?(mh+mozilla) → review+
These run with "-p linux64-tsan", this just needs to get in to trychooser.
No longer depends on: 1186188
Duplicate of this bug: 998129
RyanVM just pointed out this bug is still open.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.