[PATCH] [MC] Ensure that pending labels are flushed when -mc-relax-all flag is used

[Petr Hosek]

================
Comment at: lib/MC/MCELFStreamer.cpp:77
@@ +76,3 @@
+      size_t Offset = DF->getContents().size();
+      for (const MCSymbol &Symbol : Assembler.symbols()) {
+        MCSymbolData &SD = Symbol.getData();
----------------
phosek wrote:
> mseaborn wrote:
> > mseaborn wrote:
> > > This logic looks OK.  However, doesn't this take O(n) time in the number of symbols that have been created so far?  I'm concerned that this could lead to compilation taking O(n^2) time in the size of the module.  (Quadratic time per function would be bad, but quadratic time per module would be even worse. :-( )
> > Here's a suggestion to avoid the O(n) time problem.
> > 
> > Background:
> > 
> >  * When PendingLabels was introduced by Derek (in http://llvm.org/viewvc/llvm-project?view=revision&revision=220439), it had the following invariant: If the current fragment is an MCDataFragment, PendingLabels should be empty.
> >  * Functions such as EmitBytes() assumed they can add to the current data fragment, if there is one, without calling flushPendingLabels() first.
> >  * Your change (r234714) created cases where the invariant didn't apply, which broke EmitBytes().  (If there had been an assertion to check the invariant, we would have noticed the problem more readily.)
> > 
> > How about we fix this assumption by EmitBytes() etc., by making sure they trigger a call to flushPendingLabels()?  Maybe we could have getOrCreateDataFragment() call flushPendingLabels().  We would then allow PendingLabels to be non-empty when the current fragment is an MCDataFragment.
> > 
> > That way, we don't have to adjust any labels after they've already been assigned to an offset within a fragment.
> I agree, that's the solution I'm currently considering i.e. calling `flushPendingLabels()` from `getOrCreateDataFragment()`. I still need to run the change through bots to make sure that everything it doesn't break any other invariants.
I have updated the implementation which now ensures that pending labels are always properly flushed before emitting any instructions.

================
Comment at: test/MC/X86/AlignedBundling/rodata-section.s:2
@@ +1,3 @@
+# RUN: llvm-mc -triple=i686-nacl -filetype=obj %s -o - | \
+# RUN: llvm-objdump -disassemble -no-show-raw-insn - | FileCheck %s
+# RUN: llvm-mc -triple=i686-nacl -filetype=obj -mc-relax-all %s -o - | \
----------------
mseaborn wrote:
> Nit: Indent "llvm-objdump", because this is a continuation line (it follows a "\").
> 
> Same below.
Done.

================
Comment at: test/MC/X86/AlignedBundling/rodata-section.s:6
@@ +5,3 @@
+
+        .bundle_align_mode 5
+        .text
----------------
mseaborn wrote:
> Nit: Is this redundant if you're also using "-triple=i686-nacl"?  (Or maybe it's only redundant in the pnacl-llvm branch?)
It's only redundant in pnacl-llvm branch.

================
Comment at: test/MC/X86/AlignedBundling/rodata-section.s:12
@@ +11,3 @@
+foo:
+        subl	$12, %esp
+# CHECK: 3: movl $14, 8(%esp)
----------------
mseaborn wrote:
> Nit: It would make the test clearer if you omitted parts that aren't necessary for reproducing the bug, i.e.
>  * "foo" label
>  * prologue and epilogue
>  * call to "bar"
> etc.
Done.

================
Comment at: test/MC/X86/AlignedBundling/rodata-section.s:23
@@ +22,3 @@
+        popl	%ecx
+        jmp	  *%ecx
+
----------------
mseaborn wrote:
> Nit: Can you make the alignment indentation consistent?
It should be consistent with other tests.

http://reviews.llvm.org/D10325

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/