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

RESOLVED FIXED in Firefox 55

Status

()

Core
General
RESOLVED FIXED
8 months ago
5 months ago

People

(Reporter: dmajor, Assigned: ccorcoran)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [ele:1a])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
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'.
(Reporter)

Comment 1

5 months ago
:handyman/:ccorcoran, FYI in case you are working on this file and want to add a bonus fix.
Assignee: nobody → ccorcoran
Comment hidden (mozreview-request)
Attachment #8871456 - Flags: review?(aklotz) → review?(dmajor)
(Reporter)

Comment 3

5 months ago
mozreview-review
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?
Attachment #8871456 - Flags: review?(dmajor) → review+
(Reporter)

Comment 4

5 months ago
Oh, nevermind. I thought kModUnknown was the only negative value. I didn't see that kModOperand64 == -2. :-/
Comment hidden (mozreview-request)
Keywords: checkin-needed

Comment 6

5 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc0944ff095b
Add handling for movups opcode; r=dmajor
Keywords: checkin-needed

Comment 7

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc0944ff095b
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [ele:1a]
You need to log in before you can comment on or make changes to this bug.