1320738 - Various "warning C4742: '`string'' has different alignment" warnings breaking Fx58 DevEdition builds due to Werror being accidentally re-enabled on them

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Only affects PGO/WPO builds. I bisected it back to when we switched compilers to MSVC2015 (and confirmed that it went away on Aurora after we reverted back to 2013 for Fx48).

warning C4742: '`string'' has different alignment in 'z:\build\build\src\obj-firefox\media\webrtc\trunk\webrtc\base\base_rtc_base_approved\unified_cpp_trunk_webrtc_base0.cpp' and 'z:\build\build\src\xpcom\components\nscategorymanager.cpp': 1 and 2
warning C4742: '`string'' has different alignment in 'z:\build\build\src\obj-firefox\toolkit\crashreporter\unified_cpp_crashreporter0.cpp' and 'z:\build\build\src\xpcom\components\nscategorymanager.cpp': 1 and 2
warning C4742: '`string'' has different alignment in 'z:\build\build\src\obj-firefox\js\src\unified_cpp_js_src24.cpp' and 'z:\build\build\src\xpcom\components\nscategorymanager.cpp': 1 and 2
warning C4742: '`string'' has different alignment in 'z:\build\build\src\js\src\ctypes\library.cpp' and 'z:\build\build\src\xpcom\components\nscategorymanager.cpp': 1 and 2

Comment 1

[Gregory Szorc [:gps]]

This warning sounds scary, as any time I hear "alignment issue" in C/C++ my mind immediately jumps to potential security problems. What are the implications of this bug?

Comment 2

[(Away)]

I think this stems from: https://developercommunity.visualstudio.com/content/problem/66760/utf-8-string-literal-with-single-null-character-ca.html

Each of the three warnings in comment 0 has a pair of files where one of them contains a `u""` (http://searchfox.org/mozilla-central/search?q=u%22%22) and I bet the other contains a `""` (I didn't bother searching, I bet every translation-unit in our tree has a `""`)

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Somehow, bug 1412932 has made this a permafailing Werror issue on Beta58, but only for DevEdition builds.
https://treeherder.mozilla.org/logviewer.html#?job_id=143179545&repo=mozilla-beta

This is actively blocking us from shipping DevEdition builds off mozilla-beta, so upgrading it to blocking status.

Comment 4

[(Away)]

Attachment: Disable warning C4742

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8927005 [details] [diff] [review]
Disable warning C4742

Reassigning to someone qualified to review C++ changes.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8927005 [details] [diff] [review]
Disable warning C4742

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

Is there some way we could limit this change to a particular version of MSVC?  The warning does seem fairly useful in general, and it'd be a shame if we upgraded to a version that *didn't* issue this spurious warning and keep the warning off.

Clearing review while we get that hashed out.

Comment 7

[(Away)]

There isn't really a perfect answer here. If we stick a version check on this, and the next version still isn't fixed, then we get another build break.

FWIW I'm expecting that our next Windows compiler upgrade is not going to be MSVC...

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to David Major [:dmajor] from comment #7)
> There isn't really a perfect answer here. If we stick a version check on
> this, and the next version still isn't fixed, then we get another build
> break.

I don't think this is a terrible thing; we'd just have to update the version check to include that version as well or verify that the bug has been fixed, and then we can remove the code.  I like things that force us to consider whether the hacks we put in are necessary or not.

> FWIW I'm expecting that our next Windows compiler upgrade is not going to be
> MSVC...

Optimism!

Comment 9

[Gregory Szorc [:gps]]

Attachment: Bug 1320738 - Mark MOZ_PGO as a JS option;

The changesets in bug 1412932 changed the semantics for MOZ_PGO.
Before, it was effectively being set as an environment variable
by client.mk all the time. Afterwards - specifically after
2013c8dd1824 - the variable is set in mozconfigs via ac_add_options,
which means it is only exposed to configure, not the environment.

Investigation by dmajor revealed that -WX (warnings as errors) was
added to a js/src file's compiler invocation after the PGO code
refactor. (PGO and warnings as errors have a strange interaction
- bug 437002 - and should be disabled there.) Strangely, addition
of -WX was only present on Dev Edition PGO builds.

The reason for this is likely mozharness. Mozharness will export
the MOZ_PGO=1 environment variable for build configurations that
it knows are PGO. It appears to do this for all PGO build
configurations except Dev Edition. Since make and moz.configure
inherit environment variables, mozharness was basically papering
over the intended behavior change in 2013c8dd1824.

This commit fixes the problem by marking MOZ_PGO as a JS option
in moz.configure. This means `ac_add_options MOZ_PGO=1` (the new
convention for enabling PGO) will set MOZ_PGO for SpiderMonkey's
moz.configure.

Of course, MOZ_PGO=1 in an environment variable still works. And
mozharness's setting of this variable has the intended effect.
Eventually, I'd like to clean up the mozharness code so it is less
PGO aware and enables PGO via ac_add_options. But that's for another
day.

Review commit: https://reviewboard.mozilla.org/r/198688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198688/

Comment 10

[Gregory Szorc [:gps]]

As the commit message states, this was a regression from 2013c8dd1824 (bug 1412932).

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8927385 [details]
Bug 1320738 - Mark MOZ_PGO as a JS option;

https://reviewboard.mozilla.org/r/198688/#review203806

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/b0ded9bbd517
Mark MOZ_PGO as a JS option. r=froydnj, a=RyanVM

Comment 13

[Mozilla Autoland]

We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Still broken :(
https://treeherder.mozilla.org/logviewer.html#?job_id=143871153&repo=mozilla-beta

Comment 15

[(Away)]

Still -WX on Library.cpp...

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to David Major [:dmajor] from comment #15)
> Still -WX on Library.cpp...

Is that in the log, I assume?  Or is it somehow left over from a not-clobber build?

How can only one file in all of JS get -WX?

Comment 17

[(Away)]

(In reply to Nathan Froyd [:froydnj] from comment #16)
> (In reply to David Major [:dmajor] from comment #15)
> > Still -WX on Library.cpp...
> 
> Is that in the log, I assume?  Or is it somehow left over from a not-clobber
> build?

Yes, -WX is in the log for Library.cpp: https://public-artifacts.taskcluster.net/GL1XGYTqSnOANnXGTdr2EQ/0/public/logs/live_backing.log

> How can only one file in all of JS get -WX?

It's not just one file, all JS files (at least the few I spot checked) have -WX. Library.cpp is just the only one that happens to trigger this string alignment warning.

Comment 18

[Nathan Froyd [:froydnj]]

This is one of those times when I *really* wish we uploaded things like config.{status,log} and so forth as build artifacts.

There are lots of comments in subconfigure.py about mingw and environment hacks; we even run build/win32/dumpenv4python.pl on Windows to pipe the environment into js/src's subconfigure so it doesn't get lost.  I wonder if MOZ_PGO is disappearing from the environment because of the weird things mingw does (or the weird things we do to counteract mingw weirdness)?  Maybe we should re-export it near the end of old-configure.in "just to be sure"?

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #18)
> Maybe we should re-export it near the end of old-configure.in "just to be sure"?

I think this almost has to be it.  If we want things to appear in the environment for JS, we have to re-export them on Windows.  See bug 1233542: we had to re-export a bunch of variables to compile with clang-cl.  js/src wasn't seeing our environment variables otherwise.

Comment 20

[Nathan Froyd [:froydnj]]

Attachment: export MOZ_PGO for js/src subconfigure

That is, this patch.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a412222f22e
export MOZ_PGO for js/src subconfigure; r=gps

Comment 22

[Gregory Szorc [:gps]]

Ugh. How annoying. Thanks for the help (again), David and Nathan!

I agree we should upload config.{status,log} to help debug failures like this. Just as long as we're not leaking things like API keys.

Comment 23

[Nathan Froyd [:froydnj]]

Attachment: export MOZ_PGO for js/src subconfigure

Actually, something like this, I think: we only want to export MOZ_PGO if it's
defined in the environment.  Everything else we re-export has been set by the
configure script, but MOZ_PGO is a little different.

philor bravely a+'d this patch, sight unseen:

10:17 PM <froydnj> who wants to a+ for central a self-reviewed patch for bug 1320738? =D
10:21 PM <philor> froydnj: a=me

But it's late here, and I don't have a Windows box close at hand to `mach
configure` and check js/src.  I can try to do this tomorrow morning when I'm a
little more awake.

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6a412222f22e

Comment 25

[Ryan VanderMeulen [:RyanVM]]

That worked!

Comment 26

[Intermittent Failures Robot]

6 failures in 849 pushes (0.007 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-beta: 6

Platform breakdown:
* windows2012-64-devedition: 3
* windows2012-32-devedition: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1320738&startday=2017-11-06&endday=2017-11-12&tree=all