[MC] Relax .fill size requirements

[niravd]

Summary

Avoid requirement that number of values to fill must be known as assembler time.

Fixes PR33586.

[rnk]

llvm/lib/MC/MCObjectStreamer.cpp

646–662

Would it be cleaner to generalize MCFillFragment and then we could defer this label difference calculation until after relaxation?

llvm/lib/MC/MCParser/AsmParser.cpp

3144 ↗

(On Diff #146146)

Given that this is the only change in the file, I'd revert it.

[peter.smith]

llvm/lib/MC/MCObjectStreamer.cpp

646–662

I tend to agree with rnk here. I'm guessing that if there are size concerns about adding fields to MCFillFragment that are not commonly used then it could be worth a new fragment type, potentially derived from MCFillFragment.

647

Given that NumValues is passed to emitFill(); did you mean // Byte fills don't need to know IntNumValues?

llvm/test/MC/X86/pr33586.s

1 ↗

(On Diff #146146)

The test looks very similar to the attachment in the PR. Given that the code was from the Linux Kernel we probably ought to generalise and simplify it to avoid any GPL concerns.

[niravd]

Generalize to non-byte fills as well.

[peter.smith]

Thanks for the update. I've left some minor comments but I'm overall happy with the change.

llvm/lib/MC/MCAssembler.cpp

563–587

Maybe worth a comment summarising the specification of how .fill forms the value, or hint that the interested reader should look at the GNU as directives manual for a description.

568

I suggest removing the newline here as both loops are constructing Data.

572

Perhaps // Set ChunkSize to largest multiple of VSize in Data
or do unsigned ChunkSize = MaxChunkSize - MaxChunkSize % VSize
It could also be const unsigned.

578

I don't think you need the {} in this case.

581

Could be possible to compress without losing readability, although feel free to ignore as this is just a style choice:
if (unsigned TrailingCount = FragmentSize % ChunkSize)

OW->writeBytes(StringRef(Data, TrailingCount));

llvm/lib/MC/MCObjectStreamer.cpp

666–668

This bit of code is almost identical to emitFill above, the only difference being that Size is used rather than 1. It may be worth passing Size to emitFill above (perhaps with a default value of 1).

llvm/test/MC/AsmParser/assembler-expressions.s

57

I think we haven't got a big-endian test for .fill when the size is > 1. I think there is a Mips test that uses .fill (hilo-addressing) but it is only .fill 0x30124-8

Probably worth adding one to make sure that the endian reversal code is working.

[niravd]

Address comments. The only pending comment is regards a big-endian example.

[niravd]

llvm/test/MC/AsmParser/assembler-expressions.s

57

For this test to be properly exercised there must be a assembler-time unknown expression that we can resolve at layout time. AFAIK this must be a text section ambiguity due to instruction relaxation. The only in-tree big-endian machine that does relax instructions is ARM. It's proving to not be a simple transliteration of the other test though.

[peter.smith]

Thanks for the update, typo aside they look fine to me.

If it helps here is a program that should show relaxation on a big endian arm system:

// RUN: llvm-mc --triple=thumbv7eb-linux-gnueabihf %s -filetype=obj -o %t
// RUN: llvm-objdump -triple=thumbv7eb-linux-gnueabihf -d %t
        .syntax unified
        .text
        .thumb
.L1:
        beq Label
.L2:
        .word 0
        .fill (.L2 - .L1)/4, 4, 0x12345678
        .space 1024
Label:

llvm-objdump doesn't seem to be able to disassemble the instructions properly, but by using .word to force a .data mapping symbol it will disassemble the .fill. Big endian on an Armv7 is strange in that the object files have big-endian instructions but are reversed at link time to little-endian so I'm not surprised it doesn't handle this well.

llvm/lib/MC/MCAssembler.cpp

563

Typo: verctor -> vector

[niravd]

Fix typo. Add big-endian ARM test that avoids llvm-objdump big-endian thumb issues.

[peter.smith]

Thanks for the update, looks good to me.

llvm/include/llvm/MC/MCFragment.h

public:
 
   static bool classof(const MCFragment *F) {
     return F->getKind() == MCFragment::FT_Padding;
   }
 };
 
 class MCFillFragment : public MCFragment {
   /// Value to use for filling bytes.
-  uint8_t Value;
-
+  uint64_t Value;
+  uint8_t ValueSize;
   /// The number of bytes to insert.
-  const MCExpr &Size;
+  const MCExpr &NumValues;
 
   /// Source location of the directive that this fragment was created for.
   SMLoc Loc;
 
 public:
-  MCFillFragment(uint8_t Value, const MCExpr &Size, SMLoc Loc,
-                 MCSection *Sec = nullptr)
-      : MCFragment(FT_Fill, false, 0, Sec), Value(Value), Size(Size), Loc(Loc) {
-  }
-
-  uint8_t getValue() const { return Value; }
-  const MCExpr &getSize() const { return Size; }
+  MCFillFragment(uint64_t Value, uint8_t VSize, const MCExpr &NumValues,
+                 SMLoc Loc, MCSection *Sec = nullptr)
+      : MCFragment(FT_Fill, false, 0, Sec), Value(Value), ValueSize(VSize),
+        NumValues(NumValues), Loc(Loc) {}
+
+  uint64_t getValue() const { return Value; }
+  uint8_t getValueSize() const { return ValueSize; }
+  const MCExpr &getNumValues() const { return NumValues; }
 
   SMLoc getLoc() const { return Loc; }
 
   static bool classof(const MCFragment *F) {
     return F->getKind() == MCFragment::FT_Fill;
   }
 };
 

llvm/lib/MC/MCAssembler.cpp

uint64_t MCAssembler::computeFragmentSize(const MCAsmLayout &Layout,
   case MCFragment::FT_Data:
     return cast<MCDataFragment>(F).getContents().size();
   case MCFragment::FT_Relaxable:
     return cast<MCRelaxableFragment>(F).getContents().size();
   case MCFragment::FT_CompactEncodedInst:
     return cast<MCCompactEncodedInstFragment>(F).getContents().size();
   case MCFragment::FT_Fill: {
     auto &FF = cast<MCFillFragment>(F);
-    int64_t Size = 0;
-    if (!FF.getSize().evaluateAsAbsolute(Size, Layout))
+    int64_t NumValues = 0;
+    if (!FF.getNumValues().evaluateAsAbsolute(NumValues, Layout)) {
       getContext().reportError(FF.getLoc(),
                                "expected assembly-time absolute expression");
+      return 0;
+    }
+    int64_t Size = NumValues * FF.getValueSize();
     if (Size < 0) {
       getContext().reportError(FF.getLoc(), "invalid number of bytes");
       return 0;
     }
     return Size;
   }
 
   case MCFragment::FT_LEB:
static void writeFragment(const MCAssembler &Asm, const MCAsmLayout &Layout,
   case MCFragment::FT_CompactEncodedInst:
     ++stats::EmittedCompactEncodedInstFragments;
     OW->writeBytes(cast<MCCompactEncodedInstFragment>(F).getContents());
     break;
 
   case MCFragment::FT_Fill: {
     ++stats::EmittedFillFragments;
     const MCFillFragment &FF = cast<MCFillFragment>(F);
-    uint8_t V = FF.getValue();
+    uint64_t V = FF.getValue();
+    unsigned VSize = FF.getValueSize();
     const unsigned MaxChunkSize = 16;
     char Data[MaxChunkSize];
-    memcpy(Data, &V, 1);
-    for (unsigned I = 1; I < MaxChunkSize; ++I)
-      Data[I] = Data[0];
+    // Duplicate V into Data as byte verctor to reduce number of
+    // writes done. As such, do endian conversion here, not in OW.
+    const bool isLittleEndian = Asm.getContext().getAsmInfo()->isLittleEndian();
+    for (unsigned I = 0; I != VSize; ++I) {
+      unsigned index = isLittleEndian ? I : (VSize - I - 1);
+      Data[I] = uint8_t(V >> (index * 8));
+    }
+    for (unsigned I = VSize; I < MaxChunkSize; ++I)
+      Data[I] = Data[I - VSize];
+
+    // Set to largest multiple of VSize in Data.
+    const unsigned NumPerChunk = MaxChunkSize / VSize;
+    // Set ChunkSize to largest multiple of VSize in Data
+    const unsigned ChunkSize = VSize * NumPerChunk;
 
-    uint64_t Size = FragmentSize;
-    for (unsigned ChunkSize = MaxChunkSize; ChunkSize; ChunkSize /= 2) {
-      StringRef Ref(Data, ChunkSize);
-      for (uint64_t I = 0, E = Size / ChunkSize; I != E; ++I)
-        OW->writeBytes(Ref);
-      Size = Size % ChunkSize;
+    // Do copies by chunk.
+    for (uint64_t I = 0, E = FragmentSize / ChunkSize; I != E; ++I)
+
+    // do remainder if needed.
+    unsigned TrailingCount = FragmentSize % ChunkSize;
+    if (TrailingCount) {
+      StringRef RefTail(Data, TrailingCount);
+      OW->writeBytes(RefTail);
     }
     break;
   }
 
   case MCFragment::FT_LEB: {
     const MCLEBFragment &LF = cast<MCLEBFragment>(F);
     OW->writeBytes(LF.getContents());
     break;

llvm/lib/MC/MCFragment.cpp

for (unsigned i = 0, e = Contents.size(); i != e; ++i) {
       OS << hexdigit((Contents[i] >> 4) & 0xF) << hexdigit(Contents[i] & 0xF);
     }
     OS << "] (" << Contents.size() << " bytes)";
     break;
   }
   case MCFragment::FT_Fill:  {
     const MCFillFragment *FF = cast<MCFillFragment>(this);
     OS << " Value:" << static_cast<unsigned>(FF->getValue())
-       << " Size:" << FF->getSize();
+       << " ValueSize:" << static_cast<unsigned>(FF->getValueSize())
+       << " NumValues:" << FF->getNumValues();
     break;
   }
   case MCFragment::FT_Relaxable:  {
     const MCRelaxableFragment *F = cast<MCRelaxableFragment>(this);
     OS << "\n       ";
     OS << " Inst:";
     F->getInst().dump_pretty(OS);
     break;

llvm/lib/MC/MCObjectStreamer.cpp

 }
 
 void MCObjectStreamer::emitFill(const MCExpr &NumBytes, uint64_t FillValue,
                                 SMLoc Loc) {
   MCDataFragment *DF = getOrCreateDataFragment();
   flushPendingLabels(DF, DF->getContents().size());
 
   assert(getCurrentSectionOnly() && "need a section");
-  insert(new MCFillFragment(FillValue, NumBytes, Loc));
+  insert(new MCFillFragment(FillValue, 1, NumBytes, Loc));
 }
 
 void MCObjectStreamer::emitFill(const MCExpr &NumValues, int64_t Size,
                                 int64_t Expr, SMLoc Loc) {
-  int64_t IntNumValues;
-  if (!NumValues.evaluateAsAbsolute(IntNumValues, getAssemblerPtr())) {
-    getContext().reportError(Loc, "expected absolute expression");
-    return;
-  }
 
+  int64_t IntNumValues;
+  // Do additional checking now if we can resolve the value.
+  if (NumValues.evaluateAsAbsolute(IntNumValues, getAssemblerPtr())) {
     if (IntNumValues < 0) {
       getContext().getSourceManager()->PrintMessage(
           Loc, SourceMgr::DK_Warning,
           "'.fill' directive with negative repeat count has no effect");
       return;
     }
-
+    // Emit now if we can for better errors.
     int64_t NonZeroSize = Size > 4 ? 4 : Size;
     Expr &= ~0ULL >> (64 - NonZeroSize * 8);
     for (uint64_t i = 0, e = IntNumValues; i != e; ++i) {
       EmitIntValue(Expr, NonZeroSize);
       if (NonZeroSize < Size)
         EmitIntValue(0, Size - NonZeroSize);
     }
+    return;
+  }
+
+  // Otherwise emit as fragment.
+  MCDataFragment *DF = getOrCreateDataFragment();
+  flushPendingLabels(DF, DF->getContents().size());
+
+  assert(getCurrentSectionOnly() && "need a section");
+  insert(new MCFillFragment(Expr, Size, NumValues, Loc));
 }
 
 void MCObjectStreamer::EmitFileDirective(StringRef Filename) {
   getAssembler().addFileName(Filename);
 }
 
 void MCObjectStreamer::FinishImpl() {
   // If we are generating dwarf for assembly source files dump out the sections.

llvm/lib/MC/WasmObjectWriter.cpp

if (auto *Align = dyn_cast<MCAlignFragment>(&Frag)) {
       // If nops are requested, use zeros, as this is the data section.
       uint8_t Value = Align->hasEmitNops() ? 0 : Align->getValue();
       uint64_t Size = std::min<uint64_t>(alignTo(DataBytes.size(),
                                                  Align->getAlignment()),
                                          DataBytes.size() +
                                              Align->getMaxBytesToEmit());
       DataBytes.resize(Size, Value);
     } else if (auto *Fill = dyn_cast<MCFillFragment>(&Frag)) {
-      int64_t Size;
-      if (!Fill->getSize().evaluateAsAbsolute(Size))
+      int64_t NumValues;
+      if (!Fill->getNumValues().evaluateAsAbsolute(NumValues))
         llvm_unreachable("The fill should be an assembler constant");
-      DataBytes.insert(DataBytes.end(), Size, Fill->getValue());
+      DataBytes.insert(DataBytes.end(), Fill->getValueSize() * NumValues,
+                       Fill->getValue());
     } else {
       const auto &DataFrag = cast<MCDataFragment>(Frag);
       const SmallVectorImpl<char> &Contents = DataFrag.getContents();
 
       DataBytes.insert(DataBytes.end(), Contents.begin(), Contents.end());
     }
   }
 

llvm/test/MC/AsmParser/assembler-expressions.s

.else
    .byte 7
 .endif
 
 .byte 0xff
 
 # nop is a fixed size instruction so this should pass.
   
 # OBJTEXT: Contents of section .text
-# OBJTEXT-NEXT: 0000 9090ff34 25
+# OBJTEXT-NEXT: 0000 9090ff34 25000000 00909090 78563412
+# OBJTEXT-NEXT: 0010 78563412 90
 
 .text
   
 text1:
         nop
 # ASM-ERR: [[@LINE+1]]:5: error: expected absolute expression
 .if . - text1 == 1
   nop
 .else
   ret
 .endif
   push gs
-
+  nop
+  nop
+  nop
 # No additional errors.
 # 
 # ASM-ERR-NOT: {{[0-9]+}}:{{[0-9]+}}: error:
+
+  
+
+text2:
+  .long 0x12345678
+text3:
+  .fill (text3-text2)/4, 4, 0x12345678
+  nop
+
+
+
+
+
+  

llvm/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.err2
-# RUN: FileCheck --check-prefix=OBJ-WARNINGS %s < %t.err2
+# RUN: not llvm-mc -triple i386-unknown-unknown -filetype=obj -o %t.o %s 2> %t.err2
+# RUN: FileCheck --check-prefix=OBJ-ERRS %s < %t.err2
 
 # CHECK: TEST0:
 # CHECK: .fill 1, 1, 0xa
 TEST0:  
         .fill 1, 1, 10
 
 # CHECK: TEST1:
 # CHECK: .fill 2, 2, 0x3

 # CHECK: TEST7
 # CHECK: .fill 1, 8, 0x0
 # CHECK-WARNINGS: '.fill' directive pattern has been truncated to 32-bits
 TEST7:
   .fill 1, 8, 1<<32
 
 # CHECK: TEST8
 # CHECK: .fill -1, 8, 0x1
-# OBJ-WARNINGS: '.fill' directive with negative repeat count has no effect
+# OBJ-ERRS: '.fill' directive with negative repeat count has no effect
 TEST8:
   .fill -1, 8, 1
 
 # CHECK-WARNINGS: '.fill' directive with negative size has no effect
 TEST9:
   .fill 1, -1, 1
 
 # CHECK: TEST10
 # 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
+# CHECK: .fill TEST11-TEST12, 4, 0x12345678
+# OBJ-ERRS: '.fill' directive with negative repeat count has no effect
 TEST12:
-  .fill TEST11 - TEST12, 3, 0x12345678
+  .fill TEST11 - TEST12, 4, 0x12345678
+
+# CHECK: TEST13
+# CHECK: .fill (TEST11-TEST12)+i, 4, 0x12345678
+# OBJ-ERRS: [[@LINE+2]]:8: error: expected assembly-time absolute expression
+TEST13:
+  .fill TEST11 - TEST12+i, 4, 0x12345678
+

llvm/test/MC/AsmParser/directive_fill_2.s

+# RUN: llvm-mc -filetype=obj -defsym=I=6 -triple i686-unknown-unknown %s | llvm-objdump -triple i686-unknown-unknown -s - | FileCheck --check-prefix="CHECK" %s
+# RUN: not llvm-mc -filetype=obj -defsym=I=4 -triple i686-unknown-unknown %s -o /dev/null 2>&1 | FileCheck --check-prefix="CHECK-ERR" %s
+
+
+
+# CHECK: Contents of section .text
+# CHECK-NEXT: 0000 e9810000 00cc9090 90909090 90909090
+  
+# Make sure we emit in correct endianness.
+  
+# CHECK: Contents of section .data
+# CHECK-NEXT: 0000 78563412 78563412 78563412
+
+.text
+foo:
+jmp bar2
+# CHECK-ERR: [[@LINE+1]]:7: error: invalid number of bytes
+.fill ((I+foo) - .), 1, 0xcc
+bar:
+ .space 128, 0x90 
+bar2:
+.byte 0xff
+  
+# This fill length is not known at assembler time. 
+
+.if (I==6)
+  
+.data
+.long 0x12345678  
+.fill ((foo+8)-bar), 4, 0x12345678
+
+.endif