[llvm-profgen] Fix bug of setting function entry

[wlei]

Summary

Previously we set isFuncEntry flag to true when the funcName from DWARF is equal to the name in symbol table and we use this flag to ignore reporting callsite sample that's from an intra func branch. However, in HHVM, it appears that the symbol table name is inconsistent with the dwarf info func name, it's likely due to OptimizeGlobalAliases.

This change is a workaround in llvm-profgen side to mark the only one range as the function entry and add warnings for the remaining inconsistence.

This also fixed a missing getCanonicalFnName for symbol name which caused the mismatching as well.

[wenlei]

However, in HHVM, I found that there are many functions that none of their ranges is set to true

Why is this happening? Should we fix this so that for any function, there's always one range that is considered entry?

[wlei]

In D113492#3118908, @wenlei wrote:

However, in HHVM, I found that there are many functions that none of their ranges is set to true

Why is this happening? Should we fix this so that for any function, there's always one range that is considered entry?

For example, in HHVM:
The DWARF function name is "_ZN8facebook6falcon10ProcessKeyC2ERKS1_" and the same start address on ELF the symbol name is " _ZN8facebook6falcon10ProcessKeyC1ERKS1_". The function has only one range.

So there is name difference between DWARF and symbol table even it has only one range. I don't know the root reason for debug info, maybe some change in FE

Should we fix this so that for any function, there's always one range that is considered entry?

I'm thinking a case even we make the function entry flag correct, there's still a chance to call a non-entry function that should report the count, like "main" --> "foo.V1" , maybe foo.V1 is the function slice covering function start.

See the change in this patch, try to remove the isFuncEntry logic. Instead, we check if the source and start' range is from the same function but different split range.

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

419 ↗

(On Diff #385845)

Can this filter out non-entry jumps that is from a different function to an outlined part of a function? I.e the Coro resume case

[wenlei]

In D113492#3119430, @wlei wrote:

In D113492#3118908, @wenlei wrote:

However, in HHVM, I found that there are many functions that none of their ranges is set to true

Why is this happening? Should we fix this so that for any function, there's always one range that is considered entry?

For example, in HHVM:
The DWARF function name is "_ZN8facebook6falcon10ProcessKeyC2ERKS1_" and the same start address on ELF the symbol name is " _ZN8facebook6falcon10ProcessKeyC1ERKS1_". The function has only one range.

So there is name difference between DWARF and symbol table even it has only one range. I don't know the root reason for debug info, maybe some change in FE

Should we fix this so that for any function, there's always one range that is considered entry?

I'm thinking a case even we make the function entry flag correct, there's still a chance to call a non-entry function that should report the count, like "main" --> "foo.V1" , maybe foo.V1 is the function slice covering function start.

That shouldn't happen. Function calls has to go through prolog, and we shouldn't expect to see cross-function transfer in the middle of a function without going through prolog.

See the change in this patch, try to remove the isFuncEntry logic. Instead, we check if the source and start' range is from the same function but different split range.

[wlei]

Updating D113492: [llvm-profgen] Fix bug of split range branch sample

[wlei]

That shouldn't happen. Function calls has to go through prolog, and we shouldn't expect to see cross-function transfer in the middle of a function without going through prolog.

That makes sense. Abandon this patch, reverted to previous version.

update to mark the only range in the function as the entry and warn if no entry range exists.

llvm/tools/llvm-profgen/ProfileGenerator.cpp

419 ↗

(On Diff #385845)

I see, it can't, abandon this patch, reverted to previous version.

[wlei]

Update the statistics summary of HHVM:

Before the num of function has no entry:

1.82%(5687/312230) of functions have no entry range!

After fix to mark the only range as entry:

0.12%(383/312230) of functions have no entry range!

[hoy]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

356

Can the flag be set here based on FuncRange->Func->Ranges.size() == 1 instead?

[wlei]

add getCanonicalFnName for symbol name
move setIsFuncEntry in front of dissassembleSymbol in case there're alias symbol
addressing Hongtao's feedback

llvm/tools/llvm-profgen/ProfiledBinary.cpp

356

This is better place, thanks for the suggestion!

[wenlei]

The change description needs to be updated too.

llvm/tools/llvm-profgen/ProfiledBinary.cpp

177

This warning is a bit confusing.. Technically there's no such thing as function doesn't have an entry. How about this: Failed to determine function entry for xxx due to inconsistent name from symbol table and dwarf info.

382–383

Moving this check is for printing symbols with zero size, right?

What symbols have zero size?

[wlei]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

382–383

Yes, all is due to the alias issues. Those are two type: 1)C1 vs C2 2) undemangled symbol

zero-size: _ZN5boost6threadC1Ev
<_ZN5boost6threadC2Ev>:

zero-size: numa_set_membind_v2
<numa_set_membind_v2_int>:

See they are all right followed by their non-zero size alias.

But but not vice versa, remembering one of C1 or C2 can't be removed.

[hoy]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

382–383

On the second thought, what does this check do? I thought symbols are sorted based on their start offset before disassembled.

[wlei]

Updating D113492: [llvm-profgen] Fix bug of split range branch sample

llvm/tools/llvm-profgen/ProfiledBinary.cpp

177

Thanks for the suggestions!

382–383

I see, this check is no need then,

[hoy]

lgtm, thanks.

[wenlei]

lgtm, thanks.

llvm/tools/llvm-profgen/ErrorHandling.h

 }
 
 template <typename T, typename... Ts>
 T unwrapOrError(Expected<T> EO, Ts &&... Args) {
   if (EO)
     return std::move(*EO);
   exitWithError(EO.takeError(), std::forward<Ts>(Args)...);
 }
+
+inline void emitWarningSummary(uint64_t Num, uint64_t Total, StringRef Msg) {
+  if (!Total || !Num)
+    return;
+  WithColor::warning() << format("%.2f", static_cast<double>(Num) * 100 / Total)
+                       << "%(" << Num << "/" << Total << ") " << Msg << "\n";
+}
+
 #endif

llvm/tools/llvm-profgen/PerfReader.h

protected:
   // Update base address based on mmap events
   void updateBinaryAddress(const MMapEvent &Event);
   // Parse mmap event and update binary address
   void parseMMap2Event(TraceStream &TraceIt);
   // Parse perf events/samples and do aggregation
   void parseAndAggregateTrace();
   // Parse either an MMAP event or a perf sample
   void parseEventOrSample(TraceStream &TraceIt);
-  void emitWarningSummary(uint64_t Num, uint64_t Total, StringRef Msg);
   // Warn if the relevant mmap event is missing.
   void warnIfMissingMMap();
   // Emit accumulate warnings.
   void warnTruncatedStack();
   // Warn if range is invalid.
   void warnInvalidRange();
   // Extract call stack from the perf trace lines
   bool extractCallstack(TraceStream &TraceIt,

llvm/tools/llvm-profgen/PerfReader.cpp

static cl::opt<bool>
     UseOffset("use-offset", cl::init(true), cl::ZeroOrMore,
               cl::desc("Work with `--skip-symbolization` or "
                        "`--unsymbolized-profile` to write/read the "
                        "offset instead of virtual address."));
 static cl::opt<bool>
     IgnoreStackSamples("ignore-stack-samples", cl::init(false), cl::ZeroOrMore,
                        cl::desc("Ignore call stack samples for hybrid samples "
                                 "and produce context-insensitive profile."));
-static cl::opt<bool>
-    ShowDetailedWarning("show-detailed-warning", cl::init(false),
+cl::opt<bool> ShowDetailedWarning("show-detailed-warning", cl::init(false),
                                   cl::ZeroOrMore,
                                   cl::desc("Show detailed warning message."));
 
 extern cl::opt<std::string> PerfTraceFilename;
 extern cl::opt<bool> ShowDisassemblyOnly;
 extern cl::opt<bool> ShowSourceLocations;
 extern cl::opt<std::string> OutputFilename;
 
 namespace llvm {
 namespace sampleprof {
if (ShowDetailedWarning) {
     }
   }
   emitWarningSummary(
       InvalidReturnAddresses.size(), AggregatedSamples.size(),
       "of truncated stack samples due to invalid return address, "
       "likely caused by frame pointer omission.");
 }
 
-void PerfScriptReader::emitWarningSummary(uint64_t Num, uint64_t Total,
-                                          StringRef Msg) {
-  if (!Total || !Num)
-    return;
-  WithColor::warning() << format("%.2f", static_cast<double>(Num) * 100 / Total)
-                       << "%(" << Num << "/" << Total << ") " << Msg << "\n";
-}
-
 void PerfScriptReader::warnInvalidRange() {
   std::unordered_map<std::pair<uint64_t, uint64_t>, uint64_t,
                      pair_hash<uint64_t, uint64_t>>
       Ranges;
 
   for (const auto &Item : AggregatedSamples) {
     const PerfSample *Sample = Item.first.getPtr();
     uint64_t Count = Item.second;

llvm/tools/llvm-profgen/ProfiledBinary.h

 //===-- ProfiledBinary.h - Binary decoder -----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_PROFGEN_PROFILEDBINARY_H
 #define LLVM_TOOLS_LLVM_PROFGEN_PROFILEDBINARY_H
 
 #include "CallContext.h"
+#include "ErrorHandling.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/Symbolize/Symbolize.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDisassembler/MCDisassembler.h"
 #include "llvm/MC/MCInst.h"
class ProfiledBinary {
   // Load debug info of subprograms from DWARF section.
   void loadSymbolsFromDWARF(ObjectFile &Obj);
 
   // A function may be spilt into multiple non-continuous address ranges. We use
   // this to set whether start offset of a function is the real entry of the
   // function and also set false to the non-function label.
   void setIsFuncEntry(uint64_t Offset, StringRef RangeSymName);
 
+  // Warn if no entry range exists in the function.
+  void warnNoFuncEntry();
+
   /// Dissassemble the text section and build various address maps.
   void disassemble(const ELFObjectFileBase *O);
 
   /// Helper function to dissassemble the symbol and extract info for unwinding
   bool dissassembleSymbol(std::size_t SI, ArrayRef<uint8_t> Bytes,
                           SectionSymbolsTy &Symbols, const SectionRef &Section);
   /// Symbolize a given instruction pointer and return a full call context.
   SampleContextFrameVector symbolize(const InstructionPointer &IP,

llvm/tools/llvm-profgen/ProfiledBinary.cpp

static cl::opt<bool> UseDwarfCorrelation(
     cl::desc("Use dwarf for profile correlation even when binary contains "
              "pseudo probe."));
 
 static cl::list<std::string> DisassembleFunctions(
     "disassemble-functions", cl::CommaSeparated,
     cl::desc("List of functions to print disassembly for. Accept demangled "
              "names only. Only work with show-disassembly-only"));
 
+extern cl::opt<bool> ShowDetailedWarning;
+
 namespace llvm {
 namespace sampleprof {
 
 static const Target *getTarget(const ObjectFile *Obj) {
   Triple TheTriple = Obj->makeTriple();
   std::string Error;
   std::string ArchName;
   const Target *TheTarget =
for (const auto &ChildNode : ProbeNode.getChildren()) {
     InlineSite Location = ChildNode.first;
     ProbeContext.back().second = std::get<1>(Location);
     trackInlineesOptimizedAway(ProbeDecoder, *ChildNode.second.get(), ProbeContext);
   }
 
   ProbeContext.pop_back();
 }
 
+void ProfiledBinary::warnNoFuncEntry() {
+  uint64_t NoFuncEntryNum = 0;
+  for (auto &F : BinaryFunctions) {
+    if (F.second.Ranges.empty())
+      continue;
+    bool hasFuncEntry = false;
+    for (auto &R : F.second.Ranges) {
+      if (FuncRange *FR = findFuncRangeForStartOffset(R.first)) {
+        if (FR->IsFuncEntry) {
+          hasFuncEntry = true;
+          break;
+        }
+      }
+    }
+
+    if (!hasFuncEntry) {
+      NoFuncEntryNum++;
+      if (ShowDetailedWarning)
+        WithColor::warning()
+            << "Failed to determine function entry for " << F.first
+            << " due to inconsistent name from symbol table and dwarf info.\n";
+    }
+  }
+  emitWarningSummary(NoFuncEntryNum, BinaryFunctions.size(),
+                     "of functions failed to determine function entry due to "
+                     "inconsistent name from symbol table and dwarf info.");
+}
+
 void ProfiledBinary::load() {
   // Attempt to open the binary.
   OwningBinary<Binary> OBinary = unwrapOrError(createBinary(Path), Path);
   Binary &Binary = *OBinary.getBinary();
 
   auto *Obj = dyn_cast<ELFObjectFileBase>(&Binary);
   if (!Obj)
     exitWithError("not a valid Elf image", Path);
void ProfiledBinary::load() {
   // Track size for optimized inlinees when probe is available
   if (UsePseudoProbes && TrackFuncContextSize)
     FuncSizeTracker.trackInlineesOptimizedAway(ProbeDecoder);
 
   // Use function start and return address to infer prolog and epilog
   ProEpilogTracker.inferPrologOffsets(StartOffset2FuncRangeMap);
   ProEpilogTracker.inferEpilogOffsets(RetOffsets);
 
+  warnNoFuncEntry();
+
   // TODO: decode other sections.
 }
 
 bool ProfiledBinary::inlineContextEqual(uint64_t Address1, uint64_t Address2) {
   uint64_t Offset1 = virtualAddrToOffset(Address1);
   uint64_t Offset2 = virtualAddrToOffset(Address2);
   const SampleContextFrameVector &Context1 = getFrameLocationStack(Offset1);
   const SampleContextFrameVector &Context2 = getFrameLocationStack(Offset2);

 void ProfiledBinary::setIsFuncEntry(uint64_t Offset, StringRef RangeSymName) {
   // Note that the start offset of each ELF section can be a non-function
   // symbol, we need to binary search for the start of a real function range.
   auto *FuncRange = findFuncRangeForOffset(Offset);
   // Skip external function symbol.
   if (!FuncRange)
     return;
 
-  // Set IsFuncEntry to ture if the RangeSymName from ELF is equal to its
-  // DWARF-based function name.
-  if (!FuncRange->IsFuncEntry && FuncRange->getFuncName() == RangeSymName)
+  // Set IsFuncEntry to ture if there is only one range in the function or the
+  // RangeSymName from ELF is equal to its DWARF-based function name.
+  if (FuncRange->Func->Ranges.size() == 1 ||
+      (!FuncRange->IsFuncEntry && FuncRange->getFuncName() == RangeSymName))
     FuncRange->IsFuncEntry = true;
 }
 
 bool ProfiledBinary::dissassembleSymbol(std::size_t SI, ArrayRef<uint8_t> Bytes,
                                         SectionSymbolsTy &Symbols,
                                         const SectionRef &Section) {
   std::size_t SE = Symbols.size();
   uint64_t SectionOffset = Section.getAddress() - getPreferredBaseAddress();
   uint64_t SectSize = Section.getSize();
   uint64_t StartOffset = Symbols[SI].Addr - getPreferredBaseAddress();
   uint64_t NextStartOffset =
       (SI + 1 < SE) ? Symbols[SI + 1].Addr - getPreferredBaseAddress()
                     : SectionOffset + SectSize;
-  if (StartOffset > NextStartOffset)
-    return true;
+  setIsFuncEntry(StartOffset,
+                 FunctionSamples::getCanonicalFnName(Symbols[SI].Name));
 
   StringRef SymbolName =
       ShowCanonicalFnName
           ? FunctionSamples::getCanonicalFnName(Symbols[SI].Name)
           : Symbols[SI].Name;
   bool ShowDisassembly =
       ShowDisassemblyOnly && (DisassembleFunctionSet.empty() ||
                               DisassembleFunctionSet.count(SymbolName));
   if (ShowDisassembly)
     outs() << '<' << SymbolName << ">:\n";
 
   auto WarnInvalidInsts = [](uint64_t Start, uint64_t End) {
     WithColor::warning() << "Invalid instructions at "
                          << format("%8" PRIx64, Start) << " - "
                          << format("%8" PRIx64, End) << "\n";
   };
 
   uint64_t Offset = StartOffset;
   // Size of a consecutive invalid instruction range starting from Offset -1
   // backwards.
   uint64_t InvalidInstLength = 0;
bool ProfiledBinary::dissassembleSymbol(std::size_t SI, ArrayRef<uint8_t> Bytes,
   }
 
   if (InvalidInstLength)
     WarnInvalidInsts(Offset - InvalidInstLength, Offset - 1);
 
   if (ShowDisassembly)
     outs() << "\n";
 
-  setIsFuncEntry(StartOffset, Symbols[SI].Name);
-
   return true;
 }
 
 void ProfiledBinary::setUpDisassembler(const ELFObjectFileBase *Obj) {
   const Target *TheTarget = getTarget(Obj);
   std::string TripleName = TheTriple.getTriple();
   StringRef FileName = Obj->getFileName();