[MC] Attach labels to existing fragments instead of using a separate fragment

[dschuff]

Summary

Currently when emitting a label, a new data fragment is created for it if the
current fragment isn't a data fragment.
This change instead enqueues the label and attaches it to the next fragment
(e.g. created for the next instruction) if possible.

When bundle alignment is not enabled, this has no functionality change (it
just results in fewer extra fragments being created). For bundle alignment,
previously labels would point to the beginning of the bundle padding instead
of the beginning of the emitted instruction. This was not only less efficient
(e.g. jumping to the nops instead of past them) but also led to miscalculation
of the address of the GOT (since MC uses a label difference rather than
emitting a "." symbol).

Fixes https://code.google.com/p/nativeclient/issues/detail?id=3982

[jvoung]

include/llvm/MC/MCObjectStreamer.h

50

The function seems like it could be big in terms of code size, so this should probably not be defined inline.

73

reset() should clear the PendingLabels vector too?

lib/MC/MCObjectStreamer.cpp

132

nit: Can the "it." fit on this first line so that the whole sentence is on one line?

test/MC/X86/AlignedBundling/labeloffset.s

18

nit: The indent of the real instructions are inconsistent.

• calll and popl are at one level
• movl and the bundle_lock are at a different level

28

Should this one be Ltmp1, to test Ltmp1 as well? Otherwise it's not used.

39

It's not clear to me what you mean here and perhaps I am missing context. The difference appears to be that in the first case "cmpl" is part of a bundle_lock/unlock group, but here it is not. I'm assuming that in the first case, instructions that are bundle_locked/unlocked simply assume the relaxed larger form, while in this second case, the instructions can sometimes use a small immediate?

60

Small suggestion: maybe this test could have bar in a separate section (as if you compiled with ffunction-sections), then .Ltmp2 and the "80: popl" can be relative to the beginning of the function and not be sensitive to the size of the earlier functions in case they change.

64

The code is careful to flush the pending labels if switching sections. Could you test for that too?

test/MC/X86/AlignedBundling/long-nop-pad.s

17

Why did this end up changing from CHECK-NEXT to CHECK?

[dschuff]

• Review comments from jvoung

include/llvm/MC/MCObjectStreamer.h

50

Done

73

Done

lib/MC/MCObjectStreamer.cpp

132

Done

test/MC/X86/AlignedBundling/labeloffset.s

18

Sorry, emacs wanted tabs and I wanted spaces :)
Fixed.

28

That was just to ensure that 2 adjacent labels didn't mess things up. I updated the test, see next comment.

39

Yeah sorry I simplified the test from the my original one and that part got unintentionally oversimplified :) see if it makes more sense now.

60

done.

64

Done.

test/MC/X86/AlignedBundling/long-nop-pad.s

17

The label foo: now points to 1b the call instead of 0 the nop. so it gets printed in between the last nop and the call.

[eliben]

LGTM with some nits

lib/MC/MCObjectStreamer.cpp

45

This comment probably belongs in the header?

135

Can merge the dyn cast into the if here?

[jvoung]

LGTM too

[dschuff]

• Review from eliben

lib/MC/MCObjectStreamer.cpp

45

Done

135

Done.

[dschuff]

Closed by commit rL220439 (authored by @dschuff).

include/llvm/MC/MCObjectStreamer.h

 //===- MCObjectStreamer.h - MCStreamer Object File Interface ----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_MC_MCOBJECTSTREAMER_H
 #define LLVM_MC_MCOBJECTSTREAMER_H
 
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCStreamer.h"
 
 namespace llvm {
 class MCAssembler;
 class MCCodeEmitter;
 class MCSectionData;
 class MCSubtargetInfo;

 /// to that file format or custom semantics expected by the object writer
 /// implementation.
 class MCObjectStreamer : public MCStreamer {
   MCAssembler *Assembler;
   MCSectionData *CurSectionData;
   MCSectionData::iterator CurInsertionPoint;
   bool EmitEHFrame;
   bool EmitDebugFrame;
+  SmallVector<MCSymbolData *, 2> PendingLabels;
 
   virtual void EmitInstToData(const MCInst &Inst, const MCSubtargetInfo&) = 0;
   void EmitCFIStartProcImpl(MCDwarfFrameInfo &Frame) override;
   void EmitCFIEndProcImpl(MCDwarfFrameInfo &Frame) override;
 
+  // If any labels have been emitted but not assigned fragments, ensure that
+  // they get assigned, either to F if possible or to a new data fragment.
+  void flushPendingLabels(MCFragment *F) {
+    if (PendingLabels.size()) {
+      if (!F) {
+        F = new MCDataFragment();
+        CurSectionData->getFragmentList().insert(CurInsertionPoint, F);
+        F->setParent(CurSectionData);
+      }
+      for (MCSymbolData *SD : PendingLabels) {
+        SD->setFragment(F);
+        SD->setOffset(0);
+      }
+      PendingLabels.clear();
+    }
+  }
+
 protected:
   MCObjectStreamer(MCContext &Context, MCAsmBackend &TAB, raw_ostream &_OS,
                    MCCodeEmitter *_Emitter);
   MCObjectStreamer(MCContext &Context, MCAsmBackend &TAB, raw_ostream &_OS,
                    MCCodeEmitter *_Emitter, MCAssembler *_Assembler);
   ~MCObjectStreamer();
 
 public:
   /// state management
   void reset() override;
 
   /// Object streamers require the integrated assembler.
   bool isIntegratedAssemblerRequired() const override { return true; }
 
   MCSymbolData &getOrCreateSymbolData(const MCSymbol *Symbol) {
     return getAssembler().getOrCreateSymbolData(*Symbol);
   }
   void EmitFrames(MCAsmBackend *MAB);
   void EmitCFISections(bool EH, bool Debug) override;
 
 protected:
   MCSectionData *getCurrentSectionData() const {
     return CurSectionData;
   }
 
   MCFragment *getCurrentFragment() const;
 
-  void insert(MCFragment *F) const {
+  void insert(MCFragment *F) {
+    flushPendingLabels(F);
     CurSectionData->getFragmentList().insert(CurInsertionPoint, F);
     F->setParent(CurSectionData);
   }
 
   /// Get a data fragment to write into, creating a new one if the current
   /// fragment is not a data fragment.
-  MCDataFragment *getOrCreateDataFragment() const;
+  MCDataFragment *getOrCreateDataFragment();
 
 public:
   void visitUsedSymbol(const MCSymbol &Sym) override;
 
   MCAssembler &getAssembler() { return *Assembler; }
 
   /// @name MCStreamer Interface
   /// @{

lib/MC/MCObjectStreamer.cpp

 
 MCObjectStreamer::~MCObjectStreamer() {
   delete &Assembler->getBackend();
   delete &Assembler->getEmitter();
   delete &Assembler->getWriter();
   delete Assembler;
 }
 
 void MCObjectStreamer::reset() {
   if (Assembler)
     Assembler->reset();
   CurSectionData = nullptr;
   CurInsertionPoint = MCSectionData::iterator();
   EmitEHFrame = true;
   EmitDebugFrame = false;
   MCStreamer::reset();
 }
MCFragment *MCObjectStreamer::getCurrentFragment() const {
   assert(getCurrentSectionData() && "No current section!");
 
   if (CurInsertionPoint != getCurrentSectionData()->getFragmentList().begin())
     return std::prev(CurInsertionPoint);
 
   return nullptr;
 }
 
-MCDataFragment *MCObjectStreamer::getOrCreateDataFragment() const {
+MCDataFragment *MCObjectStreamer::getOrCreateDataFragment() {
   MCDataFragment *F = dyn_cast_or_null<MCDataFragment>(getCurrentFragment());
   // When bundling is enabled, we don't want to add data to a fragment that
   // already has instructions (see MCELFStreamer::EmitInstToData for details)
   if (!F || (Assembler->isBundlingEnabled() && F->hasInstructions())) {
     F = new MCDataFragment();
     insert(F);
   }
   return F;
void MCObjectStreamer::EmitCFIEndProcImpl(MCDwarfFrameInfo &Frame) {
   Frame.End = getContext().CreateTempSymbol();
   EmitLabel(Frame.End);
 }
 
 void MCObjectStreamer::EmitLabel(MCSymbol *Symbol) {
   MCStreamer::EmitLabel(Symbol);
 
   MCSymbolData &SD = getAssembler().getOrCreateSymbolData(*Symbol);
-
-  // FIXME: This is wasteful, we don't necessarily need to create a data
-  // fragment. Instead, we should mark the symbol as pointing into the data
-  // fragment if it exists, otherwise we should just queue the label and set its
-  // fragment pointer when we emit the next fragment.
-  MCDataFragment *F = getOrCreateDataFragment();
   assert(!SD.getFragment() && "Unexpected fragment on symbol data!");
+
+  // If there is a current fragment, mark the symbol as pointing into
+  // it. Otherwise queue the label and set its fragment pointer when we emit
+  // the next fragment.
+  MCDataFragment *F = dyn_cast_or_null<MCDataFragment>(getCurrentFragment());
+  if (F) {
     SD.setFragment(F);
     SD.setOffset(F->getContents().size());
+  } else {
+    PendingLabels.push_back(&SD);
+  }
 }
 
 void MCObjectStreamer::EmitULEB128Value(const MCExpr *Value) {
   int64_t IntValue;
   if (Value->EvaluateAsAbsolute(IntValue, getAssembler())) {
     EmitULEB128IntValue(IntValue);
     return;
   }

 void MCObjectStreamer::EmitWeakReference(MCSymbol *Alias,
                                          const MCSymbol *Symbol) {
   report_fatal_error("This file format doesn't support weak aliases.");
 }
 
 void MCObjectStreamer::ChangeSection(const MCSection *Section,
                                      const MCExpr *Subsection) {
   assert(Section && "Cannot switch to a null section!");
+  flushPendingLabels(nullptr);
 
   CurSectionData = &getAssembler().getOrCreateSectionData(*Section);
 
   int64_t IntSubsection = 0;
   if (Subsection &&
       !Subsection->EvaluateAsAbsolute(IntSubsection, getAssembler()))
     report_fatal_error("Cannot evaluate subsection number");
   if (IntSubsection < 0 || IntSubsection > 8192)

 void MCObjectStreamer::FinishImpl() {
   // If we are generating dwarf for assembly source files dump out the sections.
   if (getContext().getGenDwarfForAssembly())
     MCGenDwarfInfo::Emit(this);
 
   // Dump out the dwarf file & directory tables and line tables.
   MCDwarfLineTable::Emit(this);
 
+  flushPendingLabels(nullptr);
   getAssembler().Finish();
 }

test/MC/X86/AlignedBundling/labeloffset.s

+# RUN: llvm-mc -triple=i686-nacl -filetype=obj %s -o - | \
+# RUN: llvm-objdump -disassemble -no-show-raw-insn -r - | FileCheck %s
+
+  .bundle_align_mode 5
+  .text
+  .globl  main
+  .align  32, 0x90
+  .type main,@function
+main:                                   # @main
+# Call + pop sequence for determining the PIC base.
+        .bundle_lock align_to_end
+  calll .L0$pb
+        .bundle_unlock
+.L0$pb:
+  popl  %eax
+# CHECK: 20: popl
+# 26 bytes of instructions between the pop and the use of the pic base symbol.
+        movl    $3, 2(%ebx, %ebx)
+        movl    $3, 2(%ebx, %ebx)
+        movl    $3, 2(%ebx, %ebx)
+        hlt
+        hlt
+# CHECK: nop
+        .bundle_lock
+.Ltmp0:
+.Ltmp1:
+        cmpl    %eax, .Ltmp0
+        cmpl    %eax, (.Ltmp0-.L0$pb)
+        .bundle_unlock
+# The cmpl has bundle padding to push it from 0x3b to 0x40.
+# The value of labels .Ltmp0 and .Ltmp1 should be 0x40 and not 0x3b
+# (which is where the padding starts)
+# CHECK: 40: cmpl %eax, 64
+# The difference between the labels should be 0x20 and not 0x1b
+# CHECK: 46: cmpl %eax, 32
+  popl  %ecx
+  jmp *%ecx
+
+# Also make sure it works with a non-relaxable instruction
+        .text
+        .globl  bar
+        .align  32, 0x90
+        .type   bar,@function
+bar:
+        .bundle_lock align_to_end
+        calll .L1$pb
+        .bundle_unlock
+.L1$pb:
+        popl %eax
+# CHECK: 80: popl
+# 26 bytes of instructions between the pop and the use of the pic base symbol.
+        movl    $3, 2(%ebx, %ebx)
+        movl    $3, 2(%ebx, %ebx)
+        movl    $3, 2(%ebx, %ebx)
+        hlt
+        hlt
+# CHECK: nop
+.Ltmp2:
+        cmpl    %eax, .Ltmp2
+# CHECK: cmpl %eax, 160
+        cmpl     %eax, (.Ltmp2-.L1$pb)
+# CHECK: cmpl %eax, 32
+        popl    %ecx
+        jmp *%ecx

test/MC/X86/AlignedBundling/long-nop-pad.s

 # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - \
 # RUN:   | llvm-objdump -disassemble -no-show-raw-insn - | FileCheck %s
 
 # Test that long nops are generated for padding where possible.
 
   .text
 foo:
   .bundle_align_mode 5
 
 # This callq instruction is 5 bytes long
   .bundle_lock align_to_end
   callq   bar
   .bundle_unlock
 # To align this group to a bundle end, we need a 15-byte NOP and a 12-byte NOP.
 # CHECK:        0:  nop
 # CHECK-NEXT:   f:  nop
-# CHECK-NEXT:   1b: callq
+# CHECK:   1b: callq
 
 # This push instruction is 1 byte long
   .bundle_lock align_to_end
   push %rax
   .bundle_unlock
 # To align this group to a bundle end, we need two 15-byte NOPs, and a 1-byte.
 # CHECK:        20:  nop
 # CHECK-NEXT:   2f:  nop
 # CHECK-NEXT:   3e:  nop
 # CHECK-NEXT:   3f: pushq