[CSSPGO][llvm-profgen] Truncate stack samples with invalid return address.

[hoy]

Summary

Invalid frame addresses exist in call stack samples due to bad unwinding. This could happen to frame-pointer-based unwinding and the callee functions that do not have the frame pointer chain set up. It isn't common when the program is built with the frame pointer omission disabled, but can still happen with third-party static libs built with frame pointer omitted.

[hoy]

Updating D109638: [CSSPGO] Truncate stack samples with invalid return address.

[wenlei]

It isn't common when the program is built with the frame pointer omission disabled, but can still happen with third-party static libs built with frame pointer omitted.

Did you mean disabled -> enabled?

This could happen to frame-pointer-based unwinding and the callee functions that do not have the frame pointer chain set up.

So this leads to frame pointer being used to volatile register and assume it contains frame pointer would lead to bad frame address when unwinding the stack during profiling, is that what you observed?

llvm/tools/llvm-profgen/PerfReader.cpp

524–533

Can we hide all this complexity into getCallAddrFromFrameAddr? i.e. we could use bool getCallAddrFromFrameAddr(uint64_t FrameAddr, uint64_t &CallAddr).

530

Should we be using a warning too? For truncated context due to missing probe for merged callsite, we used warning. I think this is things of similar nature, should we also handle them in similar/consistent fashion- i.e. with warning instead of stats?

And if we expect duplicated warnings, would be good to deduplicated before printing.

[hoy]

In D109638#3000682, @wenlei wrote:

It isn't common when the program is built with the frame pointer omission disabled, but can still happen with third-party static libs built with frame pointer omitted.

Did you mean disabled -> enabled?

It's actually disabled. This happens when the program is built with -fno-omit-frame-pointer but the third-party lib is built with -fomit-frame-pointer.

This could happen to frame-pointer-based unwinding and the callee functions that do not have the frame pointer chain set up.

So this leads to frame pointer being used to volatile register and assume it contains frame pointer would lead to bad frame address when unwinding the stack during profiling, is that what you observed?

Exactly. I was seeing rbp used as a general register and its value was trashed.

llvm/tools/llvm-profgen/PerfReader.cpp

524–533

Sounds good to move it into getCallAddrFromFrameAddr.

530

Deduplicated warnings added.

[hoy]

Addressing Wenlei's comment.

[wenlei]

lgtm except two nits.

llvm/tools/llvm-profgen/PerfReader.cpp

772

nit: emitAccumulatedWarnings -> warnTruncatedStack

774

Nit: make the message more meaningful. Truncated stack sample due to invalid return address at 0xabcd, likely caused by frame pointer omission.

[hoy]

Updating D109638: [CSSPGO][llvm-profgen] Truncate stack samples with invalid return address.

llvm/tools/llvm-profgen/PerfReader.cpp

772

Makes sense. I was making this cover all warnings for future use. But sounds good to stick with truncated stack for now.

llvm/test/tools/llvm-profgen/Inputs/cs-invalid-ret-addr.perfscript

+PERF_RECORD_MMAP2 2854748/2854748: [0x400000(0x1000) @ 0 00:1d 123291722 526021]: r-xp /home/noinline-cs-noprobe.perfbin
+// test for invalid return address
+
+            4005b0
+            400686
+      7f68c5788793
+ 0x40062f/0x4005b0/P/-/-/0  0x400645/0x4005ff/P/-/-/0  0x400637/0x400645/P/-/-/0  0x4005e9/0x400634/P/-/-/0  0x4005c8/0x4005dc/P/-/-/0  0x40062f/0x4005b0/P/-/-/0  0x400645/0x4005ff/P/-/-/0  0x400637/0x400645/P/-/-/0  0x4005e9/0x400634/P/-/-/0  0x4005d7/0x4005e5/P/-/-/0  0x40062f/0x4005b0/P/-/-/0  0x400645/0x4005ff/P/-/-/0  0x400637/0x400645/P/-/-/0  0x4005e9/0x400634/P/-/-/0  0x4005d7/0x4005e5/P/-/-/0  0x40062f/0x4005b0/P/-/-/0
+
+            4005b2
+            400686
+      7f68c5788793
+ 0x40062f/0x4005b0/P/-/-/0  0x400645/0x4005ff/P/-/-/0  0x400637/0x400645/P/-/-/0  0x4005e9/0x400634/P/-/-/0  0x4005c8/0x4005dc/P/-/-/0  0x40062f/0x4005b0/P/-/-/0  0x400645/0x4005ff/P/-/-/0  0x400637/0x400645/P/-/-/0  0x4005e9/0x400634/P/-/-/0  0x4005d7/0x4005e5/P/-/-/0  0x40062f/0x4005b0/P/-/-/0  0x400645/0x4005ff/P/-/-/0  0x400637/0x400645/P/-/-/0  0x4005e9/0x400634/P/-/-/0  0x4005d7/0x4005e5/P/-/-/0  0x40062f/0x4005b0/P/-/-/0

llvm/test/tools/llvm-profgen/cs-invalid-ret-addr.test

+; REQUIRES: x86_64-linux
+; RUN: llvm-profgen --format=text  --perfscript=%S/Inputs/cs-invalid-ret-addr.perfscript --binary=%S/Inputs/noinline-cs-noprobe.perfbin --output=%t 2>&1 |  FileCheck %s
+
+; CHECK: warning: Truncated stack sample due to invalid return address at 0x400686, likely caused by frame pointer omission

llvm/tools/llvm-profgen/PerfReader.h

protected:
   ///
   void parseMMap2Event(TraceStream &TraceIt);
   // Parse perf events/samples and do aggregation
   void parseAndAggregateTrace(StringRef Filename);
   // Parse either an MMAP event or a perf sample
   void parseEventOrSample(TraceStream &TraceIt);
   // Warn if the relevant mmap event is missing.
   void warnIfMissingMMap();
+  // Emit accumulate warnings.
+  void warnTruncatedStack();
   // Extract call stack from the perf trace lines
   bool extractCallstack(TraceStream &TraceIt,
                         SmallVectorImpl<uint64_t> &CallStack);
   // Extract LBR stack from one perf trace line
   bool extractLBRStack(TraceStream &TraceIt,
                        SmallVectorImpl<LBREntry> &LBRStack);
   uint64_t parseAggregatedCount(TraceStream &TraceIt);
   // Parse one sample from multiple perf lines, override this for different
protected:
   virtual void writeRawProfile(raw_fd_ostream &OS) = 0;
 
   ProfiledBinary *Binary = nullptr;
 
   ContextSampleCounterMap SampleCounters;
   // Samples with the repeating time generated by the perf reader
   AggregatedCounter AggregatedSamples;
   PerfScriptType PerfType = PERF_UNKNOWN;
+  // Keep track of all invalid return addresses
+  std::set<uint64_t> InvalidReturnAddresses;
 };
 
 /*
   Hybrid perf script includes a group of hybrid samples(LBRs + call stack),
   which is used to generate CS profile. An example of hybrid sample:
     4005dc    # call stack leaf
     400634
     400684    # call stack root

llvm/tools/llvm-profgen/PerfReader.cpp

 //===-- PerfReader.cpp - perfscript reader  ---------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 #include "PerfReader.h"
 #include "ProfileGenerator.h"
 #include "llvm/Support/FileSystem.h"
 
+#define DEBUG_TYPE "perf-reader"
+
 static cl::opt<bool> ShowMmapEvents("show-mmap-events", cl::ReallyHidden,
                                     cl::init(false), cl::ZeroOrMore,
                                     cl::desc("Print binary load events."));
 
 cl::opt<bool> SkipSymbolization("skip-symbolization", cl::ReallyHidden,
                                 cl::init(false), cl::ZeroOrMore,
-                                cl::desc("Dump the unsumbolized profile to the "
+                                cl::desc("Dump the unsymbolized profile to the "
                                          "output file. It will show unwinder "
                                          "output for CS profile generation."));
 
 extern cl::opt<bool> ShowDisassemblyOnly;
 extern cl::opt<bool> ShowSourceLocations;
 extern cl::opt<std::string> OutputFilename;
 
 namespace llvm {
if (FrameStr.getAsInteger(16, FrameAddr)) {
       return false;
     }
     TraceIt.advance();
     // Currently intermixed frame from different binaries is not supported.
     // Ignore bottom frames not from binary of interest.
     if (!Binary->addressIsCode(FrameAddr))
       break;
 
-    // We need to translate return address to call address
-    // for non-leaf frames
+    // We need to translate return address to call address for non-leaf frames.
     if (!CallStack.empty()) {
-      FrameAddr = Binary->getCallAddrFromFrameAddr(FrameAddr);
+      auto CallAddr = Binary->getCallAddrFromFrameAddr(FrameAddr);
+      if (!CallAddr) {
+        // Stop at an invalid return address caused by bad unwinding. This could
+        // happen to frame-pointer-based unwinding and the callee functions that
+        // do not have the frame pointer chain set up.
+        InvalidReturnAddresses.insert(FrameAddr);
+        break;
+      }
+      FrameAddr = CallAddr;
     }
 
     CallStack.emplace_back(FrameAddr);
   }
 
   // Skip other unrelated line, find the next valid LBR line
   // Note that even for empty call stack, we should skip the address at the
   // bottom, otherwise the following pass may generate a truncated callstack
   while (!TraceIt.isAtEoF() && !TraceIt.getCurrentLine().startswith(" 0x")) {
if (PerfType != PERF_UNKNOWN && PerfType != Type)
       exitWithError("Inconsistent sample among different perf scripts");
     PerfType = Type;
   }
   return PerfType;
 }
 
 void HybridPerfReader::generateRawProfile() { unwindSamples(); }
 
+void PerfReaderBase::warnTruncatedStack() {
+  for (auto Address : InvalidReturnAddresses) {
+    WithColor::warning()
+        << "Truncated stack sample due to invalid return address at "
+        << format("0x%" PRIx64, Address)
+        << ", likely caused by frame pointer omission\n";
+  }
+}
+
 void PerfReaderBase::parsePerfTraces(
     cl::list<std::string> &PerfTraceFilenames) {
   // Parse perf traces and do aggregation.
   for (auto Filename : PerfTraceFilenames)
     parseAndAggregateTrace(Filename);
 
+  warnTruncatedStack();
   generateRawProfile();
 }
 
 } // end namespace sampleprof
 } // end namespace llvm

llvm/tools/llvm-profgen/ProfiledBinary.h

public:
   // using lower bound operation
   uint32_t getIndexForAddr(uint64_t Address) const {
     uint64_t Offset = virtualAddrToOffset(Address);
     auto Low = llvm::lower_bound(CodeAddrs, Offset);
     return Low - CodeAddrs.begin();
   }
 
   uint64_t getCallAddrFromFrameAddr(uint64_t FrameAddr) const {
-    return getAddressforIndex(getIndexForAddr(FrameAddr) - 1);
+    auto I = getIndexForAddr(FrameAddr);
+    FrameAddr = I ? getAddressforIndex(I - 1) : 0;
+    if (FrameAddr && addressIsCall(FrameAddr))
+      return FrameAddr;
+    return 0;
   }
 
   StringRef getFuncFromStartOffset(uint64_t Offset) {
     return FuncStartAddrMap[Offset];
   }
 
   uint32_t getFuncSizeForContext(SampleContext &Context) {
     return FuncSizeTracker.getFuncSizeForContext(Context);