[PATCH v3 12/11] Fix incorrect encoding of relaxed instructions (PR18303)
Sean Silva
silvas at purdue.edu
Sat Dec 21 17:10:01 PST 2013
FYI, the convention on the LLVM mailing lists is to attach patches rather
than have them inline. Also, all patches in a patch series should be
attached to the same message instead of being in multiple messages.
The reason for this is that most community members' mail clients (i.e.,
almost all "normal" mail clients) don't handle threading properly (and tend
to mangle the bodies, etc.), so the Linux-like conventions (i.e. what git's
email functionality does by default) don't work well. I believe it's just a
matter of passing a flag to git to change it's behavior.
-- Sean Silva
On Sat, Dec 21, 2013 at 1:22 PM, David Woodhouse <dwmw2 at infradead.org>wrote:
> When .code64 or .code32 (or .code16) directives occur in an assembler
> file, all relaxed instructions will be encoded in whichever mode was
> active at the end of the file. Which is kind of broken.
>
> http://llvm.org/bugs/show_bug.cgi?id=18303
>
> Fix this by storing the active feature set *in* the MCInst, so that it
> can be correctly honoured when the instruction is re-encoded after
> relaxation.
>
> Storing the feature set is optional; users which don't have a way to
> change the feature set will not need to bother as the SubtargetInfo will
> always have valid information and the code emitter will use that
> instead.
>
> (This should probably go earlier in the sequence before the addition
> of .code16. I'll do so if it masses muster.)
>
> diff --git a/include/llvm/MC/MCInst.h b/include/llvm/MC/MCInst.h
> index 4766815..de2c6f2 100644
> --- a/include/llvm/MC/MCInst.h
> +++ b/include/llvm/MC/MCInst.h
> @@ -151,8 +151,11 @@ class MCInst {
> unsigned Opcode;
> SMLoc Loc;
> SmallVector<MCOperand, 8> Operands;
> + /// The current set of available features.
> + uint64_t AvailableFeatures;
> + bool AvailableFeaturesSet;
> public:
> - MCInst() : Opcode(0) {}
> + MCInst() : Opcode(0), AvailableFeatures(0), AvailableFeaturesSet(false)
> {}
>
> void setOpcode(unsigned Op) { Opcode = Op; }
> unsigned getOpcode() const { return Opcode; }
> @@ -160,6 +163,13 @@ public:
> void setLoc(SMLoc loc) { Loc = loc; }
> SMLoc getLoc() const { return Loc; }
>
> + void setAvailableFeatures(uint64_t features) {
> + AvailableFeatures = features;
> + AvailableFeaturesSet = true;
> + }
> + bool isAvailableFeaturesSet() const { return AvailableFeaturesSet; }
> + uint64_t getAvailableFeatures() const { return AvailableFeatures; }
> +
> const MCOperand &getOperand(unsigned i) const { return Operands[i]; }
> MCOperand &getOperand(unsigned i) { return Operands[i]; }
> unsigned getNumOperands() const { return Operands.size(); }
> diff --git a/lib/Target/X86/AsmParser/X86AsmParser.cpp
> b/lib/Target/X86/AsmParser/X86AsmParser.cpp
> index 01746fa..0b432bf 100644
> --- a/lib/Target/X86/AsmParser/X86AsmParser.cpp
> +++ b/lib/Target/X86/AsmParser/X86AsmParser.cpp
> @@ -2420,6 +2420,7 @@ MatchAndEmitInstruction(SMLoc IDLoc, unsigned
> &Opcode,
> Op->getToken() == "finit" || Op->getToken() == "fsave" ||
> Op->getToken() == "fstenv" || Op->getToken() == "fclex") {
> MCInst Inst;
> + Inst.setAvailableFeatures(STI.getFeatureBits());
> Inst.setOpcode(X86::WAIT);
> Inst.setLoc(IDLoc);
> if (!MatchingInlineAsm)
> @@ -2443,6 +2444,7 @@ MatchAndEmitInstruction(SMLoc IDLoc, unsigned
> &Opcode,
>
> bool WasOriginallyInvalidOperand = false;
> MCInst Inst;
> + Inst.setAvailableFeatures(STI.getFeatureBits());
>
> // First, try a direct match.
> switch (MatchInstructionImpl(Operands, Inst,
> diff --git a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> index 4392de2..84ccf39 100644
> --- a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> +++ b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> @@ -42,20 +42,23 @@ public:
>
> ~X86MCCodeEmitter() {}
>
> - bool is64BitMode() const {
> - // FIXME: Can tablegen auto-generate this?
> - return (STI.getFeatureBits() & X86::Mode64Bit) != 0;
> + bool is64BitMode(const MCInst &MI) const {
> + uint64_t features = MI.isAvailableFeaturesSet() ?
> + MI.getAvailableFeatures() : STI.getFeatureBits();
> + return (features & X86::Mode64Bit) != 0;
> }
>
> - bool is32BitMode() const {
> - // FIXME: Can tablegen auto-generate this?
> - return (STI.getFeatureBits() & X86::Mode64Bit) == 0 &&
> - (STI.getFeatureBits() & X86::Mode16Bit) == 0;
> + bool is32BitMode(const MCInst &MI) const {
> + uint64_t features = MI.isAvailableFeaturesSet() ?
> + MI.getAvailableFeatures() : STI.getFeatureBits();
> + return (features & X86::Mode64Bit) == 0 &&
> + (features & X86::Mode16Bit) == 0;
> }
>
> - bool is16BitMode() const {
> - // FIXME: Can tablegen auto-generate this?
> - return (STI.getFeatureBits() & X86::Mode16Bit) != 0;
> + bool is16BitMode(const MCInst &MI) const {
> + uint64_t features = MI.isAvailableFeaturesSet() ?
> + MI.getAvailableFeatures() : STI.getFeatureBits();
> + return (features & X86::Mode16Bit) != 0;
> }
>
> /// Is16BitMemOperand - Return true if the specified instruction has
> @@ -65,7 +68,7 @@ public:
> const MCOperand &IndexReg = MI.getOperand(Op+X86::AddrIndexReg);
> const MCOperand &Disp = MI.getOperand(Op+X86::AddrDisp);
>
> - if (is16BitMode() && BaseReg.getReg() == 0 &&
> + if (is16BitMode(MI) && BaseReg.getReg() == 0 &&
> Disp.isImm() && Disp.getImm() < 0x10000)
> return true;
> if ((BaseReg.getReg() != 0 &&
> @@ -387,7 +390,7 @@ void X86MCCodeEmitter::EmitMemModRMByte(const MCInst
> &MI, unsigned Op,
>
> // Handle %rip relative addressing.
> if (BaseReg == X86::RIP) { // [disp32+RIP] in X86-64 mode
> - assert(is64BitMode() && "Rip-relative addressing requires 64-bit
> mode");
> + assert(is64BitMode(MI) && "Rip-relative addressing requires 64-bit
> mode");
> assert(IndexReg.getReg() == 0 && "Invalid rip-relative address");
> EmitByte(ModRMByte(0, RegOpcodeField, 5), CurByte, OS);
>
> @@ -486,7 +489,7 @@ void X86MCCodeEmitter::EmitMemModRMByte(const MCInst
> &MI, unsigned Op,
> BaseRegNo != N86::ESP &&
> // If there is no base register and we're in 64-bit mode, we need a
> SIB
> // byte to emit an addr that is just 'disp32' (the non-RIP relative
> form).
> - (!is64BitMode() || BaseReg != 0)) {
> + (!is64BitMode(MI) || BaseReg != 0)) {
>
> if (BaseReg == 0) { // [disp32] in X86-32 mode
> EmitByte(ModRMByte(0, RegOpcodeField, 5), CurByte, OS);
> @@ -1165,18 +1168,18 @@ void X86MCCodeEmitter::EmitOpcodePrefix(uint64_t
> TSFlags, unsigned &CurByte,
> // The AdSize prefix is only for 32-bit and 64-bit modes; in 16-bit
> mode we
> // need the address override only for JECXZ instead. Since it's only one
> // instruction, we special-case it rather than introducing an AdSize16
> bit.
> - if ((!is16BitMode() && TSFlags & X86II::AdSize) ||
> - (is16BitMode() && MI.getOpcode() == X86::JECXZ_32)) {
> + if ((!is16BitMode(MI) && TSFlags & X86II::AdSize) ||
> + (is16BitMode(MI) && MI.getOpcode() == X86::JECXZ_32)) {
> need_address_override = true;
> } else if (MemOperand == -1) {
> need_address_override = false;
> - } else if (is64BitMode()) {
> + } else if (is64BitMode(MI)) {
> assert(!Is16BitMemOperand(MI, MemOperand));
> need_address_override = Is32BitMemOperand(MI, MemOperand);
> - } else if (is16BitMode()) {
> + } else if (is16BitMode(MI)) {
> assert(!Is64BitMemOperand(MI, MemOperand));
> need_address_override = !Is16BitMemOperand(MI, MemOperand);
> - } else if (is32BitMode()) {
> + } else if (is32BitMode(MI)) {
> assert(!Is64BitMemOperand(MI, MemOperand));
> need_address_override = Is16BitMemOperand(MI, MemOperand);
> } else {
> @@ -1187,7 +1190,7 @@ void X86MCCodeEmitter::EmitOpcodePrefix(uint64_t
> TSFlags, unsigned &CurByte,
> EmitByte(0x67, CurByte, OS);
>
> // Emit the operand size opcode prefix as needed.
> - if (TSFlags & (is16BitMode() ? X86II::OpSize16 : X86II::OpSize))
> + if (TSFlags & (is16BitMode(MI) ? X86II::OpSize16 : X86II::OpSize))
> EmitByte(0x66, CurByte, OS);
>
> bool Need0FPrefix = false;
> @@ -1234,7 +1237,7 @@ void X86MCCodeEmitter::EmitOpcodePrefix(uint64_t
> TSFlags, unsigned &CurByte,
>
> // Handle REX prefix.
> // FIXME: Can this come before F2 etc to simplify emission?
> - if (is64BitMode()) {
> + if (is64BitMode(MI)) {
> if (unsigned REX = DetermineREXPrefix(MI, TSFlags, Desc))
> EmitByte(0x40 | REX, CurByte, OS);
> }
>
>
> --
> David Woodhouse Open Source Technology Centre
> David.Woodhouse at intel.com Intel Corporation
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131221/087e65d4/attachment.html>
More information about the llvm-commits
mailing list