This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profgen] Switch to DWARF-based symbol and ranges
ClosedPublic

Authored by wlei on Oct 21 2021, 4:35 PM.

Details

Summary

It happened a bug that some callsite name in the profile is not a real function, it turned out that there're some non-function symbol from the ELF text section, e.g. the global accessible branch label and also recalled that we can have one function being split into multiple ranges. We shouldn't count samples for those are not the entry of the real function.

So this change tried to fix this issue by switching to use the name or ranges from DWARF-based debug info, the range of which assure it's the real function start. For the split functions, we assume that the real entry function's DWARF name should always match the symbol table name.

The switching is also consistent with the body samples' symbol which is from DWARF.

Diff Detail

Event Timeline

wlei created this revision.Oct 21 2021, 4:35 PM
wlei requested review of this revision.Oct 21 2021, 4:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2021, 4:35 PM
wlei updated this revision to Diff 381432.Oct 21 2021, 4:49 PM

Updating D112282: [llvm-profgen] Switch to DWARF-based symbol and ranges

wlei edited the summary of this revision. (Show Details)Oct 21 2021, 5:03 PM
wlei added reviewers: hoy, wenlei.
wlei updated this revision to Diff 381440.Oct 21 2021, 5:17 PM
wlei edited the summary of this revision. (Show Details)Oct 21 2021, 5:17 PM

tweak the comments

hoy added a comment.Oct 24 2021, 11:14 PM

This work should improve the robustness of symbol correlation for cases where an original source function is implemented by multiple labeled binary code slices. Thanks for working on this.

llvm/tools/llvm-profgen/ProfileGenerator.cpp
349

typo: leverange

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

Is HighPC inclusive or not?

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

nit: getSymbolForStartOffset -> getDwarfSymbolForStartOffset ? Similar to findSymbolForOffset below.

355

assert Offset falls into the range started by the start address?

wlei added inline comments.Oct 25 2021, 10:57 AM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
349

Good catch, thanks!

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

it's inclusive, it's the same as the previous EndOffset in symbol table.

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

Fixed.

355

The upper_bound will assure Offset >= StartOffset.

Do you mean to assert (Offset <= EndOffset & "..."); ? This sounds a good place, with this, we don't need https://reviews.llvm.org/D111902 then.

wlei updated this revision to Diff 382057.Oct 25 2021, 11:01 AM

Updating D112282: [llvm-profgen] Switch to DWARF-based symbol and ranges

wlei updated this revision to Diff 382076.Oct 25 2021, 11:48 AM

check the range fall within [StartOffset, EndOffset]

wlei added inline comments.Oct 25 2021, 11:53 AM
llvm/tools/llvm-profgen/ProfiledBinary.h
355

I found that there're some lib code in the bottom of the section which will cause the assertion failure.

So I added the code below to filter out them.

if (Offset > I->second.EndOffset)
  return nullptr;

With this, it should guarantee the offset fall into the right range.

hoy added inline comments.Oct 25 2021, 2:55 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
496

From the way ranges of a dwarf symbol are populated, EndOffset should not be a part of a range (exclusive). But computeInlinedContextSizeForRange expects the opposite.

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

why remove the equal compare? A label at the end of the text section has StartOffset == NextStartOffset.

570

I see. It actually means exclusive which means HighPC does not belong to the current function :). Otherwise FunctionSize should be Range.HighPC - FunctionStart + 1.

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

nit: different parts of a function

wlei added inline comments.Oct 25 2021, 8:54 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
496

Good point, I just changed EndOffset inclusive range.

uint64_t EndOffset = Range.HighPC - getPreferredBaseAddress() - 1;
llvm/tools/llvm-profgen/ProfiledBinary.cpp
336

this is because I saw some symbols has the same StartOffset but different name and mismatch its dwarf info. for example:

[0x01] main
[0x01] foo
[0x02] move ...       main:1

both main and foo's startoffset is [0x01], previously we will skip main, but in a rare case, the body of the function is actually from main not foo( main's the dwarf range is not zero), later we need to set main as the entry of the function. So here I tried to cover this case,the only cost is we process is the zero-size symbol for setting the entry of function.

570

I see, you're right. it's also not the next start offset. I saw in spec2017, the HighPC of main is 2058d7

<main>:
  ...
  2058cf: leal  (%rdx,%rdx), %eax                 cost_compare:11
  2058d2: addl  $-1, %eax                         cost_compare:11
  2058d5: popq  %rbp
  2058d6: retq
  2058d7: int3.    # HighPC
  2058d8: int3
  2058d9: int3
  2058da: int3
  2058db: int3
  2058dc: int3
  2058dd: int3
  2058de: int3
  2058df: int3

<spec_qsort>:
  2058e0: pushq %rbp                             spec_qsort:1
  2058e1: movq  %rsp, %rbp                        spec_qsort:1
  2058e4: pushq %r15
llvm/tools/llvm-profgen/ProfiledBinary.h
181

fixed, thanks!

wlei updated this revision to Diff 382176.Oct 25 2021, 8:55 PM

addressing Hongtao's feedback

dblaikie added inline comments.Oct 25 2021, 8:59 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
496

(it might be best to use an exclusive end, like DWARF does - for consistency and to enable representing zero-length ranges (which do come up at the moment in LLVM's codegen - zero length functions can produce situations where lowpc == highpc, so if you use - 1 you might end up with a weird situation where highpc < lowpc which could be problematic)

wlei updated this revision to Diff 382185.Oct 25 2021, 9:25 PM

use an exclusive end of the range, be consistent to DWARF

llvm/tools/llvm-profgen/ProfileGenerator.cpp
496

I see, -1 is indeed a weird case for this. Thanks for the suggestion!

wenlei added inline comments.Oct 25 2021, 10:38 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
365–366

Just double check, the warnings are not being removed, but moved and covered by a separate patch, right?

Not a big deal, but it would be good for review if you can either 1) still keep it for review, 2) make a stack on top of the other patch so the warning doesn't show up on left side at all..

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

nit:

if (Symbol->SymbolName == ELFSymbolName)
  Symbol->isEntryFunction = true;
llvm/tools/llvm-profgen/ProfiledBinary.h
75

This is a bit confusing - the name DWARFSymbol indicates this is 1:1 mapping to actual dwarf symbol, however for dwarf there's one symbol for one function, instead of one symbol for one range.

77

EntryFunction is a confusing name - usually that means entry point of a program. I guess what you meant here FunctionEntry?

78

If this is indeed dwarf symbol as the struct name suggests, all the ranges of a function would share the same name, and we'd be duplicating std::string in that case. If this is actual ELF symbol name, they'll be different, then we need to name it explicitly as ElfSymbolName.

82

As struct representation for a symbol, having EndOffset only but not start offset seem weird. I think we either "complete" it as a general symbol definition, or rename it to make it explicit that this is just some special purpose struct to specific bookkeeping map.

Also please comment if the end offset is inclusive or exclusive.

wlei updated this revision to Diff 382503.Oct 26 2021, 6:53 PM

Updating D112282: [llvm-profgen] Switch to DWARF-based symbol and ranges

llvm/tools/llvm-profgen/ProfileGenerator.cpp
365–366

Good point. I actually intend to remove the warning here because I found some code in plt section which don't match any DWARF symbol will trigger this warning and it's a false positive. Added some comments to this.

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

This is because we can have multiple offset query the same function. String cmp is heavy, so skip if isEntryFunction is already set.

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

I see, understand it's not a clear name. It's not a function in dwarf, it's more like the range in ELF but it's not only the range and has other info. One function can have multiple ranges, I changed to ElfRangeInfo meaning a mixed info of the elf range. see if this looks good to you?

77

FunctionEntry sounds good

78

this is the Dwarf-based name, renamed and added the code to deduplicate the std::string

82

I added some comments to clarify this, i, e. this struct is always accessed by StartOffset2ElfRangeInfoMap whose key is the start offset so just skip it here. See if this's clearer, I'm also fine to add the "StartOffset" for good readability.

hoy added inline comments.Oct 27 2021, 12:05 PM
llvm/tools/llvm-profgen/ProfiledBinary.cpp
584

nit: Symbol -> RangeInfo

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

We are using both elf ranges and dwarf ranges as the terms. Should we stick to one, say DwarfRangeInfo and DWARFRangesVectorTy or the other way around?

341–345

nit: getElfRangeInfoForStartOffset -> findElfRangeInfoForStartOffset to be consistent with the function naming below.

wenlei added inline comments.Oct 27 2021, 5:48 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
365–366

because I found some code in plt section which don't match any DWARF symbol

But we're looking for elf symbol here, not dwarf symbol?

Regardless I'm hoping that we still have some sanity check for profiles so users would get a hint when they use mismatched profile and binary.

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

I would suggest we use FuncRange instead of tie this with dwarf/elf in the name. We don't support windows today, but nothing prevents us from supporting windows (PE/PDB) in the future.

Accordingly, we can have something like FuncSymName, RangeSymName. And for IsFunctionEntry, the two names would be the same. What do you think?

wlei updated this revision to Diff 382904.Oct 27 2021, 9:19 PM

Rename to FuncRange and other refactorings

wlei added inline comments.Oct 27 2021, 9:19 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
365–366

But we're looking for elf symbol here, not dwarf symbol?

I think the name is misleading, here it's not the elf symbol. Sorry for the confusing. Changed to FuncRange as you suggested.

Regardless I'm hoping that we still have some sanity check for profiles so users would get a hint when they use mismatched profile and binary.

Yeah, I will pick up https://reviews.llvm.org/D111902 for the sanity check.

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

fixed, thanks!

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

Thanks for the suggestions. FuncRange is a better name, renamed all corresponding variable names.

We are using both elf ranges and dwarf ranges as the terms. Should we stick to one, say DwarfRangeInfo and DWARFRangesVectorTy or the other way around?

Changed to FuncRange or RangesTy

341–345

Fixed

hoy accepted this revision.Oct 27 2021, 11:04 PM

LGTM.

llvm/tools/llvm-profgen/ProfileGenerator.cpp
365–366

Perhaps the sanity check can be done during the parsing of LBR traces so that errors will not be propagated too far.

This revision is now accepted and ready to land.Oct 27 2021, 11:04 PM
wenlei added inline comments.Oct 27 2021, 11:28 PM
llvm/tools/llvm-profgen/ProfiledBinary.h
87

nit: IsFunctionEntry->isFuncEntry to be consistent.

182

Instead of using index to access all ranges of a function, how about using symbol name directly? we could have a map (function name -> function object, and function object contains a list of ranges).

Just trying to make the scattered data a bit more structured. Seems it would be cleaner if we have a proper abstraction for Function, which contains name and list of ranges. What do you think?

If this can lead to bigger change, we can also deal with it later in a separate patch.

184

Would rellocate invalidate the StringRef holding the underlying strings?

362

nit: getAllRangesOfOneFunc -> getAllRangesForFunc

wlei updated this revision to Diff 383139.Oct 28 2021, 1:15 PM

some refactorings according to reviewer's suggestions

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

Fixed,thanks!

182

This is nice abstraction, not a big change, changed.

one thing is the Function is conflict with llvm::Function, then I changed to ProfFunction, see if this looks good?

184

Seems we all use this way to persist data for a reference type, like the context
std::unordered_set<SampleContextFrameVector, SampleContextFrameHash> Contexts;. I guess this should work.

wlei updated this revision to Diff 383143.Oct 28 2021, 1:25 PM

fix typo in the comments

hoy added inline comments.Oct 28 2021, 2:00 PM
llvm/tools/llvm-profgen/ProfiledBinary.h
184

std::unordered_map is not a volatile container, unlike std::vector. It's more like std::list.

wenlei accepted this revision.Oct 28 2021, 6:33 PM

lgtm, except two nits. thanks..

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

Now that we have a proper Function abstraction, I think it's better to have the start offset field for the ranges, so the structure is more complete.

182

Have a slight preference towards BinaryFunction (given it reflects the binary representation of the function, in terms of address ranges). Not a big deal though.

wlei updated this revision to Diff 383402.Oct 29 2021, 9:47 AM

Addressing Wenlei's feedback.

This revision was landed with ongoing or failed builds.Oct 29 2021, 9:59 AM
This revision was automatically updated to reflect the committed changes.