This is an archive of the discontinued LLVM Phabricator instance.

Fix branch relaxation in 16-bit mode.
ClosedPublic

Authored by niravd on May 31 2016, 11:33 AM.

Details

Summary

Thread through MCSubtargetInfo to relaxInstruction function allowing relaxation
to generate jumps with 16-bit sized immediates in 16-bit mode.

This fixes PR22097

Diff Detail

Event Timeline

niravd updated this revision to Diff 59106.May 31 2016, 11:33 AM
niravd retitled this revision from to Fix branch relaxation in 16-bit mode..
niravd added reviewers: craig.topper, dwmw2.
niravd updated this object.
niravd added a subscriber: llvm-commits.
niravd updated this object.Jun 6 2016, 5:42 AM
niravd edited edge metadata.

Ping for review?

craig.topper edited edge metadata.Jun 19 2016, 6:20 PM

Are there cases where 16-bit mode may need a full 32-bit offset that we need to support here? Or would that wrap the segment boundary for 16-bit mode and be illegal?

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
138

Why are we no longer passing just the Opcode to this function?

Are there cases where 16-bit mode may need a full 32-bit offset that we need to support here? Or would that wrap the segment boundary for 16-bit mode and be illegal?

Yes, since EIP calculation is always truncated to 16-bit so we can safely truncate our offset in the instruction and none of the exception checks make use of the top 16-bits of the 32-bit new EIP so it should always be okay to do the 16-bit truncated calculation in lieu of the 32-bit one.

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
138

It was to make branch and arithmetic relaxation procedures more similar in shape as they're clearly related. Having them both take Opcode would work equally as well.

I think all raised concerns have been addressed. Can I get a LGTM?

jyknight accepted this revision.Jul 8 2016, 7:05 AM
jyknight added a reviewer: jyknight.

Are there cases where 16-bit mode may need a full 32-bit offset that we need to support here? Or would that wrap the segment boundary for 16-bit mode and be illegal?

Yes, since EIP calculation is always truncated to 16-bit so we can safely truncate our offset in the instruction and none of the exception checks make use of the top 16-bits of the 32-bit new EIP so it should always be okay to do the 16-bit truncated calculation in lieu of the 32-bit one.

I don't think that's actually true. TTBOMK, EIP is actually only truncated to 16-bits for the jump/call instructions with 16-bit operands, so EIP can get above 64k either via linear program execution, or via executing a jump with a 32bit operand.

In Real mode, typically the segment limit is 64k anyways, so you can't actually access more. But, it is possible to set the limit higher via trickery (search "Unreal mode"), and reportedly this does work to let you access more code -- until you hit an interrupt, which will only save/restore the lower 16bits of EIP (oops).

In Protected mode, you can also change the default address/data operand sizes back to 16-bit, where that last part about interrupts isn't a problem anymore.

All that aside, -m16 / .code16 really is intended to imply that you're planning to fit your code/data within 64k. That is a sane meaning, and also what binutils has always done as well.

Theoretically, one might want to have a compiler mode which emits 16-bit instructions, but with 32-bit relocations/fixups. But that'd be pretty esoteric, and neither binutils nor llvm really have a goal of supporting all the crazy things you might ever possibly be able to do in x86 16-bit modes.

So, this change seems good to me.

This revision is now accepted and ready to land.Jul 8 2016, 7:05 AM
This revision was automatically updated to reflect the committed changes.