Closed Bug 919735 Opened 6 years ago Closed 6 years ago

VS2013 amd64_x86 cross-compile uses the wrong pgort120.dll

Categories

(Firefox Build System :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: dmajor, Assigned: dmajor)

References

Details

Attachments

(1 file, 2 obsolete files)

I'm filing this to keep it on my radar, but at this point I don't know if it's a real bug or just a configuration error on my part.

On a VS2013 amd64_x86 cross-compile PGO build, I get this message box during the build:

---------------------------
xpcshell.exe - System Error
---------------------------
The program can't start because pgort120.DLL is missing from your computer. Try reinstalling the program to fix this problem. 
---------------------------
OK   
---------------------------

Depends.exe shows a dependency from xpcshell.exe to pgort120.dll via xul and mozjs. What's strange is that there is no such dependency when I do a PGO build using the native-32-bit tools.

I do have a pgort120.dll in my VS2013 install folder, and if I copy it over to SysWOW64 then my cross-compiled xpcshell.exe runs correctly. 

So that leaves me wondering:
- Did I overlook some config steps that would have allowed pgort120 to be found?
- Should VS have installed it to my system folder?
- Is it a bug that the natively-built xpcshell doesn't depend on pgort120?

I'm currently looking into other issues so I'll dig into this later.
vs don't install pgort into system folder,just keep it in the building environment. If you call vcvarsamd64_x86.bat, everything from the vs shall be ready for you.
(In reply to zhoubcfan from comment #1)
> vs don't install pgort into system folder,just keep it in the building
> environment. If you call vcvarsamd64_x86.bat, everything from the vs shall
> be ready for you.

You are right. My build still fails, but I used the wrong command window while filing this bug. If I use a proper build window then vcvarsamd64_x86.bat sets up some paths.

Here is the real problem:

---------------------------
xpcshell.exe - Application Error
---------------------------
The application was unable to start correctly (0xc000007b). Click OK to close the application. 
---------------------------
OK   
---------------------------

From some web searching, it seems that this error code can happen if you try to load 64-bit DLLs from a 32-bit app. And it appears that this is the case in the cross-compile:

$ where pgort120.dll
c:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\bin\amd64\pgort120.dll

vcvarsamd64_x86.bat adds bin\amd64_x86 to the PATH, followed by bin\amd64. But there is no bin\amd64_x86\pgort120.dll (only bin\pgort120.dll) so instead we try to load bin\amd64\pgort120.dll which is the wrong file.
I didn't meet the problem.
I think what it boils down to is this: normally, in a cross-compile, you don't (can't) run your output (imagine if we had been building ARM for example). So in that sense it's not unreasonable that VS sets the build environment PATH to contain only 64-bit tools. But in order to gather PGO data, our build system does execute its own output. And in this case that ends up launching a 32-bit app in a 64-bit tools environment, leading to the DLL confusion.
(In reply to zhoubcfan from comment #3)
> I didn't meet the problem.

Were you running a PGO build?
(In reply to David Major [:dmajor] from comment #5)
> (In reply to zhoubcfan from comment #3)
> > I didn't meet the problem.
> 
> Were you running a PGO build?

due to cross toolchain problem, the ctypes can't be built, so I open the native and the cross toolchain command windows at the same time. I can't remember which one I use to gather the profile. So probably I used the native environment.
gps, can you take a look at comment 4 -- any idea what it would take to get around this? Maybe we'd want do profile runs under a different environment from the build?

(If you catch up on the history, please disregard comment 0, that was user error)
Flags: needinfo?(gps)
Summary: VS2013 amd64_x86 cross-compile missing pgort120.dll → VS2013 amd64_x86 cross-compile uses the wrong pgort120.dll
copy the dll like msvcr120.dll into the build is enough
Component: General → Build Config
We currently invoke profileserver.py via "make pgo-profile-run":
http://mxr.mozilla.org/mozilla-central/source/client.mk#248
http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#387
http://mxr.mozilla.org/mozilla-central/source/build/pgo/profileserver.py

If we need to put in a workaround for this case that's fine. This seems like a failing of the toolchain, but I'm not sure that our use case here is well-supported. It'd be interesting to see if this same issue happens if you try to build a simple PGO program using the VC++ command prompt. (Do they have a batch script for the 64->32 cross toolchain in a command prompt?)
call both vcvars32.bat and vcvarsamd64_x86.bat can also solve the problem.
I hate our Windows development environment so much.

Surely we can work some magic where the PGO running invokes some other .bat script or sets the appropriate environment variables.

What about release automation land? Do they even run MozillaBuild through the .bat scripts it provides?

I defer to Ted for domain knowledge here.
Flags: needinfo?(gps)
I'm 100% OK with just putting some horrible hack in wherever we need to put it. We can put the path to the pgort.dll in the mozconfig and put it in $PATH in profileserver.py, that seems totally doable.
follow comment10, we can just only use the cross link when lining the big xul.dll, since the native compile tools run faster.
I haven't been working on this since it's kind of pointless without a fix for bug 922441.
Assignee: dmajor → nobody
Depends on: 922441
(In reply to David Major [:dmajor] from comment #14)
> I haven't been working on this since it's kind of pointless without a fix
> for bug 922441.

There are already several workaround solutions mentioned.
David, what's the status of this bug?
Flags: needinfo?(dmajor)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #16)
> David, what's the status of this bug?

The bug is still present. Locally, I've been working around it by copying VC\bin\pgort120.dll to VC\bin\amd64_x86\pgort120.dll.
Flags: needinfo?(dmajor)
I see, thanks.  So this should block us starting to test this on our infrastructure, is that correct?
Yes, we'll need to get this sorted out before doing serious testing.

Anybody want to jump in with a patch? If this is left to me, the results may not be pretty!
Let's try my suggestion per comment 12: add some WIN32_VC_BIN_PATH=... to the mozconfig, AC_SUBST it in configure.in, and then in profileserver.py do:
from buildconfig import substs

if "WIN32_VC_BIN_PATH" in substs:
    env["PATH"] = env["PATH"] + ";" + env["WIN32_VC_BIN_PATH"]

Not terribly complicated.
(Feel free to bikeshed on the variable name, I just made that up.)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #21)
> (Feel free to bikeshed on the variable name, I just made that up.)

calling both vcvars32.bat and vcvarsamd64_x86.bat can easily solve the problem.
That doesn't fit well into our automation, I don't think we're going to do that.
Argh, it failed outside of profileserver as well, when xpcshell was trying to precompile_startupcache.
Attached patch Path hack (obsolete) — Splinter Review
I'd like to minimize the amount of magic outside knowledge that has to be passed in -- what do you think of doing a relative path from VS120COMNTOOLS?
Attachment #8390930 - Flags: review?(ted)
Ugh, substs["TARGET_CPU"].find("86") won't do the right thing, since it would also catch "x86_64"
Attached patch Path hack (obsolete) — Splinter Review
Attachment #8390930 - Attachment is obsolete: true
Attachment #8390930 - Flags: review?(ted)
Attachment #8390952 - Flags: review?(ted)
Comment on attachment 8390952 [details] [diff] [review]
Path hack

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

Wish you didn't have to write this twice, but doesn't seem worth factoring out somewhere common for the few lines of code.

::: build/pgo/profileserver.py
@@ +56,5 @@
>      env["MOZ_CRASHREPORTER_NO_REPORT"] = "1"
>      env["XPCOM_DEBUG_BREAK"] = "warn"
> +
> +    # For VC12, make sure we can find the right bitness of pgort120.dll
> +    if "VS120COMNTOOLS" in env and substs["TARGET_CPU"].endswith("86"):

It might be less ugly to say "and not CONFIG['HAVE_64BIT_OS']".

@@ +57,5 @@
>      env["XPCOM_DEBUG_BREAK"] = "warn"
> +
> +    # For VC12, make sure we can find the right bitness of pgort120.dll
> +    if "VS120COMNTOOLS" in env and substs["TARGET_CPU"].endswith("86"):
> +      vc12dir = os.path.abspath(env["VS120COMNTOOLS"] + "/../../VC/bin")

You technically want an os.path.join here, not a +.

@@ +59,5 @@
> +    # For VC12, make sure we can find the right bitness of pgort120.dll
> +    if "VS120COMNTOOLS" in env and substs["TARGET_CPU"].endswith("86"):
> +      vc12dir = os.path.abspath(env["VS120COMNTOOLS"] + "/../../VC/bin")
> +      if os.path.exists(vc12dir):
> +        env["PATH"] = vc12dir + ";" + env["PATH"]

Do we want this first in PATH? I guess we're not actually executing any toolchain commands here so it's probably academic.
Attachment #8390952 - Flags: review?(ted) → review+
> Do we want this first in PATH? I guess we're not actually executing any
> toolchain commands here so it's probably academic.

Yes, we need it to precede the 64-bit bin folder that's already on the PATH.
Attached patch Path hackSplinter Review
As landed.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bf2687d93f
Attachment #8390952 - Attachment is obsolete: true
Attachment #8391581 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b6bf2687d93f
Assignee: nobody → dmajor
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.