Page MenuHomePhabricator

[ARM] Fix invalid symbol redefinition due to duplicated jumptable (PR42760)
ClosedPublic

Authored by nikic on Thu, Aug 1, 1:45 PM.

Details

Summary

Fix for https://bugs.llvm.org/show_bug.cgi?id=42760. A tBR_JTr instruction is duplicated by tail duplication, which results in the same jumptable with the same label being emitted twice.

Fix this by marking tBR_JTr as not duplicable. The corresponding ARM instructions (BR_JTr, BR_JTm_i12, BR_JTm_rs and BR_JTadd) are already marked as not duplicable.

Not sure whether the test-case adds any value, it's a fairly ugly bugpoint reduction of the original issue.

Diff Detail

Repository
rL LLVM

Event Timeline

nikic created this revision.Thu, Aug 1, 1:45 PM

Maybe you can make a simpler MIR testcase? (http://llvm.org/docs/MIRLangRef.html)

It's probably feasible to teach constant islands and the asmprinter to handle jump tables with multiple uses correctly, but if we've disabled it for ARM/Thumb2 I'm fine doing the same for Thumb1.

nikic updated this revision to Diff 213106.Fri, Aug 2, 11:58 AM

Simplify testcase.

nikic updated this revision to Diff 213109.Fri, Aug 2, 12:12 PM

Also mark tTBB_JT and tTBH_JT as not duplicable.

nikic added a comment.Fri, Aug 2, 12:27 PM

I've reduced the test case to a more reasonable size now...

It's probably feasible to teach constant islands and the asmprinter to handle jump tables with multiple uses correctly, but if we've disabled it for ARM/Thumb2 I'm fine doing the same for Thumb1.

I just checked over all the jumptable opcodes: All the ARM+Thumb2 were marked, and all the Thumb1 weren't. I've now added the isNotDuplicable annotation to tTBB_JT and tTBH_JT as well.

As to actually making the duplication work, I don't really have a handle on how complicated that would be. We should probably land a minimal fix first in any case, because this also needs to go into the LLVM9 branch.

efriedma added inline comments.Fri, Aug 2, 2:30 PM
llvm/lib/Target/ARM/ARMInstrThumb.td
1469 ↗(On Diff #213109)

I can't imagine this actually matters, given we only form tbb/tbh in constant islands, but fine.

llvm/test/CodeGen/Thumb/pr42760.ll
2 ↗(On Diff #213109)

The goal is that it doesn't crash, yes, but CHECK lines are also important to ensure that you're actually triggering the codepath you think you're triggering, if the compiler changes in the future. In particular, that you're generating a jump table, and it would be feasible to tail-duplicate the jump table branch if it wasn't marked noduplicate).

nikic updated this revision to Diff 213131.Fri, Aug 2, 2:38 PM

Generate check lines.

efriedma accepted this revision.Fri, Aug 2, 2:41 PM

LGTM

This revision is now accepted and ready to land.Fri, Aug 2, 2:41 PM
nikic marked 3 inline comments as done.Fri, Aug 2, 2:42 PM
nikic added inline comments.
llvm/lib/Target/ARM/ARMInstrThumb.td
1469 ↗(On Diff #213109)

I've added this for consistency with the t2TBB_JT/t2TBH_JT opcodes. Happy to drop if not needed.

llvm/test/CodeGen/Thumb/pr42760.ll
2 ↗(On Diff #213109)

I've added the CHECK lines now.

This revision was automatically updated to reflect the committed changes.
nikic marked an inline comment as done.