[MC] Support symbolic expressions in assembly directives

[phosek]

Summary

This matches the behavior of GNU assembler which supports symbolic
expressions in absolute expressions used in assembly directives.

[phosek]

This should fix the error I introduced in the previous change.

[phosek]

This should test failure I introduced in D8072, if someone can review the change, I'd be happy to land this to unbreak the tree.

[• rafael]

How about using -filetype=obj? You should still get a warning with that, no?

[phosek]

That's a better solution.

[• rafael]

LGTM

[phosek]

Shall I mark this as accepted?

[phosek]

The original change was reverted because of the test failure, so I've updated this revision to include both the original change and the updated test which is now passing.

[• rafael]

Up to you. Using phab is optional, so just a lgtm in the list is sufficient.

[• rafael]

Lgtm. Just be sure to run check-all before committing :-)

[phosek]

This is strange, I did run all the test several times on my machine (Ubuntu x86-64) and all tests passed, but after landing the change, I got several failures on different platforms such as OS X . The source of the failure is unclear to me as the expression emitted for .fill 1, 1, 10 is .fill 1, 1, 0x0 instead of .fill 1, 1, 0xa. I'm not sure if it's related to the platform, but I'll get an OS X machine and try to replicate the failure.

[phosek]

I should have also mentioned that I've reverted the change myself to avoid further build breakages, sorry about all the issues related to this change.

include/llvm/MC/MCObjectStreamer.h

   bool EmitRelocDirective(const MCExpr &Offset, StringRef Name,
                           const MCExpr *Expr, SMLoc Loc) override;
   void EmitFill(uint64_t NumBytes, uint8_t FillValue) override;
+  void emitFill(const MCExpr &NumBytes, uint64_t FillValue,
+                SMLoc Loc = SMLoc()) override;
+  void emitFill(const MCExpr &NumValues, int64_t Size, int64_t Expr,
+                SMLoc Loc = SMLoc()) override;
+
   void FinishImpl() override;
 
   /// Emit the absolute difference between two symbols if possible.

include/llvm/MC/MCStreamer.h

   /// This implements directives such as '.space'.
   virtual void EmitFill(uint64_t NumBytes, uint8_t FillValue);
 
+  /// \brief Emit \p Size bytes worth of the value specified by \p FillValue.
+  ///
+  /// This is used to implement assembler directives such as .space or .skip.
+  ///
+  /// \param NumBytes - The number of bytes to emit.
+  /// \param FillValue - The value to use when filling bytes.
+  /// \param Loc - The location of the expression for error reporting.
+  virtual void emitFill(const MCExpr &NumBytes, uint64_t FillValue,
+                        SMLoc Loc = SMLoc());
+
+  /// \brief Emit \p NumValues copies of \p Size bytes. Each \p Size bytes is
+  /// taken from the lowest order 4 bytes of \p Expr expression.
+  ///
+  /// This is used to implement assembler directives such as .fill.
+  ///
+  /// \param NumValues - The number of copies of \p Size bytes to emit.
+  /// \param Size - The size (in bytes) of each repeated value.
+  /// \param Expr - The expression from which \p Size bytes are used.
+  virtual void emitFill(uint64_t NumValues, int64_t Size, int64_t Expr);
+  virtual void emitFill(const MCExpr &NumValues, int64_t Size, int64_t Expr,
+                        SMLoc Loc = SMLoc());
+
   /// \brief Emit NumBytes worth of zeros.
   /// This function properly handles data in virtual sections.
   void EmitZeros(uint64_t NumBytes);

lib/MC/MCAsmStreamer.cpp

 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/SourceMgr.h"
 #include <cctype>
 
 using namespace llvm;

 
   void EmitFill(uint64_t NumBytes, uint8_t FillValue) override;
 
+  void emitFill(const MCExpr &NumBytes, uint64_t FillValue,
+                SMLoc Loc = SMLoc()) override;
+
+  void emitFill(uint64_t NumValues, int64_t Size, int64_t Expr) override;
+
+  void emitFill(const MCExpr &NumValues, int64_t Size, int64_t Expr,
+                SMLoc Loc = SMLoc()) override;
+
   void EmitValueToAlignment(unsigned ByteAlignment, int64_t Value = 0,
                             unsigned ValueSize = 1,
                             unsigned MaxBytesToEmit = 0) override;

 void MCAsmStreamer::EmitFill(uint64_t NumBytes, uint8_t FillValue) {
   if (NumBytes == 0) return;
 
+  const MCExpr *E = MCConstantExpr::create(NumBytes, getContext());
+  emitFill(*E, FillValue);
+}
+
+void MCAsmStreamer::emitFill(const MCExpr &NumBytes, uint64_t FillValue,
+                             SMLoc Loc) {
   if (const char *ZeroDirective = MAI->getZeroDirective()) {
-    OS << ZeroDirective << NumBytes;
+    // FIXME: Emit location directives
+    OS << ZeroDirective;
+    NumBytes.print(OS, MAI);
     if (FillValue != 0)
       OS << ',' << (int)FillValue;
     EmitEOL();
     return;
   }
 
-  // Emit a byte at a time.
-  MCStreamer::EmitFill(NumBytes, FillValue);
+  MCStreamer::emitFill(NumBytes, FillValue);
+}
+
+void MCAsmStreamer::emitFill(uint64_t NumValues, int64_t Size, int64_t Expr) {
+  if (NumValues == 0)
+    return;
+
+  const MCExpr *E = MCConstantExpr::create(NumValues, getContext());
+  emitFill(*E, Size, Expr);
+}
+
+void MCAsmStreamer::emitFill(const MCExpr &NumValues, int64_t Size,
+                             int64_t Expr, SMLoc Loc) {
+  // FIXME: Emit location directives
+  OS << "\t.fill\t";
+  NumValues.print(OS, MAI);
+  OS << ", " << Size << ", 0x";
+  OS.write_hex(truncateToSize(Expr, 32));
+  EmitEOL();
 }
 
 void MCAsmStreamer::EmitValueToAlignment(unsigned ByteAlignment, int64_t Value,

lib/MC/MCObjectStreamer.cpp

 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/TargetRegistry.h"
 using namespace llvm;
 

   insert(new MCFillFragment(FillValue, NumBytes));
 }
 
+void MCObjectStreamer::emitFill(const MCExpr &NumBytes, uint64_t FillValue,
+                                SMLoc Loc) {
+  MCDataFragment *DF = getOrCreateDataFragment();
+  flushPendingLabels(DF, DF->getContents().size());
+
+  int64_t IntNumBytes;
+  if (!NumBytes.evaluateAsAbsolute(IntNumBytes, getAssembler())) {
+    getContext().reportError(Loc, "expected absolute expression");
+    return;
+  }
+
+  if (IntNumBytes <= 0) {
+    getContext().reportError(Loc, "invalid number of bytes");
+    return;
+  }
+
+  EmitFill(IntNumBytes, FillValue);
+}
+
+void MCObjectStreamer::emitFill(const MCExpr &NumValues, int64_t Size,
+                                int64_t Expr, SMLoc Loc) {
+  int64_t IntNumValues;
+  if (!NumValues.evaluateAsAbsolute(IntNumValues, getAssembler())) {
+    getContext().reportError(Loc, "expected absolute expression");
+    return;
+  }
+
+  if (IntNumValues < 0) {
+    getContext().getSourceManager()->PrintMessage(
+        Loc, SourceMgr::DK_Warning,
+        "'.fill' directive with negative repeat count has no effect");
+    return;
+  }
+
+  MCStreamer::emitFill(IntNumValues, Size, Expr);
+}
+
 void MCObjectStreamer::FinishImpl() {
   // If we are generating dwarf for assembly source files dump out the sections.
   if (getContext().getGenDwarfForAssembly())

lib/MC/MCParser/AsmParser.cpp

 bool AsmParser::parseDirectiveZero() {
   checkForValidSection();
 
-  int64_t NumBytes;
-  if (parseAbsoluteExpression(NumBytes))
+  SMLoc NumBytesLoc = Lexer.getLoc();
+  const MCExpr *NumBytes;
+  if (parseExpression(NumBytes))
     return true;
 
   int64_t Val = 0;

 
   Lex();
 
-  getStreamer().EmitFill(NumBytes, Val);
+  getStreamer().emitFill(*NumBytes, Val, NumBytesLoc);
 
   return false;
 }

 bool AsmParser::parseDirectiveFill() {
   checkForValidSection();
 
-  SMLoc RepeatLoc = getLexer().getLoc();
-  int64_t NumValues;
-  if (parseAbsoluteExpression(NumValues))
+  SMLoc NumValuesLoc = Lexer.getLoc();
+  const MCExpr *NumValues;
+  if (parseExpression(NumValues))
     return true;
 
-  if (NumValues < 0) {
-    Warning(RepeatLoc,
-            "'.fill' directive with negative repeat count has no effect");
-    NumValues = 0;
-  }
-
   int64_t FillSize = 1;
   int64_t FillExpr = 0;
 

 
   if (FillSize < 0) {
     Warning(SizeLoc, "'.fill' directive with negative size has no effect");
-    NumValues = 0;
+    NumValues = MCConstantExpr::create(0, getStreamer().getContext());
   }
   if (FillSize > 8) {
     Warning(SizeLoc, "'.fill' directive with size greater than 8 has been truncated to 8");

   if (!isUInt<32>(FillExpr) && FillSize > 4)
     Warning(ExprLoc, "'.fill' directive pattern has been truncated to 32-bits");
 
-  if (NumValues > 0) {
-    int64_t NonZeroFillSize = FillSize > 4 ? 4 : FillSize;
-    FillExpr &= ~0ULL >> (64 - NonZeroFillSize * 8);
-    for (uint64_t i = 0, e = NumValues; i != e; ++i) {
-      getStreamer().EmitIntValue(FillExpr, NonZeroFillSize);
-      if (NonZeroFillSize < FillSize)
-        getStreamer().EmitIntValue(0, FillSize - NonZeroFillSize);
-    }
-  }
+  getStreamer().emitFill(*NumValues, FillSize, FillExpr, NumValuesLoc);
 
   return false;
 }

 bool AsmParser::parseDirectiveSpace(StringRef IDVal) {
   checkForValidSection();
 
-  int64_t NumBytes;
-  if (parseAbsoluteExpression(NumBytes))
+  SMLoc NumBytesLoc = Lexer.getLoc();
+  const MCExpr *NumBytes;
+  if (parseExpression(NumBytes))
     return true;
 
   int64_t FillExpr = 0;

 
   Lex();
 
-  if (NumBytes <= 0)
-    return TokError("invalid number of bytes in '" + Twine(IDVal) +
-                    "' directive");
-
   // FIXME: Sometimes the fill expr is 'nop' if it isn't supplied, instead of 0.
-  getStreamer().EmitFill(NumBytes, FillExpr);
+  getStreamer().emitFill(*NumBytes, FillExpr, NumBytesLoc);
 
   return false;
 }

lib/MC/MCStreamer.cpp

 /// EmitFill - Emit NumBytes bytes worth of the value specified by
 /// FillValue.  This implements directives such as '.space'.
 void MCStreamer::EmitFill(uint64_t NumBytes, uint8_t FillValue) {
-  const MCExpr *E = MCConstantExpr::create(FillValue, getContext());
   for (uint64_t i = 0, e = NumBytes; i != e; ++i)
-    EmitValue(E, 1);
+    EmitIntValue(FillValue, 1);
+}
+
+void MCStreamer::emitFill(uint64_t NumValues, int64_t Size, int64_t Expr) {
+  int64_t NonZeroSize = Size > 4 ? 4 : Size;
+  Expr &= ~0ULL >> (64 - NonZeroSize * 8);
+  for (uint64_t i = 0, e = NumValues; i != e; ++i) {
+    EmitIntValue(Expr, NonZeroSize);
+    if (NonZeroSize < Size)
+      EmitIntValue(0, Size - NonZeroSize);
+  }
 }
 
 /// The implementation in this class just redirects to EmitFill.

 }
 void MCStreamer::EmitULEB128Value(const MCExpr *Value) {}
 void MCStreamer::EmitSLEB128Value(const MCExpr *Value) {}
+void MCStreamer::emitFill(const MCExpr &NumBytes, uint64_t Value, SMLoc Loc) {}
+void MCStreamer::emitFill(const MCExpr &NumValues, int64_t Size, int64_t Expr,
+                          SMLoc Loc) {}
 void MCStreamer::EmitValueToAlignment(unsigned ByteAlignment, int64_t Value,
                                       unsigned ValueSize,
                                       unsigned MaxBytesToEmit) {}

test/MC/AsmParser/directive_fill.s

 # RUN: llvm-mc -triple i386-unknown-unknown %s 2> %t.err | FileCheck %s
 # RUN: FileCheck --check-prefix=CHECK-WARNINGS %s < %t.err
+# RUN: llvm-mc -triple i386-unknown-unknown -filetype=obj -o %t.o %s 2> %t.err
+# RUN: FileCheck --check-prefix=OBJ-WARNINGS %s < %t.err
 
 # CHECK: TEST0:
-# CHECK: .byte 10
+# CHECK: .fill 1, 1, 0xa
 TEST0:  
         .fill 1, 1, 10
 
 # CHECK: TEST1:
-# CHECK: .short 3
-# CHECK: .short 3
+# CHECK: .fill 2, 2, 0x3
 TEST1:  
         .fill 2, 2, 3
 
 # CHECK: TEST2:
-# CHECK: .long 4
-# CHECK: .long 0
+# CHECK: .fill 1, 8, 0x4
 TEST2:  
         .fill 1, 8, 4
 
 # CHECK: TEST3
-# CHECK: .byte 0
-# CHECK: .byte 0
-# CHECK: .byte 0
-# CHECK: .byte 0
+# CHECK: .fill 4
 TEST3:
   .fill 4
 
 # CHECK: TEST4
-# CHECK: .short 0
-# CHECK: .short 0
-# CHECK: .short 0
-# CHECK: .short 0
+# CHECK: .fill 4, 2
 TEST4:
   .fill 4, 2
 
 # CHECK: TEST5
-# CHECK: .short  2
-# CHECK: .byte   0
-# CHECK: .short  2
-# CHECK: .byte   0
-# CHECK: .short  2
-# CHECK: .byte   0
-# CHECK: .short  2
-# CHECK: .byte   0
+# CHECK: .fill 4, 3, 0x2
 TEST5:
   .fill 4, 3, 2
 
 # CHECK: TEST6
-# CHECK: .long 2
-# CHECK: .long 0
+# CHECK: .fill 1, 8, 0x2
 # CHECK-WARNINGS: '.fill' directive with size greater than 8 has been truncated to 8
 TEST6:
   .fill 1, 9, 2
 
 # CHECK: TEST7
-# CHECK: .long 0
-# CHECK: .long 0
+# CHECK: .fill 1, 8, 0x100000000
 # CHECK-WARNINGS: '.fill' directive pattern has been truncated to 32-bits
 TEST7:
   .fill 1, 8, 1<<32
 
-# CHECK-WARNINGS: '.fill' directive with negative repeat count has no effect
+# CHECK: TEST8
+# CHECK: .fill -1, 8, 0x1
+# OBJ-WARNINGS: '.fill' directive with negative repeat count has no effect
 TEST8:
   .fill -1, 8, 1
 

   .fill 1, -1, 1
 
 # CHECK: TEST10
-# CHECK: .short  22136
-# CHECK: .byte   52
+# CHECK: .fill 1, 3, 0x12345678
 TEST10:
   .fill 1, 3, 0x12345678
+
+# CHECK: TEST11
+# CHECK: .fill TEST11-TEST10, 1, 0x0
+TEST11:
+  .fill TEST11 - TEST10
+
+# CHECK: TEST12
+# CHECK: .fill TEST11-TEST12, 3, 0x12345678
+# OBJ-WARNINGS: '.fill' directive with negative repeat count has no effect
+TEST12:
+  .fill TEST11 - TEST12, 3, 0x12345678

test/MC/AsmParser/directive_space.s

 # CHECK: .space 1
 TEST2:
         .skip 1
+
+# CHECK: TEST3
+# CHECK: .space TEST0-TEST1
+TEST3:
+        .space TEST0 - TEST1

test/MC/AsmParser/symbolic-expression.s

+# RUN: llvm-mc -filetype=obj -triple=i386-unknown-elf %s | llvm-objdump -t - | FileCheck %s
+
+# CHECK: 00000000         .text           00000000 TEST0
+TEST0:
+  .fill 0x10
+# CHECK: 00000010         .text           00000000 TEST1
+TEST1:
+  .fill TEST1 - TEST0 + 0x5
+# CHECK: 00000025         .text           00000000 TEST2
+TEST2:
+  .zero TEST2 - (TEST1 + 0x5)
+# CHECK: 00000035         .text           00000000 TEST3
+TEST3:
+  .skip (TEST1 - TEST0) * 2
+# CHECK: 00000055         .text           00000000 TEST4
+TEST4:
+  .space TEST2 - TEST1, 1