Page MenuHomePhabricator

Test lldb backtraces with machine function splitter
ClosedPublic

Authored by tmsriram on Oct 23 2020, 4:36 PM.

Details

Summary

clang supports option -fsplit-machine-functions and this test checks if the backtraces are sane when functions are split

With -fsplit-machine-functions, a function with profiles can get split into 2 parts, the original function containing hot code and a cold part as determined by the profile info and the cold cutoff threshold.. The cold part gets the ".cold" suffix to disambiguate its symbol from the hot part and can be placed arbitrarily in the address space.

This test checks if the back-trace looks correct when the cold part is executed.

Diff Detail

Event Timeline

tmsriram created this revision.Oct 23 2020, 4:36 PM
tmsriram requested review of this revision.Oct 23 2020, 4:36 PM
tmsriram edited the summary of this revision. (Show Details)Oct 23 2020, 5:48 PM
snehasish added inline comments.Oct 23 2020, 7:00 PM
lldb/test/Shell/Unwind/split-machine-functions.test
2

typo "produces"

17

Do we need a symbol ordering file here? Without it the split part should go to .text.split and the other functions would go to .text effectively creating the gap which you want here.

tmsriram added inline comments.Oct 23 2020, 10:56 PM
lldb/test/Shell/Unwind/split-machine-functions.test
17

In the final binary, in the default case, .text (after merging the individual .text sections) and .text.split are put next to each other. If foo is the last function in .text, foo will be placed next to foo.cold. In this case, I do see main between foo and foo.cold so there is a gap in the default case but the function ordering from the compiler is arbitrary and is not necessarily the same as the source order. Since MFS does not rely on any symbol ordering, I can just delete it but just want to mention that the gap is not guaranteed just because they are in separate sections.

snehasish added inline comments.Oct 24 2020, 10:44 AM
lldb/test/Shell/Unwind/Inputs/split-machine-functions.ll
3

Is it useful to include the source here? The generated IR is fairly straightforward and could just be edited by hand if necessary.

64

Instead of having a (auto generated) comment with the attrs would making these actual attributes make the test more robust?
(here and above)

66–67

These two lines can be removed I think? I guess they are here since you used -O0 to generate the bitcode.

lldb/test/Shell/Unwind/split-machine-functions.test
17

SGTM, can you add a comment here saying that a symbol ordering file is used to ensure that there is at least one symbol between foo and foo.cold since ordering is arbitrary?

Also would it make things cleaner if you used split-file (https://reviews.llvm.org/rGbcea3a7a288) for the symbol ordering parts instead of "RUN: echo ..." for the ordering file?

tmsriram updated this revision to Diff 300712.Oct 26 2020, 10:06 AM
tmsriram marked 5 inline comments as done.

Address the following reviewer comments:

  • Use split-file for sym order
  • Cleanup the .ll file further
  • Typo fixes.
lldb/test/Shell/Unwind/Inputs/split-machine-functions.ll
3

I was told in prior tests to include a reproducible if it exists. I agree this is not complicated .ll but it doesn't hurt? I am fine either way.

64

I intentionally removed the metadata for the attrs as the principle has been to to keep the tests trim. Removing these attrs too. Alright?

lldb/test/Shell/Unwind/split-machine-functions.test
17

Nice suggestion. I could use split-file for the ir too but since we already have an Inputs directory keeping it separate makes the file look less cluttered. Alright?

snehasish accepted this revision.Oct 26 2020, 10:25 AM

lgtm, thanks for adding the test.

lldb/test/Shell/Unwind/Inputs/split-machine-functions.ll
3

Sure, lets keep if thats the convention here.

This revision is now accepted and ready to land.Oct 26 2020, 10:25 AM