Closed
Bug 1512921
Opened 6 years ago
Closed 6 years ago
Bump Windows clang build to trunk for cfg
Categories
(Firefox Build System :: Toolchains, enhancement)
Firefox Build System
Toolchains
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1521129
People
(Reporter: tjr, Assigned: tjr)
References
Details
Attachments
(1 file, 4 obsolete files)
13.70 KB,
patch
|
tjr
:
review+
|
Details | Diff | Splinter Review |
Per https://bugzilla.mozilla.org/show_bug.cgi?id=1485016#c21 we'll do a bump to trunk to pick up CFG fixes
Assignee | ||
Comment 1•6 years ago
|
||
Will wait to land this until I see the 'soft freeze is over' email.
Attachment #9030253 -
Flags: review?(dmajor)
Sorry for the churn, but could I ask you to bump the revision a little higher?
The reason is that `r342652-unpoison-thread-stacks.patch` keeps bouncing on LLVM trunk. The situation I want to avoid is where we pick up a revision where the landing was present, so we delete our local patch, but then down the road we pick up an even newer LLVM where the patch was again reverted.
The most recent revert was r347933, so if you use that revision, and keep the unpoison.patch, that should be good. It would probably be a good idea to double check that CFG tests are still happy with that revision.
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9030253 -
Attachment is obsolete: true
Attachment #9030253 -
Flags: review?(dmajor)
Attachment #9030548 -
Flags: review?(dmajor)
Assignee | ||
Comment 5•6 years ago
|
||
It looks like this clang version is hitting an error: https://treeherder.mozilla.org/#/jobs?repo=try&revision=345886c98f3c6f55d9aeebe7bd1ee5d27e4f79fd&selectedJob=216617253
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to David Major [:dmajor] from comment #6)
> Ouch. :/
>
> Do you mind trying latest trunk?
Seems to work! I'm triggering Talos...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3674b2952f4a41ec75b4b90249ea12ade2df4d1
Assignee | ||
Comment 8•6 years ago
|
||
Okay; this one avoids the compiler crash and preserve the unpoison patch.
Attachment #9030548 -
Attachment is obsolete: true
Attachment #9030548 -
Flags: review?(dmajor)
Attachment #9031538 -
Flags: review?(dmajor)
Attachment #9031538 -
Attachment is patch: true
Attachment #9031538 -
Flags: review?(dmajor) → review+
I'm sure this was more hassle than you were expecting, thank you for sticking with it!
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 10•6 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8a4a6617e30
Bump clang to trunk 348970 for Windows builds. r=dmajor
Keywords: checkin-needed
Comment 11•6 years ago
|
||
Bug 1512921 - Bump clang to trunk 348970 for Windows builds. r=dmajor
Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217490222&repo=mozilla-inbound&lineNumber=44312
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c8a4a6617e305d5adec50b8cad2ec54c5893b270
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fae385f03658bdc71d870079711203ce4255722
Flags: needinfo?(tom)
Assignee | ||
Comment 12•6 years ago
|
||
So we're getting odr violations from ASAN opt builds.
I found https://bugs.llvm.org/show_bug.cgi?id=37545 which might be the issue but I am unsure. The two source code annotations found in the logs are:
https://searchfox.org/mozilla-central/rev/47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc#162
sqlite3.c : if( zFilename==0 ) zFilename = "\000\000";
https://searchfox.org/mozilla-central/rev/47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/db/sqlite3/src/sqlite3.c
dmajor, do you have a recommended course of action?
Flags: needinfo?(tom) → needinfo?(dmajor)
Comment 13•6 years ago
|
||
I don't think this is LLVM 37545 because it's all in one translation unit.
The ODR checker is complaining about the string literals `""` and `"\000\000"` -- this smells of string tail merging. Let me see if anything stands out in upstream's recent history...
Flags: needinfo?(dmajor)
Comment 14•6 years ago
|
||
Does the failure go away if you add an `else` here https://searchfox.org/mozilla-central/source/old-configure.in#892 that for MOZ_ASAN sets `LDFLAGS="$LDFLAGS -opt:nolldtailmerge"`?
Assignee | ||
Comment 15•6 years ago
|
||
Hm; it didn't seem to take... https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66a38def27505472d053e0dc6ba223dccbf052a&selectedJob=217554142
I'll need to dig in deeper tomorrow
Comment 16•6 years ago
|
||
`test -z` means not-ASAN, so you'll want to be in the else case.
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to David Major [:dmajor] from comment #16)
> `test -z` means not-ASAN, so you'll want to be in the else case.
Gah.
Okay this compiled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fe5a12d7fc07783d2798e526224ff698a059e8f&selectedJob=217618233
There is one ASAN global-buffer-overflow failure. It seems like it might be related; it talks about the string 'y' and I found the following in the referenced files:
EVAL("y", &y);
https://searchfox.org/mozilla-central/rev/47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/js/src/jsapi-tests/testDeepFreeze.cpp#46
CHECK(a.addEdgeTo(b, u"y"));
https://searchfox.org/mozilla-central/rev/47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/js/src/jsapi-tests/testUbiNode.cpp#906
Comment 18•6 years ago
|
||
Oh, you may need to prevent tail merging in js/src/old-configure.in as well.
Comment 19•6 years ago
|
||
Also I'm kind of suspicious about __asan_wrap_wcslen being called on non-wide strings. There might be a logic bug somewhere... it's too bad we don't have symbols for jsapi-tests.exe. If your latest try push doesn't work, I may take a stab at building locally to see what that callstack is.
Assignee | ||
Comment 20•6 years ago
|
||
(In reply to David Major [:dmajor] from comment #19)
> Also I'm kind of suspicious about __asan_wrap_wcslen being called on
> non-wide strings. There might be a logic bug somewhere... it's too bad we
> don't have symbols for jsapi-tests.exe. If your latest try push doesn't
> work, I may take a stab at building locally to see what that callstack is.
This try run should have the symbols for jsapi-tests.exe in target.cppunittest.tests.tar.gz
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a555cae8a829566a645634fa2397bc79a995c71d
(In reply to David Major [:dmajor] from comment #18)
> Oh, you may need to prevent tail merging in js/src/old-configure.in as well.
Yes, this worked:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=102f1907807419cc5f1d926d360a3b49b4ed533c&selectedJob=217726792
Assignee | ||
Comment 21•6 years ago
|
||
Add nolldtailmerge
Attachment #9031538 -
Attachment is obsolete: true
Attachment #9032256 -
Flags: review?(dmajor)
Comment 22•6 years ago
|
||
Comment on attachment 9032256 [details] [diff] [review]
Bug 1512921 - Bump clang to trunk 348970 for Windows builds. r=dmajor
Could you add a comment to both configure.in's that goes something like: # String tail merging doesn't play nice with ASan's ODR checker.
Attachment #9032256 -
Flags: review?(dmajor) → review+
Assignee | ||
Comment 23•6 years ago
|
||
Carry over r+ from Comment 22
Attachment #9032256 -
Attachment is obsolete: true
Attachment #9032294 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 24•6 years ago
|
||
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/585f7d2135ee
Bump clang to trunk 348970 for Windows builds. r=dmajor
Keywords: checkin-needed
Comment 25•6 years ago
|
||
bugherder |
Comment 26•6 years ago
|
||
Backed out as requested by tjr in https://bugzilla.mozilla.org/show_bug.cgi?id=1515631#c3
Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/644731a71be81b31a049deee70170af3ca36efa1
Comment 27•6 years ago
|
||
the landing of this caused a perf regression:
== Change summary for alert #18351 (as of Wed, 19 Dec 2018 05:24:04 GMT) ==
Regressions:
6% raptor-webaudio-firefox windows7-32 opt 207.88 -> 220.58
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=18351
the backout here shows a perf improvement:
== Change summary for alert #18435 (as of Fri, 21 Dec 2018 08:58:38 GMT) ==
Improvements:
5% raptor-webaudio-firefox windows7-32 opt 218.00 -> 206.25
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=18435
as a note, both opt and pgo regressed/improved, we just have alerts for opt on the original branch
Comment 28•6 years ago
|
||
I'm going to split out the -opt:nolldtailmerge
piece and re-land it with author=tjr and r=me, as we'll need it for bug 1521129 (and in turn bug 1512822).
Assignee | ||
Updated•6 years ago
|
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•