Closed Bug 1512921 Opened Last year Closed 10 months ago

Bump Windows clang build to trunk for cfg

Categories

(Firefox Build System :: Toolchains, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1521129

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Duplicate of this bug: 1497605
Ouch. :/

Do you mind trying latest trunk?
(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
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!
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
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)
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)
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"`?
`test -z` means not-ASAN, so you'll want to be in the else case.
(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
Oh, you may need to prevent tail merging in js/src/old-configure.in as well.
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.
(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
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+
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
https://hg.mozilla.org/mozilla-central/rev/585f7d2135ee
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
See Also: → 1515335
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

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).

See Also: → 1521133
Status: REOPENED → RESOLVED
Closed: 11 months ago10 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1521129
You need to log in before you can comment on or make changes to this bug.