1987227 - Emitting incorrect x64 instruction for cmpb in some cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task)

Description

[Jan de Mooij [:jandem]]

With patches for bug 1986983 I got a mysterious jit-test failure on Try, only on Win64 debug builds. I managed to reproduce it locally and I got a time travel debugging trace from WinDbg.

In CodeGenerator::visitStringToLowerCase we use branch8 to check if a char is already lower case:

      masm.branch8(Assembler::NotEqual,
                   BaseIndex(toLowerCaseTable, current, TimesOne), current,
                   &hasUpper);

For certain registers, for example if toLowerCaseTable == rax and current == rdi, we emit a cmp instruction that compares to the wrong register:

cmp    %bh,(%rax,%rdi,1)

Changing oneByteOp to oneByteOp8 in cmpb_rm fixes this:

cmp    %dil,(%rax,%rdi,1)

If the bh register happens to have the same value in it as dil, this resulted in LStringToLowerCase thinking it could just return the input string and we fail the test.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1987227 part 2 - Emit correct instructions for cmp8 with byte registers on x64. r?iain!

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1987227 part 1 - Add test for cmp8 with byte registers on x64. r?iain!

This uses the Zydis disassembler to check the generated code is what we expect.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6bc9b83aa841
https://hg.mozilla.org/integration/autoland/rev/897ce9eb6a45
part 1 - Add test for cmp8 with byte registers on x64. r=iain
https://github.com/mozilla-firefox/firefox/commit/6488621f1e18
https://hg.mozilla.org/integration/autoland/rev/63b10ce476d9
part 2 - Emit correct instructions for cmp8 with byte registers on x64. r=iain

Comment 4

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/897ce9eb6a45
https://hg.mozilla.org/mozilla-central/rev/63b10ce476d9

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1987227 part 1 - Add test for cmp8 with byte registers on x64. r?iain!

This uses the Zydis disassembler to check the generated code is what we expect.

Original Revision: https://phabricator.services.mozilla.com/D264054

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1987227 part 1 - Add test for cmp8 with byte registers on x64. r?iain!

This uses the Zydis disassembler to check the generated code is what we expect.

Original Revision: https://phabricator.services.mozilla.com/D264054

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1987227 part 1 - Add test for cmp8 with byte registers on x64. r?iain!

This uses the Zydis disassembler to check the generated code is what we expect.

Original Revision: https://phabricator.services.mozilla.com/D264054

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1987227 part 1 - Add test for cmp8 with byte registers on x64. r?iain!

This uses the Zydis disassembler to check the generated code is what we expect.

Original Revision: https://phabricator.services.mozilla.com/D264054

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1987227 part 1 - Add test for cmp8 with byte registers on x64.

This uses the Zydis disassembler to check the generated code is what we expect.

Original Revision: https://phabricator.services.mozilla.com/D264054

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1987227 part 2 - Emit correct instructions for cmp8 with byte registers on x64.

Use oneByteOp8 to emit the REX prefix for byte registers if needed. This fixes an
issue with our code for LStringToLowerCase for certain register combinations/values.

With patches for bug 1986983 this was caught by jit-tests on Win64-debug builds.

Original Revision: https://phabricator.services.mozilla.com/D263972

Comment 11

[Donal Meehan [:dmeehan]]

:jandem, the uplift request patches are missing the Uplift Request form?
https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift#Submit_the_Uplift_Request_Form

Comment 12

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Broken websites.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Self-contained fix for x64 platforms to generate the right instruction.
• String changes made/needed: N/A
• Is Android affected?: no

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Donal Meehan [:dmeehan] from comment #11)

:jandem, the uplift request patches are missing the Uplift Request form?
https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift#Submit_the_Uplift_Request_Form

Huh weird, when I went to "Uplift Assessment" for part 2 now it still had the answers I entered last time so I just submitted that again. I'm not sure what went wrong the first time. Is this okay now?

Comment 14

[Donal Meehan [:dmeehan]]

(In reply to Jan de Mooij [:jandem] from comment #13)

(In reply to Donal Meehan [:dmeehan] from comment #11)

:jandem, the uplift request patches are missing the Uplift Request form?
https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift#Submit_the_Uplift_Request_Form

Huh weird, when I went to "Uplift Assessment" for part 2 now it still had the answers I entered last time so I just submitted that again. I'm not sure what went wrong the first time. Is this okay now?

Thanks, it looks ok now

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/7a6140d1daa5
https://hg.mozilla.org/releases/mozilla-esr140/rev/c92630a3ae46