This is an archive of the discontinued LLVM Phabricator instance.

The issues with X86 prefixes
AbandonedPublic

Authored by avt77 on Aug 16 2017, 5:52 AM.

Details

Summary

The current version of LLVM X86 disassembler incorrectly interprets some possible sets of x86 prefixes (see PR7709, 17697, 19251, 21640, etc.). This patch is the first step to resolve those issues. In particulary this patch could close the first two PRs from the above and should be able to close some others soon.

Diff Detail

Repository
rL LLVM

Event Timeline

avt77 created this revision.Aug 16 2017, 5:52 AM
avt77 added a comment.Aug 16 2017, 6:26 AM

I found one issue in the current version of the patch that's why I'm asking to postpone its review until the next version: hope to publish it in some hours or tomorrow.

avt77 updated this revision to Diff 111344.Aug 16 2017, 7:53 AM

I fixed the last minute issue raised in the first version.

craig.topper added inline comments.Aug 20 2017, 11:40 PM
lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
384 ↗(On Diff #111344)

Don't leave around commented out code

394 ↗(On Diff #111344)

Remove commented out code

421 ↗(On Diff #111344)

First '&&' should be on the previous line.

avt77 updated this revision to Diff 112141.Aug 22 2017, 2:51 AM

I did fixes raised by Craig.

craig.topper added inline comments.Aug 22 2017, 3:47 PM
lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
295 ↗(On Diff #112141)

What about 32-bit mode?

306 ↗(On Diff #112141)

wee->we

avt77 added inline comments.Aug 23 2017, 12:46 AM
lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
295 ↗(On Diff #112141)

I was sure that SSE2, etc. work on 64-bit CPUs only but it seems I was wrong: Wikipedia says:

The following IA-32 CPUs support SSE2:
Intel NetBurst-based CPUs (Pentium 4, Xeon, Celeron, Pentium D, Celeron D)
Intel Pentium M and Celeron M
Intel Atom
Transmeta Efficeon
VIA C7

OK, I'll fix it.

avt77 updated this revision to Diff 112323.Aug 23 2017, 3:09 AM

Craig's issues fixed.

craig.topper edited edge metadata.Aug 24 2017, 3:06 PM

Please add a 32-bit mode test as well.

lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
993 ↗(On Diff #112323)

Is this 64-bit mode checks till needed?

avt77 added inline comments.Aug 25 2017, 2:25 AM
lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
993 ↗(On Diff #112323)

In fact we need

... else if (insn->mode != MODE_16BIT ....

I'll do it. Tnx.

avt77 updated this revision to Diff 112694.Aug 25 2017, 7:53 AM

32-bit tests added and the condition fixed properly.

This revision is now accepted and ready to land.Aug 25 2017, 9:30 AM
This revision was automatically updated to reflect the committed changes.
RKSimon reopened this revision.Sep 5 2017, 5:21 AM
RKSimon edited edge metadata.

Reopening after reversion at rL311987

This revision is now accepted and ready to land.Sep 5 2017, 5:21 AM
RKSimon requested changes to this revision.Sep 5 2017, 5:22 AM
RKSimon added a reviewer: echristo.
RKSimon added a subscriber: echristo.

@echristo Please can you post the regressions you mentioned in rL311987 ?

This revision now requires changes to proceed.Sep 5 2017, 5:22 AM
echristo edited edge metadata.Sep 5 2017, 2:54 PM

The two I had were 0xf2666d and 0xf3666d that were broken with the earlier patch that I mentioned in my reversion email following up on the original.

avt77 abandoned this revision.Sep 19 2017, 3:43 AM

D37262 covers all issues from this review plus some others that's why we don't need D36788 any more.