This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by phosek on Jun 8 2015, 3:48 PM.

Details

Summary

The current implementation doesn't always flush all pending labels before emitting
data which can result in an incorrectly placed labels in case when when instruction
bundling is enabled and -mc-relax-all flag is being used. To address this issue,
we always flush pending labels before emitting data.

The change was tested by running PNaCl toolchain trybots with
-mc-relax-all flag set.

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

Diff Detail

Event Timeline

phosek updated this revision to Diff 27343.Jun 8 2015, 3:48 PM
phosek retitled this revision from to [MC] Proper handle labels when -mc-relax-all flag is used.
phosek updated this object.
phosek edited the test plan for this revision. (Show Details)
phosek added a reviewer: eliben.
phosek added a subscriber: mseaborn.

Can you add a test for this, please?

phosek edited reviewers, added: mseaborn; removed: eliben.Jun 8 2015, 4:19 PM
phosek set the repository for this revision to rL LLVM.
phosek edited subscribers, added: Unknown Object (MLST); removed: mseaborn.
phosek updated this revision to Diff 27346.Jun 8 2015, 4:26 PM
  • Test added
mseaborn added inline comments.Jun 10 2015, 2:32 PM
lib/MC/MCELFStreamer.cpp
77 ↗(On Diff #27346)

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. :-( )

test/MC/X86/AlignedBundling/rodata-section.s
3

Nit: Indent "llvm-objdump", because this is a continuation line (it follows a "\").

Same below.

7

Nit: Is this redundant if you're also using "-triple=i686-nacl"? (Or maybe it's only redundant in the pnacl-llvm branch?)

13

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.

24

Nit: Can you make the alignment indentation consistent?

mseaborn added inline comments.Jun 10 2015, 11:21 PM
lib/MC/MCELFStreamer.cpp
77 ↗(On Diff #27346)

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.

phosek added inline comments.Jun 10 2015, 11:33 PM
lib/MC/MCELFStreamer.cpp
77 ↗(On Diff #27346)

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.

phosek retitled this revision from [MC] Proper handle labels when -mc-relax-all flag is used to [MC] Ensure that pending labels are flushed when -mc-relax-all flag is used.Jun 17 2015, 5:14 PM
phosek updated this object.
phosek updated this object.Jun 17 2015, 5:18 PM
phosek updated this revision to Diff 27902.Jun 17 2015, 5:19 PM
  • Test added
  • Ensure that pending labels are always flushed
lib/MC/MCELFStreamer.cpp
77 ↗(On Diff #27346)

I have updated the implementation which now ensures that pending labels are always properly flushed before emitting any instructions.

test/MC/X86/AlignedBundling/rodata-section.s
3

Done.

7

It's only redundant in pnacl-llvm branch.

13

Done.

24

It should be consistent with other tests.

mseaborn accepted this revision.Jun 20 2015, 4:26 PM
mseaborn edited edge metadata.

In the commit message, can you add "when instruction bundling is enabled and..." before "when -mc-relax-all flag is being used", to clarify, please? (And to match your other change, http://reviews.llvm.org/D10044.)

LGTM, thanks.

This revision is now accepted and ready to land.Jun 20 2015, 4:26 PM
phosek updated this object.Jun 26 2015, 6:03 PM
phosek edited edge metadata.
phosek updated this object.
This revision was automatically updated to reflect the committed changes.