Closed Bug 1320738 Opened 3 years ago Closed 2 years ago

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

Categories

(Firefox Build System :: General, defect, blocker)

defect
Not set
blocker

Tracking

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 verified)

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- verified

People

(Reporter: RyanVM, Assigned: gps)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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
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?
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 `""`)
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.
Severity: normal → blocker
Flags: needinfo?(gps)
Attached patch Disable warning C4742 (obsolete) — Splinter Review
Assignee: nobody → dmajor
Flags: needinfo?(gps)
Attachment #8927005 - Flags: review?(core-build-config-reviews)
Comment on attachment 8927005 [details] [diff] [review]
Disable warning C4742

Reassigning to someone qualified to review C++ changes.
Attachment #8927005 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
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.
Attachment #8927005 - Flags: review?(nfroyd)
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...
(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!
Attachment #8927005 - Attachment is obsolete: true
Blocks: 1416285
As the commit message states, this was a regression from 2013c8dd1824 (bug 1412932).
Depends on: 1412932
Keywords: regression
Attachment #8927385 - Flags: review?(core-build-config-reviews)
Summary: Various "warning C4742: '`string'' has different alignment" warnings in Win PGO builds → Various "warning C4742: '`string'' has different alignment" warnings breaking Fx58 DevEdition builds due to Werror being accidentally re-enabled on them
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/b0ded9bbd517
Mark MOZ_PGO as a JS option. r=froydnj, a=RyanVM
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.
Assignee: dmajor → gps
Target Milestone: --- → mozilla58
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Still broken :(
https://treeherder.mozilla.org/logviewer.html#?job_id=143871153&repo=mozilla-beta
Status: RESOLVED → REOPENED
Flags: needinfo?(gps)
Resolution: FIXED → ---
Still -WX on Library.cpp...
(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?
(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.
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"?
(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.
That is, this patch.
Attachment #8927529 - Flags: review+
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a412222f22e
export MOZ_PGO for js/src subconfigure; r=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.
Flags: needinfo?(gps)
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.
Attachment #8927534 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6a412222f22e
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
That worked!
Status: RESOLVED → VERIFIED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.