Closed Bug 1105729 Opened 9 years ago Closed 9 years ago

Windows build bustage: error C3861: '_xgetbv': identifier not found

Categories

(Core :: Security, defect)

x86
Windows 7
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
thunderbird36 ? fixed
thunderbird37 --- fixed

People

(Reporter: clokep, Assigned: bobowen)

Details

(Keywords: dogfood, regression)

Attachments

(1 file, 3 obsolete files)

c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/security/sandbox/chromium/base/cpu.cc(195) : error C3861: '_xgetbv': identifier not found
c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/config/rules.mk:934: recipe for target 'cpu.obj' failed
mozmake.exe[5]: Leaving directory 'c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/objdir-tb/security/sandbox'
c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/config/recurse.mk:74: recipe for target 'security/sandbox/target' failed
c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/objdir-tb/_virtualenv/Scripts/python.exe -m mozbuild.action.cl cl -FoUnified_cpp_layout_mathml1.obj -c -I../../dist/stl_wrappers -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -Ic:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/layout/mathml -I. -Ic:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/layout/mathml/../base -Ic:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/layout/mathml/../generic -Ic:/builds/moz2_slave/tb-c-cens-w32-ntly-000000000000/build/mozilla/layout/mathml/../style -Ic:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/layout/mathml/../tables -Ic:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/layout/mathml/../xul -Ic:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/dom/base -Ic:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mozilla/dom/mathml -I../../dist/include -Ic:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/objdir-tb/dist/include/nspr -Ic:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/objdir-tb/dist/include/nss -MD -FI ../../dist/include/mozilla-config.h -DMOZILLA_CLIENT -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR- -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy- -Fdgenerated.pdb c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/objdir-tb/layout/mathml/Unified_cpp_layout_mathml1.cpp
mozmake.exe[5]: *** [cpu.obj] Error 2
mozmake.exe[4]: *** [security/sandbox/target] Error 2
mozmake.exe[4]: *** Waiting for unfinished jobs.... 

Been unable to track down a commit that would have caused this, although the sandbox code from chromium was updated recently in bug 1041775
Severity: normal → blocker
Keywords: dogfood, regression
OS: Mac OS X → Windows 7
Reading e.g. Bug 977791 or Bug 977829 it seems that _xgetbv() is not available in VS2010. Thunderbird build servers are still using VS2010. I can test later if my Windows build with VS2013 works correctly.
My Windows build with VS2013 finished successfully, therefore this looks like incompatibility with VS2010 caused by Bug 1041775. Upgrade to VS2013 for Thunderbird is tracked with Bug 1085767, for SeaMonkey it is tracked with Bug 1092468.

Is VS2010 currently considered a supported platform? If yes, could the checkin for Bug 1041775 be changed to work with VS2010?
I was sure I had built this with VSVC2010 as I had to roll back one of the files to get it to work, just running a new build now.

Out of interest, does Thunderbird need the sandbox code?
Firefox built fine with Visual Studio 10.0, so I assume something else must be set up differently for the Firefox build.

If the sandboxing is not required it can be turned off with --disable-sandbox.
--disable-sanbox seems no effective on comm-central if mozconfig.common.override works correctly.
https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=ab3bb52b346b
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> --disable-sanbox seems no effective on comm-central if
> mozconfig.common.override works correctly.
> https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-
> central&revision=ab3bb52b346b

From what I can tell, the mail/ mozconfig files don't pull in a mozconfig.common.override file.
I've tried adding to build/win*/mozconfig.vs2010* files as these look like the right ones to me.
Try push running:
https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=6394bddeb2aa


Also, the build servers seem to be using compiler version 16.00.30319.01, on my machine it is 16.00.40219.01.
I think this is because I have SP1 and the servers don't.

I'm trying to set up a VM without SP1 to test this.
This patch fixed the issue, although clearly other unrelated things are still failing in the try push.

I'm not sure who would normally review this sort of change for Thunderbird.

I've confirmed that I get the same error when building with VSVC2010 without SP1.
When I install SP1 it gets past that error, but I'm struggling to get it to build due to other problems.

Ted, Mike: do you think that it is reasonable to require SP1 to build Firefox with VS2010 express or am I going to have to get bug 1041775 backed out (if I can't find a different solution)?
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(ted)
Flags: needinfo?(mh+mozilla)
One other option that appears to fix this compilation issue is to add the declaration of _xgetbv from the SP1 version of the file to security/sandbox/chromium/base/shim/sdkdecls.h.

This file already gets force included for the sandbox build to work around other issues.

This gets us past the initial _xgetbv compile error, although because I'm getting other compilation issues to do with gfx/angle, I'm not sure it will fully compile.
It might have problems when it comes to link.

As this is a change to m-c, I'm not sure how to include this for a c-c try push.
(In reply to Bob Owen (:bobowen) from comment #9)
> As this is a change to m-c, I'm not sure how to include this for a c-c try
> push.

https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer has instructions. I may be able to push a try push, but I'm not at a desktop easily able to test it.

(In reply to Stefan Sitter from comment #3)
> Is VS2010 currently considered a supported platform? If yes, could the
> checkin for Bug 1041775 be changed to work with VS2010?

VS2010 is intended to still be supported by at least December <mumble mumble>, last I heard.
(In reply to Joshua Cranmer [:jcranmer] from comment #10)
> (In reply to Bob Owen (:bobowen) from comment #9)
> > As this is a change to m-c, I'm not sure how to include this for a c-c try
> > push.

Right, I see the section, thanks.
As I suspected the try push with the _xgetbv declaration patch failed when linking:
https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central
A possible replacement in assembler:
https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=7d596ff17a59

opt build is failing, but for different reasons, might be machine related.
VS2010 is still supported, it will likely be unsupported at the next uplift. If there's a simple workaround for now that would be great, it only has to be a short-term thing. Requiring 2010SP1 doesn't seem terribly onerous.
Flags: needinfo?(ted)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> VS2010 is still supported, it will likely be unsupported at the next uplift.
> If there's a simple workaround for now that would be great, it only has to
> be a short-term thing. Requiring 2010SP1 doesn't seem terribly onerous.

Thanks Ted.
If Thunderbird doesn't need the chromium sandbox yet, then I think the right thing to do is to not compile it at all, which the first patch achieves.
I'm not sure who would review Thunderbird build changes normally or who can answer the question as to whether it is needed.
I think it probably isn't.

Installing SP1 fixes the issue for Firefox, so maybe we don't need to do anything other than update build instructions.  I was hitting other issues when trying to build with VS2010 like Bug 1076020 as well.

The assembly code fix also seems to work.
I think it might need other #if guards and it certainly needs checking.
To say my assembly coding is rusty, would imply that there was something there to oxidise in the first place. :-)

(In reply to Stefan Sitter from comment #16)
> There are already some solutions in tree that workaround the very same
> problem
> https://mxr.mozilla.org/mozilla-central/search?string=_xgetbv
> 
> For example here:
> https://mxr.mozilla.org/mozilla-central/source/media/libyuv/source/cpu_id.
> cc#97

Yeah, I took my fix by adapting some of these along with other solutions I found elsewhere.
"need" is relative. I guess nobody "needs" it per se? However, we do want to stay as closely aligned to firefox as possible. The next general release is tb38, so thunderbird is probably happy with most changes that lets us build until the VS2013 upgrade and we can use sandboxing then. If the assembly fix works that sounds like the best way to go (least disruption).
(In reply to Bob Owen (:bobowen) from comment #18)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> > VS2010 is still supported, it will likely be unsupported at the next uplift.
> > If there's a simple workaround for now that would be great, it only has to
> > be a short-term thing. Requiring 2010SP1 doesn't seem terribly onerous.
> 
> Thanks Ted.
> If Thunderbird doesn't need the chromium sandbox yet, then I think the right
> thing to do is to not compile it at all, which the first patch achieves.
> I'm not sure who would review Thunderbird build changes normally or who can
> answer the question as to whether it is needed. 

It has been our experience in the past that adding --disable-* features not commensurate with Firefox leads to frequent bustage, since things that don't break on try or mozilla-* don't get seen as broken.
Comment on attachment 8530427 [details] [diff] [review]
Add the declaration and definition of _xgetbv using inline assembly code for MSC versions before VS2010 SP1..

>+static unsigned long long _xgetbv(unsigned int xcr) {
>+  unsigned int eax0, edx0;
>+  __asm {
>+    mov ecx,xcr
>+    xgetbv
>+    mov edx0,edx
>+    mov eax0,eax
>+  }
>+  return (static_cast<unsigned long long>(edx0) << 32) | eax0;
>+}
Actually you can simplify this to
uint64_t __fastcall _xgetbv(uint32_t xcr) {
  __asm xgetbv;
}

I would also put it into cpu.cc with all the other compiler-specific code.
Due to the branch merge both comm-aurora (36.0a2) and comm-central (37.0a1) are currently affected by this error.
How to proceed with this bug? Comm-central builds are broken for one week already and comm-aurora builds since merge day.
Hi Tim,
I hope you're OK to review this.
Also, we must meet up at some point. :-)

(In reply to Stefan Sitter from comment #26)
> How to proceed with this bug? Comm-central builds are broken for one week
> already and comm-aurora builds since merge day.

Sorry about the delay in addressing this, I was traveling all day Monday and didn't have chance yesterday.

(In reply to neil@parkwaycc.co.uk from comment #21)
> Comment on attachment 8530427 [details] [diff] [review]
> Add the declaration and definition of _xgetbv using inline assembly code for
> MSC versions before VS2010 SP1..
> 
> >+static unsigned long long _xgetbv(unsigned int xcr) {
> >+  unsigned int eax0, edx0;
> >+  __asm {
> >+    mov ecx,xcr
> >+    xgetbv
> >+    mov edx0,edx
> >+    mov eax0,eax
> >+  }
> >+  return (static_cast<unsigned long long>(edx0) << 32) | eax0;
> >+}
> Actually you can simplify this to
> uint64_t __fastcall _xgetbv(uint32_t xcr) {
>   __asm xgetbv;
> }
> 
> I would also put it into cpu.cc with all the other compiler-specific code.

Thanks for this Neil.
I realised I could miss out some of the register moving, but had put it in to make it clear what was going on.
However your fastcall simplification is so elegant, I had to go with it and put the explanation in a comment instead.

Normally we've tried to keep changes out of the Chromium code where we can, but as this should be a fairly short term fix, I think you're right that the simplest thing is to add it to cpu.cc.
I'll file a bug to make sure this gets removed when we lose the VS2010 dependency.
Attachment #8530295 - Attachment is obsolete: true
Attachment #8530427 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
Attachment #8531741 - Flags: review?(tabraldes)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #30)
> Just FYI, I pushed a try with attachment 8531741 [details] [diff] [review].
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-
> central&revision=b971f7df9280

Excellent, thanks for doing this.
I had c-c cloned back on my workstation at home, but not on my laptop, so this was really helpful.

Here's a try push for Firefox try to show that it doesn't affect the Windows builds there:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=447a662ffb79
Comment on attachment 8531741 [details] [diff] [review]
Pre VS2010 SP1 define our own verion of _xgetbv. v1

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

Sorry for the delay!
Attachment #8531741 - Flags: review?(tabraldes) → review+
https://hg.mozilla.org/mozilla-central/rev/d58c0218bb55
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Target Milestone: Thunderbird 38.0 → Thunderbird 37.0
Comment on attachment 8531741 [details] [diff] [review]
Pre VS2010 SP1 define our own verion of _xgetbv. v1

Approval Request Comment
[Feature/regressing bug #]: build bustage caused by bug 1041775
[User impact if declined]: no comm-aurora Thunderbird builds for Windows
[Describe test coverage new/current, TBPL]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8531741 - Flags: approval-mozilla-aurora?
Attachment #8531741 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b716e52f2027
Component: Build Config → Security
Keywords: checkin-needed
Product: Thunderbird → Core
Target Milestone: Thunderbird 37.0 → mozilla37
The patch doesn't work on the Thunderbird Windows 8 x64 opt builders:

> https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4697&repo=comm-central
> [...]/mozilla/security/sandbox/chromium/base/cpu.cc(60) : 
> error C4235: nonstandard extension used : '__asm' keyword not supported on this architecture
> error C2065: 'xgetbv' : undeclared identifier

but maybe this can be ignored until after the update to VS2013.
You need to log in before you can comment on or make changes to this bug.