This is an archive of the discontinued LLVM Phabricator instance.

[memprof] Update the frame is inline logic and unittests.
ClosedPublic

Authored by snehasish on Mar 16 2022, 10:34 AM.

Details

Summary

Since DI frames are enumerated with the leaf function at index 0, this
patch fixes the logic when IsInlineFrame is set. Also update the
unittests to check that only the last frame is marked as non-inline from
a set of DI Frames for a PC address.

Diff Detail

Event Timeline

snehasish created this revision.Mar 16 2022, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 10:34 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
snehasish requested review of this revision.Mar 16 2022, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 10:34 AM
tejohnson accepted this revision.Mar 16 2022, 4:19 PM

lgtm

Unfortunately the ordering is not documented on the DIInliningInfo class. And confusingly, one piece of code sets an Inlined flag by the check "I > 0" (https://github.com/llvm/llvm-project/blob/49c048add4c980936fc2918838288ae2d795587d/llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp#L198).
However, if I look at how that flag is used, it is used to print "(inlined by)" *before* the current frame's function name, so it seems to apply to the I-1 frame, which actually means that all but the last frame are inlined - consistent with the change you have made here. Consider making a separate NFC change to add a comment to DIInliningInfo?

This revision is now accepted and ready to land.Mar 16 2022, 4:19 PM

lgtm

Unfortunately the ordering is not documented on the DIInliningInfo class. And confusingly, one piece of code sets an Inlined flag by the check "I > 0" (https://github.com/llvm/llvm-project/blob/49c048add4c980936fc2918838288ae2d795587d/llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp#L198).
However, if I look at how that flag is used, it is used to print "(inlined by)" *before* the current frame's function name, so it seems to apply to the I-1 frame, which actually means that all but the last frame are inlined - consistent with the change you have made here. Consider making a separate NFC change to add a comment to DIInliningInfo?

Sent https://reviews.llvm.org/D122033 to add a comment to getFrame(). Thanks for taking the time to examine existing usage to double check!

This revision was landed with ongoing or failed builds.Mar 21 2022, 10:42 AM
This revision was automatically updated to reflect the committed changes.