1512921 - Bump Windows clang build to trunk for cfg

Status: RESOLVED DUPLICATE of bug 1521129

(Firefox Build System :: Toolchains, enhancement)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1485016#c21 we'll do a bump to trunk to pick up CFG fixes

Comment 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1512921 - Bump clang to trunk 347623 for Windows builds

Will wait to land this until I see the 'soft freeze is over' email.

Comment 2

[(Away)]

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.

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1512921 - Bump clang to trunk 347933 for Windows builds

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

It looks like this clang version is hitting an error: https://treeherder.mozilla.org/#/jobs?repo=try&revision=345886c98f3c6f55d9aeebe7bd1ee5d27e4f79fd&selectedJob=216617253

Comment 6

[(Away)]

Ouch. :/

Do you mind trying latest trunk?

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(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

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1512921 - Bump clang to trunk 348970 for Windows builds

Okay; this one avoids the compiler crash and preserve the unpoison patch.

Comment 9

[(Away)]

I'm sure this was more hassle than you were expecting, thank you for sticking with it!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8a4a6617e30
Bump clang to trunk 348970 for Windows builds. r=dmajor

Comment 11

[Dorel Luca [:dluca]]

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

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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?

Comment 13

[(Away)]

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

Comment 14

[(Away)]

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"`?

Comment 15

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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

[(Away)]

`test -z` means not-ASAN, so you'll want to be in the else case.

Comment 17

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(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

[(Away)]

Oh, you may need to prevent tail merging in js/src/old-configure.in as well.

Comment 19

[(Away)]

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.

Comment 20

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(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 21

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1512921 - Bump clang to trunk 348970 for Windows builds. r=dmajor

Add nolldtailmerge

Comment 22

[(Away)]

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.

Comment 23

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1512921 - Bump clang to trunk 348970 for Windows builds. r=dmajor

Carry over r+ from Comment 22

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/585f7d2135ee
Bump clang to trunk 348970 for Windows builds. r=dmajor

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/585f7d2135ee

Comment 26

[Narcis Beleuzu [:NarcisB]]

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

[Joel Maher ( :jmaher ) (UTC -8)]

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

[(Away)]

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