ARM IAS: support .inst directive

[compnerd]

Summary

This adds support for the .inst directive. This is an ARM specific directive to
indicate an instruction encoded as a constant expression. The major difference
between .word, .short, or .byte and .inst is that the latter will be
disassembled as an instruction since it does not get flagged as data.

[rengolin]

Hi Saleem,

Thanks for working on this, please update bug http://llvm.org/PR18199 after this has gone in to show that .inst is implemented (and give the commit number).

Also, please use "diff -U999" so that we can see the context before and after each change, as this way I have to go back and forth to an editor to understand which functions and classes were changed.

[t.p.northover]

Hi Saleem,

This looks pretty good, despite the comments I've added. One thing I'd say that didn't attach to any particular line is that a test for llvm-mc's text printing would be a good idea (i.e. ARMTargetAsmStreamer::emitInst).

Cheers.

Tim.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

8303

I agree with Joerg that we probably shouldn't have redundant arguments like Width and Suffix.

8330–8337

It seems a little odd that "size == 2" always produces an error but "size == 4" a warning.

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

196

Size is unused in this function.

390

I think I'd prefer "Size == 2 || Size == 4".

393–394

I believe instructions are always stored in little-endian format on ARM (well, approximately, it seems to be configurable in R-class processors to avoid annoying people with brain-dead legacy code: see A3.3.1).

If so, this logic is probably unnecessary.

399

I'm reasonably sure there are more even numbers than 4.

[compnerd]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

8330–8337

I modelled the behaviour against GAS. I would argue that relying on the truncation to be a warning is bad form, and have made this an error as well. While at it, introduced an additional negative test.

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

196

Inlined the size selection.

393–394

Unfortunately, for compatibility with GAS, we need to do this stupid swapping :-(.

399

Bad message; fixed :-).

[compnerd]

Address Tim's comments; add additional context for Renato.

[t.p.northover]

I believe instructions are always stored in little-endian format on ARM (well, approximately, it seems to be configurable in R-class processors to avoid annoying people with brain-dead legacy code: see A3.3.1).

If so, this logic is probably unnecessary.

Unfortunately, for compatibility with GAS, we need to do this stupid swapping :-(.

I've just been scarred for life by digging into what goes on.
Apparently the linker gets the job of swapping the byte order on all
instructions back to little-endian, so the swapping *is* needed.

Other than that, I've just got one more small point (the rest looks
fine to me). The error message ""inst.w operand is too big" seems to
be printed in ARM mode, where it's a bit misleading.

Cheers.

Tim.

[rengolin]

Hi Saleem,

Thanks for the context, it's a lot easier to understand the diffs that way. ;)

The patch looks good to me, too (apart from a few comments below), and the tests are really good, thanks!

cheers,
--renato

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

8362

Nit pick on style, this should be before the block to avoid extra indentation:

const MCConstantExpr *Value = dyn_cast_or_null<MCConstantExpr>(Expr);
if (!Value)
  return Error(Loc, "expected constant expression");

switch (Width) {
...

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

386

Is it possible to put an assert here to make sure we're in the right mode instead of assuming the parser did a good job?

[compnerd]

Good point on the diagnostics, Tim. I've made the diagnostics more appropriate and added an additional test.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

8362

Done

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

386

I like more strictness, added an assert(!IsThumb) and assert(IsThumb) before the mapping symbol emission appropriately.

include/llvm/MC/MCStreamer.h

virtual void emitRegSave(const SmallVectorImpl<unsigned> &RegList,
                            bool isVector) = 0;
 
   virtual void switchVendor(StringRef Vendor) = 0;
   virtual void emitAttribute(unsigned Attribute, unsigned Value) = 0;
   virtual void emitTextAttribute(unsigned Attribute, StringRef String) = 0;
   virtual void emitFPU(unsigned FPU) = 0;
   virtual void emitArch(unsigned Arch) = 0;
   virtual void finishAttributeSection() = 0;
+  virtual void emitInst(uint32_t Inst, char Suffix = '\0') = 0;
 };
 
 /// MCStreamer - Streaming machine code generation interface.  This interface
 /// is intended to provide a programatic interface that is very similar to the
 /// level that an assembler .s file provides.  It has callbacks to emit bytes,
 /// handle directives, etc.  The implementation of this interface retains
 /// state to know what the current section is etc.
 ///

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

 #include "MCTargetDesc/ARMAddressingModes.h"
 #include "MCTargetDesc/ARMArchName.h"
 #include "MCTargetDesc/ARMBaseInfo.h"
 #include "MCTargetDesc/ARMMCExpr.h"
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/OwningPtr.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCELFStreamer.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"

   bool parseDirectiveFnStart(SMLoc L);
   bool parseDirectiveFnEnd(SMLoc L);
   bool parseDirectiveCantUnwind(SMLoc L);
   bool parseDirectivePersonality(SMLoc L);
   bool parseDirectiveHandlerData(SMLoc L);
   bool parseDirectiveSetFP(SMLoc L);
   bool parseDirectivePad(SMLoc L);
   bool parseDirectiveRegSave(SMLoc L, bool IsVector);
+  bool parseDirectiveInst(SMLoc L, char Suffix = '\0');
 
   StringRef splitMnemonic(StringRef Mnemonic, unsigned &PredicationCode,
                           bool &CarrySetting, unsigned &ProcessorIMod,
                           StringRef &ITMask);
   void getMnemonicAcceptInfo(StringRef Mnemonic, StringRef FullInst,
                              bool &CanAcceptCarrySet,
                              bool &CanAcceptPredicationCode);
 

   else if (IDVal == ".setfp")
     return parseDirectiveSetFP(DirectiveID.getLoc());
   else if (IDVal == ".pad")
     return parseDirectivePad(DirectiveID.getLoc());
   else if (IDVal == ".save")
     return parseDirectiveRegSave(DirectiveID.getLoc(), false);
   else if (IDVal == ".vsave")
     return parseDirectiveRegSave(DirectiveID.getLoc(), true);
+  else if (IDVal == ".inst")
+    return parseDirectiveInst(DirectiveID.getLoc());
+  else if (IDVal == ".inst.n")
+    return parseDirectiveInst(DirectiveID.getLoc(), 'n');
+  else if (IDVal == ".inst.w")
+    return parseDirectiveInst(DirectiveID.getLoc(), 'w');
   return true;
 }
 
 /// parseDirectiveWord
 ///  ::= .word [ expression (, expression)* ]
 bool ARMAsmParser::parseDirectiveWord(unsigned Size, SMLoc L) {
   if (getLexer().isNot(AsmToken::EndOfStatement)) {
     for (;;) {

     return Error(L, ".save expects GPR registers");
   if (IsVector && !Op->isDPRRegList())
     return Error(L, ".vsave expects DPR registers");
 
   getTargetStreamer().emitRegSave(Op->getRegList(), IsVector);
   return false;
 }
 
+/// parseDirectiveInst
+///  ::= .inst opcode [, ...]
+///  ::= .inst.n opcode [, ...]
+///  ::= .inst.w opcode [, ...]
+bool ARMAsmParser::parseDirectiveInst(SMLoc Loc, char Suffix) {
+  int Width;
+
+  if (isThumb()) {
+    switch (Suffix) {
+    case 'n':
+      Width = 2;
+      break;
+    case 'w':
+      Width = 4;
+      break;
+    default:
+      Parser.eatToEndOfStatement();
+      return Error(Loc, "cannot determine Thumb instruction size, "
+                        "use inst.n/inst.w instead");
+    }
+  } else {
+    if (Suffix) {
+      Parser.eatToEndOfStatement();
+      return Error(Loc, "width suffixes are invalid in ARM mode");
+    }
+    Width = 4;
+  }
+
+  if (getLexer().is(AsmToken::EndOfStatement)) {
+    Parser.eatToEndOfStatement();
+    return Error(Loc, "expected expression following directive");
+  }
+
+  for (;;) {
+    const MCExpr *Expr;
+
+    if (getParser().parseExpression(Expr))
+      return Error(Loc, "expected expression");
+
+    if (const MCConstantExpr *Value = dyn_cast_or_null<MCConstantExpr>(Expr)) {
+      switch (Width) {
+      case 2:
+        if (Value->getValue() > 0xffff)
+          return Error(Loc, "inst.n operand is too big, use inst.w instead");
+        break;
+      case 4:
+        if (Value->getValue() > 0xffffffff)
+          return Error(Loc, "inst.w operand is too big");
+        break;
+      default:
+        llvm_unreachable("only supported widths are 2 and 4");
+      }
+
+      getTargetStreamer().emitInst(Value->getValue(), Suffix);
+
+      if (getLexer().is(AsmToken::EndOfStatement))
+        break;
+
+      if (getLexer().isNot(AsmToken::Comma))
+        return Error(Loc, "unexpected token in directive");
+
+      Parser.Lex();
+    } else {
+      return Error(Loc, "expected constant expression");
+    }
+  }
+
+  Parser.Lex();
+  return false;
+}
+
 /// Force static initialization.
 extern "C" void LLVMInitializeARMAsmParser() {
   RegisterMCAsmParser<ARMAsmParser> X(TheARMTarget);
   RegisterMCAsmParser<ARMAsmParser> Y(TheThumbTarget);
 }
 
 #define GET_REGISTER_MATCHER
 #define GET_SUBTARGET_FEATURE_NAME

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

 
 #include "ARMBuildAttrs.h"
 #include "ARMArchName.h"
 #include "ARMFPUName.h"
 #include "ARMRegisterInfo.h"
 #include "ARMUnwindOp.h"
 #include "ARMUnwindOpAsm.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/MC/MCAsmBackend.h"
+#include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCELF.h"
 #include "llvm/MC/MCELFStreamer.h"
 #include "llvm/MC/MCELFSymbolFlags.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
class ARMTargetAsmStreamer : public ARMTargetStreamer {
   virtual void emitRegSave(const SmallVectorImpl<unsigned> &RegList,
                            bool isVector);
 
   virtual void switchVendor(StringRef Vendor);
   virtual void emitAttribute(unsigned Attribute, unsigned Value);
   virtual void emitTextAttribute(unsigned Attribute, StringRef String);
   virtual void emitArch(unsigned Arch);
   virtual void emitFPU(unsigned FPU);
+  virtual void emitInst(uint32_t Inst, char Suffix = '\0');
   virtual void finishAttributeSection();
 
 public:
   ARMTargetAsmStreamer(formatted_raw_ostream &OS, MCInstPrinter &InstPrinter);
 };
 
 ARMTargetAsmStreamer::ARMTargetAsmStreamer(formatted_raw_ostream &OS,
                                            MCInstPrinter &InstPrinter)
void ARMTargetAsmStreamer::emitArch(unsigned Arch) {
   OS << "\t.arch\t" << GetArchName(Arch) << "\n";
 }
 void ARMTargetAsmStreamer::emitFPU(unsigned FPU) {
   OS << "\t.fpu\t" << GetFPUName(FPU) << "\n";
 }
 void ARMTargetAsmStreamer::finishAttributeSection() {
 }
 
+void ARMTargetAsmStreamer::emitInst(uint32_t Inst, char Suffix) {
+  OS << "\t.inst";
+  if (Suffix)
+    OS << "." << Suffix;
+  OS << "\t0x" << utohexstr(Inst) << "\n";
+}
+
 class ARMTargetELFStreamer : public ARMTargetStreamer {
 private:
   // This structure holds all attributes, accounting for
   // their string/numeric value, so we can later emmit them
   // in declaration order, keeping all in the same vector
   struct AttributeItem {
     enum {
       HiddenAttribute = 0,
private:
   virtual void emitRegSave(const SmallVectorImpl<unsigned> &RegList,
                            bool isVector);
 
   virtual void switchVendor(StringRef Vendor);
   virtual void emitAttribute(unsigned Attribute, unsigned Value);
   virtual void emitTextAttribute(unsigned Attribute, StringRef String);
   virtual void emitArch(unsigned Arch);
   virtual void emitFPU(unsigned FPU);
+  virtual void emitInst(uint32_t Inst, char Suffix = '\0');
   virtual void finishAttributeSection();
 
   size_t calculateContentSize() const;
 
 public:
   ARMTargetELFStreamer()
     : ARMTargetStreamer(), CurrentVendor("aeabi"), FPU(ARM::INVALID_FPU),
       Arch(ARM::INVALID_ARCH), AttributeSection(0) {
virtual void EmitInstruction(const MCInst& Inst) {
     if (IsThumb)
       EmitThumbMappingSymbol();
     else
       EmitARMMappingSymbol();
 
     MCELFStreamer::EmitInstruction(Inst);
   }
 
+  virtual void emitInst(uint32_t Inst, char Suffix) {
+    unsigned Size;
+    char Buffer[4];
+    const bool LittleEndian = getContext().getAsmInfo()->isLittleEndian();
+
+    switch (Suffix) {
+    case '\0':
+      Size = 4;
+
+      EmitARMMappingSymbol();
+      for (unsigned II = 0, IE = Size; II != IE; II++) {
+        const unsigned I = LittleEndian ? (Size - II - 1) : II;
+        Buffer[Size - II - 1] = uint8_t(Inst >> I * CHAR_BIT);
+      }
+
+      break;
+    case 'n':
+    case 'w':
+      Size = (Suffix == 'n' ? 2 : 4);
+
+      EmitThumbMappingSymbol();
+      for (unsigned II = 0, IE = Size; II != IE; II = II + 2) {
+        const unsigned I0 = LittleEndian ? II + 0 : (Size - II - 1);
+        const unsigned I1 = LittleEndian ? II + 1 : (Size - II - 2);
+        Buffer[Size - II - 2] = uint8_t(Inst >> I0 * CHAR_BIT);
+        Buffer[Size - II - 1] = uint8_t(Inst >> I1 * CHAR_BIT);
+      }
+
+      break;
+    default:
+      llvm_unreachable("Invalid Suffix");
+    }
+
+    MCELFStreamer::EmitBytes(StringRef(Buffer, Size));
+  }
+
   /// This is one of the functions used to emit data into an ELF section, so the
   /// ARM streamer overrides it to add the appropriate mapping symbol ($d) if
   /// necessary.
   virtual void EmitBytes(StringRef Data) {
     EmitDataMappingSymbol();
     MCELFStreamer::EmitBytes(Data);
   }
 
case AttributeItem::TextAttribute:
       Streamer.EmitIntValue(0, 1); // '\0'
       break;
     }
   }
 
   Contents.clear();
   FPU = ARM::INVALID_FPU;
 }
+void ARMTargetELFStreamer::emitInst(uint32_t Inst, char Suffix) {
+  getStreamer().emitInst(Inst, Suffix);
+}
 
 void ARMELFStreamer::FinishImpl() {
   MCTargetStreamer &TS = getTargetStreamer();
   ARMTargetStreamer &ATS = static_cast<ARMTargetStreamer &>(TS);
   ATS.finishAttributeSection();
 
   MCELFStreamer::FinishImpl();
 }

test/MC/ARM/inst-arm-suffixes.s

+@ RUN: llvm-mc %s -triple armv7-linux-gnueabi -filetype asm -o - 2> %t \
+@ RUN:   | FileCheck --check-prefix CHECK-ERRORS %s %t
+
+  .syntax unified
+  .arm
+
+  .align 2
+  .global suffixes_invalid_in_arm
+  .type suffixes_invalid_in_arm,%function
+suffixes_invalid_in_arm:
+  .inst.n 2
+@ CHECK-ERRORS: width suffixes are invalid in ARM mode
+  .inst.w 4
+@ CHECK-ERRORS: width suffixes are invalid in ARM mode
+

test/MC/ARM/inst-constant-required.s

+@ RUN: llvm-mc %s -triple=armv7-linux-gnueabi -filetype asm -o - 2> %t \
+@ RUN:   | FileCheck --check-prefix CHECK-ERRORS %s %t
+
+  .syntax unified
+  .arm
+
+  .align 2
+  .global constant_expression_required
+  .type constant_expression_required,%function
+constant_expression_required:
+.Label:
+  movs r0, r0
+  .inst .Label
+@ CHECK-ERRORS: expected constant expression
+

test/MC/ARM/inst-directive-emit.s

+@ RUN: llvm-mc %s -triple armv7-linux-gnueabi -filetype asm -o - | FileCheck %s
+
+  .syntax unified
+  .thumb
+
+  .align 2
+  .global emit_asm
+  .type emit_asm,%function
+emit_asm:
+  .inst.w 0xf2400000, 0xf2c00000
+
+@ CHECK:  .text
+@ CHECK:  .code 16
+@ CHECK:  .align  2
+@ CHECK:  .globl  emit_asm
+@ CHECK:  .type emit_asm,%function
+@ CHECK: emit_asm:
+@ CHECK:  inst.w 0xF2400000
+@ CHECK:  inst.w 0xF2C00000
+

test/MC/ARM/inst-directive.s

+@ RUN: llvm-mc %s -triple=armv7-linux-gnueabi -filetype=obj -o - \
+@ RUN:   | llvm-readobj -s -sd | FileCheck %s
+
+  .syntax unified
+
+@-------------------------------------------------------------------------------
+@ arm_inst
+@-------------------------------------------------------------------------------
+  .arm
+
+  .section  .inst.arm_inst
+
+  .align  2
+  .global arm_inst
+  .type arm_inst,%function
+arm_inst:
+  .inst 0xdefe
+
+@ CHECK: Section {
+@ CHECK:   Name: .inst.arm_inst
+@ CHECK:   SectionData (
+@ CHECK-NEXT:     0000: FEDE0000
+@ CHECK-NEXT:   )
+
+@-------------------------------------------------------------------------------
+@ thumb_inst_n
+@-------------------------------------------------------------------------------
+  .thumb
+
+  .section  .inst.thumb_inst_n
+
+  .align  2
+  .global thumb_inst_n
+  .type thumb_inst_n,%function
+thumb_inst_n:
+  .inst.n 0xdefe
+
+@ CHECK: Section {
+@ CHECK:   Name: .inst.thumb_inst_n
+@ CHECK:   SectionData (
+@ CHECK-NEXT:     0000: FEDE
+@ CHECK-NEXT:   )
+
+@-------------------------------------------------------------------------------
+@ thumb_inst_w
+@-------------------------------------------------------------------------------
+  .thumb
+
+  .section  .inst.thumb_inst_w
+
+  .align  2
+  .global thumb_inst_w
+  .type thumb_inst_w,%function
+thumb_inst_w:
+  .inst.w 0x00000000
+
+@ CHECK: Section {
+@ CHECK:   Name: .inst.thumb_inst_w
+@ CHECK:   SectionData (
+@ CHECK-NEXT:     0000: 00000000
+@ CHECK-NEXT:   )
+
+@-------------------------------------------------------------------------------
+@ thumb_inst_w
+@-------------------------------------------------------------------------------
+  .thumb
+
+  .section  .inst.thumb_inst_inst
+
+  .align  2
+  .global thumb_inst_inst
+  .type thumb_inst_inst,%function
+thumb_inst_inst:
+  .inst.w 0xf2400000, 0xf2c00000
+
+@ CHECK: Section {
+@ CHECK:   Name: .inst.thumb_inst_inst
+@ CHECK:   SectionData (
+@ CHECK-NEXT:     0000: 40F20000 C0F20000
+@ CHECK-NEXT:   )
+

test/MC/ARM/inst-thumb-overflow-2.s

+@ RUN: llvm-mc %s -triple armv7-linux-gnueabi -filetype asm -o - 2> %t \
+@ RUN:   | FileCheck --check-prefix CHECK-ERRORS %s %t
+
+  .syntax unified
+  .thumb
+
+  .align 2
+  .global constant_overflow
+  .type constant_overflow,%function
+constant_overflow:
+  .inst.w 1 << 32
+@ CHECK-ERRORS: inst.w operand is too big
+

test/MC/ARM/inst-thumb-overflow.s

+@ RUN: llvm-mc %s -triple armv7-linux-gnueabi -filetype asm -o - 2> %t \
+@ RUN:   | FileCheck --check-prefix CHECK-ERRORS %s %t
+
+  .syntax unified
+  .thumb
+
+  .align 2
+  .global constant_overflow
+  .type constant_overflow,%function
+constant_overflow:
+  .inst.n 1 << 31
+@ CHECK-ERRORS: inst.n operand is too big, use inst.w instead
+

test/MC/ARM/inst-thumb-suffixes.s

+@ RUN: llvm-mc %s -triple armv7-linux-gnueabi -filetype asm -o - 2> %t \
+@ RUN:   | FileCheck --check-prefix CHECK-ERRORS %s %t
+
+  .syntax unified
+  .thumb
+
+  .align 2
+  .global suffixes_required_in_thumb
+  .type suffixes_required_in_thumb,%function
+suffixes_required_in_thumb:
+  .inst 0x0000
+@ CHECK-ERRORS: cannot determine Thumb instruction size, use inst.n/inst.w instead
+