1343149 - WindowsDllInterceptor doesn't support 'movups' emitted by clang-cl

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(Away)]

TestDllInterceptor.exe fails on clang-cl builds because we can't hook our own 'rotatePayload' function:

0:000> u TestDllInterceptor!rotatePayload
00007ff7`100e1000 488b02          mov     rax,qword ptr [rdx]
00007ff7`100e1003 0f104208        movups  xmm0,xmmword ptr [rdx+8]
00007ff7`100e1007 0f1102          movups  xmmword ptr [rdx],xmm0
00007ff7`100e100a 48894210        mov     qword ptr [rdx+10h],rax

We get as far as the '0f' but fail on the '10'.

Comment 1

[(Away)]

:handyman/:ccorcoran, FYI in case you are working on this file and want to add a bonus fix.

Comment 2

[Carl Corcoran [:ccorcoran]]

Attachment: Bug 1343149: Add handling for movups opcode;

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

Comment 3

[(Away)]

Comment on attachment 8871456 [details]
Bug 1343149: Add handling for movups opcode;

https://reviewboard.mozilla.org/r/142932/#review146676

::: xpcom/build/nsWindowsDllInterceptor.h:875
(Diff revision 1)
> +                   origBytes[nOrigBytes] == 0x11) {
> +          // SSE: movups xmm, xmm/m128
> +          //      movups xmm/m128, xmm
> +          COPY_CODES(1);
> +          int nModRmSibBytes = CountModRmSib(&origBytes[nOrigBytes]);
> +          if(nModRmSibBytes < 0) {

Nit: space after the if

I think I want to file a cleanup bug to remove all checks for CountModRmSib < 0 -- it can't happen. Its "default:" case is logically impossible, and we shouldn't bother with the null pointer check. Would you be opposed to that?

Comment 4

[(Away)]

Oh, nevermind. I thought kModUnknown was the only negative value. I didn't see that kModOperand64 == -2. :-/

Comment 5

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8871456 [details]
Bug 1343149: Add handling for movups opcode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142932/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc0944ff095b
Add handling for movups opcode; r=dmajor

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc0944ff095b