Closed Bug 681238 Opened 10 years ago Closed 10 years ago

TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to add hook

Categories

(Firefox Build System :: General, defect)

x86_64
Windows Server 2008
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla9

People

(Reporter: armenzg, Assigned: glandium)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 680818 got fixed and we can now pass the compilation step and reach this make check failure.

See bug 606473 as well.

From:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1314087809.1314099252.14141.gz#err0

Running TestDllInterceptor tests
NEXT ERROR TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to add hook
make[4]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit/xre/test/win'
NEXT ERROR make[4]: *** [check] Error 1
make[3]: *** [check] Error 2
make[3]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit/xre/test'
make[2]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit/xre'
make[2]: *** [check] Error 2
make[1]: *** [check] Error 2
make[1]: Leaving directory `/e/builds/moz2_slave/m-cen-w64/build/obj-firefox/toolkit'
make: *** [check] Error 2
program finished with exit code 2
elapsedTime=301.292000
TinderboxPrint: check<br/>12518/<em class="testfail">1</em>
Unknown Error: command finished with exit code: 2
GetVersionExA entry has too complex MOV.  (mov r64, [rel32])

We hook LdrLoadDll, SetWindowLongPtr (SetWindowLong on Win32), and etc, but we don't inject GetVersionEx.  I believe that this isn't good test case for injection.

We should turn off this test or modify test case.
Modifying the test case to the functions we actually hook sounds like a good way to go.
How long will that take (roughly)? days or more?
Could we disable the test while it gets fixed?

In the next few days I hope to have try support in case fixing the test will be easier that way.

You probably know but just to be explicit, this is the last bug that I need to be fixed before I can show the win64 builds on tbpl.
The problem is finding a function that can be verified to have been called successfully when hooked (not the hook, the original function).
We hook:
- GetWindowInfo
- SetWindowLongA/SetWindowLongPtrA
- SetWindowLongW/SetWindowLongPtrW
- TrackPopupMenu
- LdrLoadDll

None of these seem to fit comment 5, except the first one, which requires a window...
This works for me, but merely by luck. I doubt it works on x64. We'd have to find the right contents for the fillPreload function such that the generated code is supported by the interceptor code. We could go as far as actually writing well-suited assembly.
My x86 machine code is a bit rusty, but this should be correct.

Mainly, I needed sub r, imm8 and mov r, [r+imm8] on x86 ; rex.wr and mov r, [r] on x64. These are the most common instructions I was getting when trying various functions for the test case, with or without optimization.
The x64 part could probably be refactored further, and share with x86, but I didn't feel like going much further.

I also spotted a bug in the test for mov r, [r] on x86, that tested REG instead of R/M to filter out the [SIB] and disp32  cases that take more bytes.
Attachment #555704 - Flags: review?(vladimir)
This makes the test hook a custom function instead of GetVersionEx. It's using UINT64 values mainly because the x64 dll interceptor doesn't support mov with 32 bit registers.
I tested with MSVC8 on x86 and x64, with and without optimization in both cases, and it passed.
Attachment #555705 - Flags: review?(vladimir)
Attachment #555423 - Attachment is obsolete: true
This adds a test to verify that we can indeed hook the functions that our codebase hooks and that are listed in comment 6.
Attachment #555706 - Flags: review?(vladimir)
Comment on attachment 555704 [details] [diff] [review]
part 0 - Support a few more opcodes and fix x86 mov r, [r] test

I'm going to assume the asm decoding is correct :)
Attachment #555704 - Flags: review?(vladimir) → review+
Backed out, because it traded the win64 orange with a win debug one :(
http://hg.mozilla.org/integration/mozilla-inbound/rev/9f7ccdc68e3f
So, what is happening on win debug is that the function GetProcAddress returns is actually the ILT (import lookup table) entry for the function, and that's only a jump to the real function. We don't support jumps in the interceptor on x86. This is due to -DEBUG turning off -OPT:REF, which actually gets rid of these. So building the executable with -DEBUG -OPT:REF works, but this doesn't sound very robust: there's no guarantee that -OPT:REF will continue to reliably remove these ILT entries. The other option is to add support for jumps in the interceptor.
This adds support for jumps on x86
Attachment #556790 - Flags: review?(vladimir)
I pushed the change to try to see how it behaves:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=62214c5e8cf8

For comparison I will look at:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=76e73aad0fab
(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #17)
> I pushed the change to try to see how it behaves:
> http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=62214c5e8cf8
> 
> For comparison I will look at:
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=76e73aad0fab

The results are satisfactory.
Whiteboard: fixed-in-bs → [inbound]
http://hg.mozilla.org/mozilla-central/rev/5dda9668493b
http://hg.mozilla.org/mozilla-central/rev/38bbf1e8ae5d
http://hg.mozilla.org/mozilla-central/rev/ce43a8644bc0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Duplicate of this bug: 653361
Depends on: 972671
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.