This is an archive of the discontinued LLVM Phabricator instance.

[MIPS][LLVM-MC] Fix Disassemble of Negative Offset
ClosedPublic

Authored by nitesh.jain on Feb 23 2016, 2:49 AM.

Details

Summary

The type of Imm in MipsDisassembler.cpp was incorrect since SignExtend64 return int64_t type.As per the MIPSr6 doc ,the offset is added to the address of the instruction following the branch (not the branch itself), to form a PC-relative effective target address hence “4” is added to the offset. The offset of some test case are update to reflect the changes due to “ + 4 ” offset and new test case for negative offset are added.

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain retitled this revision from to [MIPS][LLVM-MC] Fix Disassemble of Negative Offset .
nitesh.jain updated this object.
nitesh.jain added a reviewer: dsanders.
nitesh.jain set the repository for this revision to rL LLVM.
nitesh.jain added subscribers: jaydeep, bhushan, slthakur and 2 others.
vkalintiris requested changes to this revision.Feb 29 2016, 9:10 AM
vkalintiris edited edge metadata.

Can you update the following tests too?

LLVM :: ExecutionEngine/RuntimeDyld/Mips/ELF_N64R6_relocations.s
LLVM :: ExecutionEngine/RuntimeDyld/Mips/ELF_O32R6_relocations.s
LLVM :: MC/Disassembler/Mips/mips64r6/valid-mips64r6.txt (You forgot to update the BALC instruction).

They are failing on my machine with check-all.

This revision now requires changes to proceed.Feb 29 2016, 9:10 AM
nitesh.jain updated this revision to Diff 49609.Mar 2 2016, 5:47 AM
nitesh.jain edited edge metadata.

The tests are updated as per suggestion.

Hi Vasileios,

Could you please find some time to review this ?

Thanks

dsanders edited edge metadata.Mar 11 2016, 6:30 AM

Can you check whether the assemble/disassemble/assemble or disassemble/assemble/disassemble round-trip works for the affected instructions?
Assuming it does: LGTM with the FIXME's removed.

test/MC/Disassembler/Mips/mips32r6/valid-mips32r6.txt
36 ↗(On Diff #49609)

Are these FIXME's still true? If you assemble the mnemonics from the disassembler do you get the same opcodes?
I suspect this change may have fixed the ones next to the tests you've updated.

nitesh.jain added inline comments.Mar 15 2016, 11:17 PM
test/MC/Disassembler/Mips/mips32r6/valid-mips32r6.txt
36 ↗(On Diff #49609)

The encoding and decoding are not inverse of each other. Since while decoding the offset is left shifted by 2 and then 4 is added but in case of encoding it will just right shift by 2. Thus for example

encoding 0x18 0x42 0x01 0x4d -->disassemble -->bgezalc $2, 1336
bgezalc $2, 1336-->assemble-->0x18 0x42 0x01 0x4e
0x18 0x42 0x01 0x4e-->disassemble-->bgezalc $2, 1340.

So these FIXME's will always be true

Hi,
Could you please find some time to review this ?

Thanks

dsanders added inline comments.Mar 23 2016, 3:27 AM
test/MC/Disassembler/Mips/mips32r6/valid-mips32r6.txt
36 ↗(On Diff #49609)

That's definitely a bug, they should be inverses of each other.

I've just looked at the encode function and I think it's wrong for the path this test will use. The immediate path is just 'X >> 2' but the symbol reference path is '(X - 4) >> 2'. Would you be willing to fix the immediate-encode path as part of this patch? That way we can drop these FIXME's.

dsanders accepted this revision.Mar 23 2016, 4:45 AM
dsanders edited edge metadata.
dsanders added inline comments.
test/MC/Disassembler/Mips/mips32r6/valid-mips32r6.txt
36 ↗(On Diff #49609)

It appears I'm mistaken on this but we think the current behaviour is weird/wrong in both GAS and IAS. In GAS, immediates are absolute addresses that are modified with +4 for unknown reasons. In IAS, immediates are currently offsets and are not modified.

Your change matches GAS's behaviour so: LGTM with the 'FIXME: ' comments changed to '# The encode/decode functions are not inverses of each other in the immediate case'

nitesh.jain edited edge metadata.

Update the diff as per suggestion

This comment was removed by nitesh.jain.

Hi Vasileios,

Could you please find some time to review this ?

Thanks

Hi Vasileios,

Could you please find some time to review this ?

Thanks

vkalintiris accepted this revision.Apr 20 2016, 2:23 AM
vkalintiris edited edge metadata.

Hi Nitesh, you can go ahead and commit this. But you should change the comments from:

# The encode/decode functions are not inverses of each other.

to:

# The encode/decode functions are not inverses of each other in the immediate case

first, ie. you missed the in the immediate case bit :)

This revision is now accepted and ready to land.Apr 20 2016, 2:23 AM
This revision was automatically updated to reflect the committed changes.