This is an archive of the discontinued LLVM Phabricator instance.

[CSSPGO][llvm-profgen] Filter out the instructions without location info for symbolizer
ClosedPublic

Authored by wlei on Feb 10 2021, 10:16 AM.

Details

Summary

It appears some instructions doesn't have the debug location info and the symbolizer will return an empty call stack for them which will cause some crash later in profile unwinding. Actually we do not record the sample info for them, so this change just filter out those instruction.

As those instruction would appears at the begin and end of the instruction list, without them we need to add the boundary check for IP advance and backward.

Also for pseudo probe based profile, we actually don't need the symbolized location info, so here just change to use an empty stack for it. This could save half of the binary loading time.

Diff Detail

Event Timeline

wlei created this revision.Feb 10 2021, 10:16 AM
wlei requested review of this revision.Feb 10 2021, 10:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 10:16 AM
wlei edited the summary of this revision. (Show Details)Feb 10 2021, 10:30 AM
wlei added reviewers: wmi, hoy, wenlei, davidxl.

symbolizer will return an empty call stack for them which will cause some crash later in profile unwinding.

Can you share more details on what which part had trouble dealing with such cases?

llvm/tools/llvm-profgen/PerfReader.cpp
60

Since all code addresses for the entire binary is record in a single vector, this HasNext check is only going to help for the first/last instruction of the entire binary.

Is the problem you're seeing very specific to first/last instruction of the entire binary? I thought instruction without location could happen anywhere..

llvm/tools/llvm-profgen/ProfiledBinary.cpp
431

What about still advancing the index before returning false? I.e. advancing IP past last instruction should leave IP in invalid state - this might help expose issues where return value from advance is not checked, otherwise we might be slightly accessing last instruction repeatedly. We could also set address to UINT_MAX for such case, so extra check may be avoided. Same for backward, but UINT_MIN for address.

llvm/tools/llvm-profgen/ProfiledBinary.h
182

nit: getNumInstructions or getNumCodeAddresses ?

hoy added inline comments.Feb 10 2021, 3:42 PM
llvm/tools/llvm-profgen/PerfReader.cpp
60

Yeah, empty inline context should not be uncommon. Is the case where IP.backward() resulted in an invalid IP that crashed the inlineContextEqual check?

wlei added a comment.Feb 10 2021, 4:41 PM

symbolizer will return an empty call stack for them which will cause some crash later in profile unwinding.

Can you share more details on what which part had trouble dealing with such cases?

Sure, it's the case with the inlineContextEqual(as we are discussing in the comments below), if the locationStack is empty, the equal function will crash. After I filter out all the instruction without location, then I got the out-of-boundary crash. so that's why I also need to fix advance and backward.

In our internal prototype, it gives all the instruction without location info a fake location, like <function_name, -1>, -1 is the location line number.
here for profgen's symbolizer, it only gives an empty stack.

llvm/tools/llvm-profgen/PerfReader.cpp
60

this HasNext check is only going to help for the first/last instruction of the entire binary.
IP.backward() resulted in an invalid IP that crashed the inlineContextEqual check

Exactly, you both are right.

Is the problem you're seeing very specific to first/last instruction of the entire binary? I thought instruction without location could happen anywhere..

Yes, it could happen anywhere. if the instruction without location is not the first/ last instruction, it will jump to the next valid address which also get false for inlineContextEqual, it's fine for that. Only the first/last one, it will be out of boundary.

For our internal tool, it use a fake location, like <function_name, -1> which can ensure inlineContextEqual always return false. and I also saw the first/last instruction is always a fake location, so there is no out of boundary issue for it.

An alternative to fix this is to do the same as our internal one, for all the empty location stack, we create a fake location, say <invalid, -1>, then I guess we don't need to check the boundary for advance and backward (theoretically we should also check but we didn't see the case first/last insn isn't a fake location for our internal tool). One trade-off is it could sightly increase the memory usage.

Any thoughts on this?

llvm/tools/llvm-profgen/ProfiledBinary.cpp
431

Good suggestion! met the repeating instruction issue..

Will make the out of boundary address to UINT_MIN/UINT_MAX.

llvm/tools/llvm-profgen/ProfiledBinary.h
182

Good catch!

hoy added inline comments.Feb 10 2021, 5:20 PM
llvm/tools/llvm-profgen/PerfReader.cpp
60

Thanks for the explanation. Can this out-of-range issue be fixed by making inlineContextEqual always return false for empty stacks? Tweaking inlineContextEqual so that no special care to out-of-bound IP sound cleaner to me.

wlei updated this revision to Diff 323129.Feb 11 2021, 1:27 PM

address Hongtao's feedback

wlei added inline comments.Feb 11 2021, 2:06 PM
llvm/tools/llvm-profgen/PerfReader.cpp
60

Good point! I forgot it also needs to change getInlineLeafFrameLoc and getExpandedContextStr for the empty location stack check.

Submitted the latest diff for the tweaking, ran them on the SPEC, it all passed!

hoy added inline comments.Feb 11 2021, 4:14 PM
llvm/tools/llvm-profgen/PerfReader.cpp
60

Thanks for the refactoring. Looks a lot cleaner now.

95

Can we not return null here and instead just check if the return value has empty string below?

llvm/tools/llvm-profgen/ProfiledBinary.cpp
137

Add comment for this? An instruction without a valid debug line will be ignored by sample processing.

wlei added inline comments.Feb 11 2021, 4:58 PM
llvm/tools/llvm-profgen/PerfReader.cpp
95

This is a polymorphism function, for the callee the base class's not straightforward to get the Context string value. Or make a virtual function isEmpty for them?

llvm/tools/llvm-profgen/ProfiledBinary.cpp
137

Good catch!

hoy added inline comments.Feb 11 2021, 5:19 PM
llvm/tools/llvm-profgen/PerfReader.cpp
95

I see. It's fine to leave it as is. A virtual method adds additional cost. A null key can be used to identify bad line numbers.

wlei updated this revision to Diff 323360.Feb 12 2021, 9:25 AM

address Hongtao's feedback: added comments

Also fix a bug with total value, it should also accumulate the count even for a invalid debug line

wlei added inline comments.Feb 12 2021, 9:34 AM
llvm/test/tools/llvm-profgen/inline-cs-noprobe.test
4 ↗(On Diff #323360)

Note for reviewers, this is due to the total value computation change, fix bugs to use SUM of count instead of MAX.

wenlei added inline comments.Feb 12 2021, 9:55 AM
llvm/tools/llvm-profgen/ProfiledBinary.cpp
139

nit: return std::string()

llvm/tools/llvm-profgen/ProfiledBinary.h
232

do we still need NameOnly parameter?

wlei updated this revision to Diff 323390.Feb 12 2021, 10:32 AM

Address Wenlei's feedback and fix lint

hoy accepted this revision.Feb 12 2021, 10:41 AM
This revision is now accepted and ready to land.Feb 12 2021, 10:41 AM
wenlei accepted this revision.Feb 12 2021, 10:59 AM

thanks, lgtm.

This revision was landed with ongoing or failed builds.Feb 12 2021, 4:48 PM
This revision was automatically updated to reflect the committed changes.