Page MenuHomePhabricator

[X86] Fix for bugzilla 31576 - add support for "data32" instruction prefix
ClosedPublic

Authored by myatsina on Jan 9 2017, 6:17 AM.

Details

Summary

This patch fixes bugzilla 31576 (https://llvm.org/bugs/show_bug.cgi?id=31576).

"data32" instruction prefix was not defined in the llvm.
An exception had to be added to the X86 tablegen because both "data16" and "data32" are encoded to 0x66 (but in different modes).

Diff Detail

Repository
rL LLVM

Event Timeline

myatsina updated this revision to Diff 83619.Jan 9 2017, 6:17 AM
myatsina retitled this revision from to [X86] Fix for bugzilla 31576 - add support for "data32" instruction prefix.
myatsina updated this object.
myatsina added reviewers: craig.topper, rafael.
myatsina set the repository for this revision to rL LLVM.
niravd edited edge metadata.Jan 9 2017, 6:48 AM

Can you add some disassembler tests?

Can you add some disassembler tests?

Do you mean to add tests for the tablegen diassembler change?

myatsina edited edge metadata.Jan 9 2017, 8:56 AM
myatsina added a subscriber: llvm-commits.

Can you add some disassembler tests?

Do you mean to add tests for the tablegen diassembler change?

Yes. I also notice that the testsuite is short on testing data16/data32. It'd be nice to add a few more tests for these as well.

Can you add some disassembler tests?

Do you mean to add tests for the tablegen diassembler change?

Yes. I also notice that the testsuite is short on testing data16/data32. It'd be nice to add a few more tests for these as well.

Which kind of tests do you have in mind? Combination of "data16"/"data32" with additional instructions (like lgdt in the bug)? Some additional tests I'm missing?

niravd added a comment.EditedJan 10 2017, 11:10 AM

Can you add some disassembler tests?

Do you mean to add tests for the tablegen diassembler change?

Yes. I also notice that the testsuite is short on testing data16/data32. It'd be nice to add a few more tests for these as well.

Which kind of tests do you have in mind? Combination of "data16"/"data32" with additional instructions (like lgdt in the bug)? Some additional tests I'm missing?

Tests to check the error messages for each an instruction (lgdt and it's explicitly sized variants should be fine) for the 16, 32, and 64 bit modes. gas will throw an error for "data16 ldgtw 0" but not for "data16 ldgtl 0". I don't think we need to throw an error for the former but we should have a test codifying the lack of failure for the second at least.

myatsina updated this revision to Diff 84131.Jan 12 2017, 9:03 AM

Added relevant data16/data32+lgdt tests.

Regarding the disassembler tests - the llvm build itself is the test, if somebody breaks this disassembler tablegen change, the llvm build will fail.

Sorry I think I was being overly terse and unclear.

For the disassembler, I'd like to make sure we generate the correct output text for 16-bit mode emitted data prefixes.

That is, we should have an explicit test to check that:
echo ".byte 0x66" | as -32 -o /tmp/a.o
llvm-objdump -triple i386-unknown-unknown-code16 -d /tmp/a.o

should give us "data32" not "data16"

For the other tests, I was looking for tests that check for errors we use data16/32 in an invalid mode or in a redundant way. So for 32-bit "data32 lgdt 0" should emit an error (redundant) and data32 should not be valid in 64-bit mode.

myatsina updated this revision to Diff 84595.Jan 16 2017, 2:33 PM

I've added the test that check for errors.

Regarding the disassembly, when data16/data32 is adjacent to an instruction like lgdt, then the encoding and the decoding is correct and the asm printer prints the right instruction. I've added disassembly tests for these cases.
When data16/data32 stands on its own the 0x66 byte by is disassembled correctly, but the asm printer always prints "data16" and not "data32" for the 16 bit mode.

I've found somebody already added an exception for CALLpcrel32 instruction in X86ATTInstPrinter because there's seems to be some lack of support of the "Requires" attribute.
I can add an exception there as well, but I'm not sure if this is the right thing to do because data16/data32 shouldn't really stand on it's own, it's a prefix to the instruction that follows.

niravd accepted this revision.Jan 17 2017, 11:17 AM

Regarding the disassembly, when data16/data32 is adjacent to an instruction like lgdt, then the encoding and the decoding is correct and the asm printer prints the right instruction. I've added disassembly tests for these cases.
When data16/data32 stands on its own the 0x66 byte by is disassembled correctly, but the asm printer always prints "data16" and not "data32" for the 16 bit mode.

I've found somebody already added an exception for CALLpcrel32 instruction in X86ATTInstPrinter because there's seems to be some lack of support of the "Requires" attribute.
I can add an exception there as well, but I'm not sure if this is the right thing to do because data16/data32 shouldn't really stand on it's own, it's a prefix to the instruction that follows.

Oof. That's unfortunate. As ugly as it is, I think you should add the special case to like CALLpcrel32 just so we're certain we always generate code that doesn't trigger warnings/errors.

I still don't see the additional tests so make sure they're added, but otherwise LGTM.

This revision is now accepted and ready to land.Jan 17 2017, 11:17 AM

Can you see the tests? I've added comments next to them.
Do this tests capture what you meant?

test/MC/X86/data-prefix-fail.s
8 ↗(On Diff #84595)

This is the error checking test

test/MC/X86/data-prefix16.s
5 ↗(On Diff #84595)

And this is one of the 3 disassembly tests (the other 2 are below).

Can you see the tests? I've added comments next to them.
Do this tests capture what you meant?

Yes.

But I thought you also had one using llvm-objdump so we also test the object->instruction disassembly path.

Can you see the tests? I've added comments next to them.
Do this tests capture what you meant?

Yes.

But I thought you also had one using llvm-objdump so we also test the object->instruction disassembly path.

The llvm-objdump tests are the newly added data-prefix16.s, data-prefix32.s, data-prefix64.s which have the following run line:

RUN: llvm-mc -triple<triple> -filetype=obj %s -o - | llvm-objdump -triple <triple> -d - | FileCheck %s

I will add to those tests the "data16"/"data32" disassembly once I tweak the asm printer.

This revision was automatically updated to reflect the committed changes.