Page MenuHomePhabricator

Fix PR44001: assert failure in getFunctionLocalOffsetAfterInsn
ClosedPublic

Authored by thopre on Thu, Nov 14, 11:33 PM.

Details

Summary

Assert in getFunctionLocalOffsetAfterInsn() fails when processing a call
MachineInstr inside a bundle and compiling with debug info. This is
because labels are added by DwarfDebug::beginInstruction() which is
called for each top-level MI by EmitFunctionBody()'s for-loop iteration
but constructCallSiteEntryDIEs() which calls
getFunctionLocalOffsetAfterInsn() iterates over all MIs.

This commit modifies constructCallSiteEntryDIEs() to get the associated
bundle MI for call MIs inside a bundle and use that to when calling
getFunctionLocalOffsetAfterInsn() and getLabelAfterInsn(). It also skips
loop iterations for bundle MIs since the loop statements are concerned
with debug info for each physical instructions and bundles represent a
group of instructions.

Event Timeline

thopre created this revision.Thu, Nov 14, 11:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 14, 11:33 PM

@thopre Thanks for the patch!

Can you please add another test case, by testing the Dwarf output produced by the AsmPrinter in this case (e.g. test the DW_TAG_call_sites and its attributes)?

llvm/test/DebugInfo/Hexagon/bundled-call-pr44001.ll
23 ↗(On Diff #229453)

Usually, we don't need the attributes.

32 ↗(On Diff #229453)

It is enough only producer: "clang version 10.0.0".

33 ↗(On Diff #229453)

Better directory: "/"?

43 ↗(On Diff #229453)

Same as above. (It is enough only producer: "clang version 10.0.0".)

thopre updated this revision to Diff 229454.Fri, Nov 15, 12:21 AM
thopre marked 4 inline comments as done.

Address comments about existing testcase.

@thopre Thanks for the patch!

Can you please add another test case, by testing the Dwarf output produced by the AsmPrinter in this case (e.g. test the DW_TAG_call_sites and its attributes)?

Will do, but will have to wait Monday I'm afraid.

vsk added a comment.Fri, Nov 15, 9:57 AM

This looks good to me, modulo prior reviewer comments. Thanks!

bjope added a subscriber: bjope.Fri, Nov 15, 12:15 PM
thopre updated this revision to Diff 229814.Mon, Nov 18, 5:19 AM

Check DWARF call site info in testcase

thopre marked an inline comment as done.Mon, Nov 18, 5:25 AM
thopre added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
771–779

BTW this seems wrong to me, it should probably be getLabelBeforeInsn(MI) but would need labels to be added before call MIs inside a bundle rather than after bundle MIs with a call MI. On the testcase below it gives a DW_AT_low_pc of 0x8 for the following code:

Disassembly of section .text:

00000000 foo:

0:       00 40 00 5a     5a004000 {      call 0x0
4:       00 c0 9d a0     a09dc000        allocframe(#0) } 
8:       40 3f 00 51     51003f40 {      r0 = add(r0,#1);        dealloc_return }

This seems good!

As @djtodoro mentions, a test case which verifies the DWARF output would be good.

Downstream we have had some unrelated issues with requesting labels for bundled MIs, which we solved with making the requestLabel*() and getLabel*() functions do getBundleStart() for bundled instructions. We can probably look towards using the same approach as here for the issue(s) which caused us to add that bundle handling in the first case, and see if it's possible/valuable to upstream that. If so, the requestLabel*() and getLabel*() functions should probably have !isInsideBundle() assertions.

Thanks for the test, but do you mind creating a MIR test case? That will show more precisely what we are checking.

dstenb added inline comments.Mon, Nov 18, 5:35 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
771–779

It seems that the DW_AT_low_pc attribute is the return address in the GNU extension, so I think that's correct.

The call site entry has a DW_AT_low_pc attribute which is the return address after the call. This corresponds to the return address computed by CFI in the called function (6.4).

http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2010-August/002318.html

This seems good!

As @djtodoro mentions, a test case which verifies the DWARF output would be good.

Downstream we have had some unrelated issues with requesting labels for bundled MIs, which we solved with making the requestLabel*() and getLabel*() functions do getBundleStart() for bundled instructions. We can probably look towards using the same approach as here for the issue(s) which caused us to add that bundle handling in the first case, and see if it's possible/valuable to upstream that. If so, the requestLabel*() and getLabel*() functions should probably have !isInsideBundle() assertions.

I believe labels get inserted in other places as well which might add some labels on actual MIs rather than bundle MIs so I'm not sure assert in those function would be appropriate.

thopre updated this revision to Diff 229824.Mon, Nov 18, 5:57 AM

Make it a MIR testcase

thopre updated this revision to Diff 229825.Mon, Nov 18, 6:00 AM

Fix comment for PCAddr

Harbormaster completed remote builds in B41109: Diff 229825.
thopre marked 2 inline comments as done.Mon, Nov 18, 6:04 AM
thopre added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
771–779

Alright, fixed the comment accordingly then.

thopre marked an inline comment as done.Mon, Nov 18, 6:06 AM

Note about the testcase: I had to remove the CFI directives from the MIR testcase because it wouldn't parse it. i.e. -stop-after=patchable-function followed by -start-after=patchable-function would not work unless I edit the CFI directives out.

This looks good to me (with minor nit included).

llvm/test/DebugInfo/Hexagon/bundled-call-pr44001.mir
28 ↗(On Diff #229825)

You can delete this one, since it is not being used in the foo()function.

thopre updated this revision to Diff 229852.Mon, Nov 18, 8:09 AM

Simplify testcase further

thopre updated this revision to Diff 229937.Mon, Nov 18, 3:18 PM
  • Fix RUN line for llc to detect MIR
  • make CHECK for call return address more specific since MIR can safely be mapped to instructions

Sorry, but one more thing. The MIR test should go into llvm/test/DebugInfo/MIR/Hexagon.

thopre updated this revision to Diff 230010.Tue, Nov 19, 2:22 AM

Move testcase to llvm/test/DebugInfo/MIR/Hexagon

djtodoro accepted this revision.Tue, Nov 19, 2:37 AM

I am OK with the change (with the last nit included).

Please wait for someone else of the reviewers to confirm it as well. Thanks a lot for this!

llvm/test/DebugInfo/MIR/Hexagon/bundled-call-pr44001.mir
11

This should not contain your local path.

This revision is now accepted and ready to land.Tue, Nov 19, 2:37 AM
dstenb accepted this revision.Tue, Nov 19, 3:12 AM

LGTM. Thanks!

thopre updated this revision to Diff 230026.Tue, Nov 19, 3:23 AM

Remove local path from ModuleID field.

thopre closed this revision.Tue, Nov 19, 3:24 AM
thopre marked an inline comment as done.