[CSSPGO][llvm-profgen] Renovate perfscript check and command line input validation

[wlei]

Summary

This include some changes related with PerfReader's the input check and command line change:

1. It appears there might be thousands of leading MMAP-Event line in the perfscript for large workload. For this case, the 4k threshold is not eligible to determine it's a hybrid sample. This change renovated the isHybridPerfScript by going through the script without threshold limitation checking whether there is a non-empty call stack immediately followed by a LBR sample. It will stop once it find a valid one.

2. Added several input validations for the command line switches in PerfReader.

3. Changed the command line show-disassembly to show-disassembly-only, it will print to stdout and exit early which leave an empty output profile.

[wenlei]

llvm/tools/llvm-profgen/PerfReader.h

522

A comment showing expected input would be helpful. For LBR part, do we want to check more than leading 0x?

Should we also detect case where input doesn't have LBR at all? We don't have to do it here, but after we've processed the entire input, if no hybrid sample is found, emit an error too?

[wlei]

llvm/tools/llvm-profgen/PerfReader.h

522

Should we also detect case where input doesn't have LBR at all? We don't have to do it here, but after we've processed the entire input, if no hybrid sample is found, emit an error too?

Do you mean we should have three state for that?

1. LBR + call stack, which is detected in this function.
2. LBR only (when we detect the LBR but Count is zero)
3. no LBRs at all.

For the 3) case, we will emit an error.

[hoy]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

404

nit: isShowDisassemblyEnabled

llvm/tools/llvm-profgen/llvm-profgen.cpp

47

We should probably return here with --show-disassembly and the output file should have the disassemblies instead of a sample profile.

[wenlei]

llvm/tools/llvm-profgen/PerfReader.h

522

Ok now I see the following error for #3, thanks for clarifying. Though I think it handles the case for hybrid sample, but if samples don't have stack, we don't emit error when there's no lbr? But that's probably fine as regular (non-hybrid) one is a TODO anyways.

'Hybrid perf sample is corrupted, No LBR sample line

llvm/tools/llvm-profgen/llvm-profgen.cpp

47

I thought show-disassembly prints the assembly to stdout, while the generated profile goes into output file. However, if we want the tool to show assembly instead of generating profile, we may need to rename the switch to something like show-disassembly-only.

[wlei]

Addressing Hongtao and Wenlei's feedback

[wlei]

llvm/tools/llvm-profgen/PerfReader.h

522

A comment showing expected input would be helpful.

Example added

For LBR part, do we want to check more than leading 0x?

Just separate a function for this and add another condition to check whether '/' exists

but if samples don't have stack, we don't emit error when there's no lbr

I think I miss this. If there is no stack and no lbr, we should emit error earlier. Added in the latest diff, thanks for the remainder.

llvm/tools/llvm-profgen/llvm-profgen.cpp

47

I see, how about we support both?
show-disassembly print to stdout and also generate profile.
show-disassembly-only only print to output file, this can be used for the disassembly output only test.

[hoy]

llvm/tools/llvm-profgen/PerfReader.h

525

Add a test for this?

llvm/tools/llvm-profgen/llvm-profgen.cpp

47

Sounds good, thanks!

[wlei]

Address Hongtao's feedback: added test case for invalid script check

[wlei]

llvm/tools/llvm-profgen/PerfReader.h

525

Just added the invalid-perfscript.test, is that the test you want?
I think we already have the test for hybrid sample since all the profile generation test use hybrid sample as input.

[wenlei]

llvm/tools/llvm-profgen/PerfReader.h

62

typo: PERF_INVALID. Can we keep the enum 0 based?

llvm/tools/llvm-profgen/llvm-profgen.cpp

47

These wrappers and layers in reader and binary feels like we're doing a bit too much for showAssembly. In the end, showAssembly is not the main purpose of the tool. How about simplify it:

• Let's always print showAssembly to stdout. This is more like a verbose log rather than output, so I think it's better to avoid dumping it in the output file.
• Only have show-disassembly-only mode (which is mostly used in combination with probe annotation), add early exit for that, and produce empty output profile.

[wlei]

address reviewers' feedback:
rename showAssembly to show-disassembly-only and change back to print to stdout

llvm/tools/llvm-profgen/PerfReader.h

62

Good catch!

llvm/tools/llvm-profgen/llvm-profgen.cpp

47

thanks for the discussing, fixed！

[wlei]

fix clang-tidy

[wenlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

86

Why do we need to go through PerfReader::getOutputFilename in order to access the output file from command line?

Same for ProfiledBinary::isShowDisassemblyOnlyEnabled etc., can we access the global directly (add extern declaration if needed)? These wrappers seem a bit overkill given they're not really per-binary for per-reader config.

[hoy]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

21

Why need this? Will std::out work?

llvm/tools/llvm-profgen/ProfiledBinary.h

261

Remove this since we are keeping only show-disassembly-only?

[wlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

86

StringRef OutputFilename = PerfReader::getOutputFilename();

yeah, I miss to remove this.

can we access the global directly (add extern declaration if needed)?

I thought to use static function wrapper as the global. Yeah, use an extern global is clearer. Let me change it

[wlei]

address Wenlei and Hongtao's feedback: switch to use global command line variable and some refactoring work

[wlei]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

21

This is redundant, removed.

llvm/tools/llvm-profgen/ProfiledBinary.h

261

Yeah, overlook this, thanks for the reminder.

[wenlei]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

33

This works, but we could make it simpler:

ProfileBinary.cpp:

cl::opt<bool> ShowDisassembly("show-disassembly", cl::ReallyHidden,
                                     cl::init(false), cl::ZeroOrMore,
                                     cl::desc("Print disassembled code."));

llvm-profgen.cpp:

extern cl::opt<bool> ShowDisassembly;

[wlei]

change to extern cl::opt<bool> ShowDisassemblyOnly

[wlei]

llvm/tools/llvm-profgen/ProfiledBinary.cpp

33

Yeah, learned it!

[wenlei]

lgtm, thanks.

[hoy]

LGTM. Maybe update the summary to reflect the command line change? Thanks.

llvm/tools/llvm-profgen/PerfReader.h

if (!std::getline(Fin, CurrentLine)) {
       return;
     }
     LineNumber++;
   }
 };
 
 // The type of perfscript
 enum PerfScriptType {
   PERF_INVILID = 0,
   PERF_LBR = 1,       // Only LBR sample
   PERF_LBR_STACK = 2, // Hybrid sample including call stack and LBR stack.
 };
 
 // The parsed LBR sample entry.
 struct LBREntry {
   uint64_t Source = 0;
   uint64_t Target = 0;

 // counter
 using BinarySampleCounterMap =
     std::unordered_map<ProfiledBinary *, ContextSampleCounterMap>;
 
 // Load binaries and read perf trace to parse the events and samples
 class PerfReader {
 
 public:
-  PerfReader(cl::list<std::string> &BinaryFilenames);
+  PerfReader(cl::list<std::string> &BinaryFilenames,
+             cl::list<std::string> &PerfTraceFilenames);
 
-  // Hybrid sample(call stack + LBRs) profile traces are seprated by double line
-  // break, search for that within the first 4k charactors to avoid going
-  // through the whole file.
+  // Determine the perfscript contains hybrid samples(call stack + LBRs) by
+  // checking whether there is a non-empty call stack immediately followed by
+  // a LBR sample
   static bool isHybridPerfScript(StringRef FileName) {
-    auto BufOrError = MemoryBuffer::getFileOrSTDIN(FileName, 4000);
-    if (!BufOrError)
-      exitWithError(BufOrError.getError(), FileName);
-    auto Buffer = std::move(BufOrError.get());
-    if (Buffer->getBuffer().find("\n\n") == StringRef::npos)
-      return false;
+    TraceStream TraceIt(FileName);
+    uint64_t FrameAddr = 0;
+    while (!TraceIt.isAtEoF()) {
+      int32_t Count = 0;
+      while (!TraceIt.isAtEoF() &&
+             !TraceIt.getCurrentLine().ltrim().getAsInteger(16, FrameAddr)) {
+        Count++;
+        TraceIt.advance();
+      }
+      if (!TraceIt.isAtEoF()) {
+        if (Count && TraceIt.getCurrentLine().startswith(" 0x"))
           return true;
+        TraceIt.advance();
+      }
+    }
+    return false;
   }
 
   // The parsed MMap event
   struct MMapEvent {
     uint64_t PID = 0;
     uint64_t BaseAddress = 0;
     uint64_t Size = 0;
     uint64_t Offset = 0;
public:
   PerfScriptType getPerfScriptType() const { return PerfType; }
   // Entry of the reader to parse multiple perf traces
   void parsePerfTraces(cl::list<std::string> &PerfTraceFilenames);
   const BinarySampleCounterMap &getBinarySampleCounters() const {
     return BinarySampleCounters;
   }
 
 private:
+  /// Validate the command line input
+  void validateCommandLine(cl::list<std::string> &BinaryFilenames,
+                           cl::list<std::string> &PerfTraceFilenames);
   /// Parse a single line of a PERF_RECORD_MMAP2 event looking for a
   /// mapping between the binary name and its memory layout.
   ///
   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);

llvm/tools/llvm-profgen/PerfReader.cpp

while (State.hasNextLBR()) {
     recordBranchCount(Branch, State, Repeat);
   }
   // As samples are aggregated on trie, record them into counter map
   collectSamplesFromFrameTrie(State.getDummyRootPtr());
 
   return true;
 }
 
-PerfReader::PerfReader(cl::list<std::string> &BinaryFilenames) {
+void PerfReader::validateCommandLine(
+    cl::list<std::string> &BinaryFilenames,
+    cl::list<std::string> &PerfTraceFilenames) {
+  // Allow the invalid perfscript if we only use to show binary disassembly
+  if (!ProfiledBinary::isShowDisassemblyEnable()) {
+    for (auto &File : PerfTraceFilenames) {
+      if (!llvm::sys::fs::exists(File)) {
+        std::string Msg = "Input perf script(" + File + ") doesn't exist!";
+        exitWithError(Msg);
+      }
+    }
+  }
+  if (BinaryFilenames.size() > 1) {
+    // TODO: remove this if everything is ready to support multiple binaries.
+    exitWithError(
+        "Currently only support one input binary, multiple binaries' "
+        "profile will be merged in one profile and make profile "
+        "summary info inaccurate. Please use `llvm-perfdata` to merge "
+        "profiles from multiple binaries.");
+  }
+  for (auto &Binary : BinaryFilenames) {
+    if (!llvm::sys::fs::exists(Binary)) {
+      std::string Msg = "Input binary(" + Binary + ") doesn't exist!";
+      exitWithError(Msg);
+    }
+  }
+  if (CSProfileGenerator::MaxCompressionSize < -1) {
+    exitWithError("Value of --compress-recursion should >= -1");
+  }
+  if (ProfiledBinary::isShowSourceLocationsEnable() &&
+      !ProfiledBinary::isShowDisassemblyEnable()) {
+    exitWithError("--show-source-locations should work together with "
+                  "--show-disassembly!");
+  }
+}
+
+PerfReader::PerfReader(cl::list<std::string> &BinaryFilenames,
+                       cl::list<std::string> &PerfTraceFilenames) {
+  validateCommandLine(BinaryFilenames, PerfTraceFilenames);
   // Load the binaries.
   for (auto Filename : BinaryFilenames)
     loadBinary(Filename, /*AllowNameConflict*/ false);
 }
 
 ProfiledBinary &PerfReader::loadBinary(const StringRef BinaryPath,
                                        bool AllowNameConflict) {
   // The binary table is currently indexed by the binary name not the full
void PerfReader::checkAndSetPerfType(
   }
 
   if (HasHybridPerf) {
     PerfType = PERF_LBR_STACK;
   } else {
     // TODO: Support other type of perf script
     PerfType = PERF_INVILID;
   }
-
-  if (BinaryTable.size() > 1) {
-    // TODO: remove this if everything is ready to support multiple binaries.
-    exitWithError("Currently only support one input binary, multiple binaries' "
-                  "profile will be merged in one profile and make profile "
-                  "summary info inaccurate. Please use `perfdata` to merge "
-                  "profiles from multiple binaries.");
-  }
 }
 
 void PerfReader::generateRawProfile() {
   if (getPerfScriptType() == PERF_LBR_STACK) {
     // Unwind samples if it's hybird sample
     unwindSamples();
   } else if (getPerfScriptType() == PERF_LBR) {
     // TODO: range overlap computation for regular AutoFDO

llvm/tools/llvm-profgen/ProfileGenerator.cpp

 //===-- ProfileGenerator.cpp - Profile Generator  ---------------*- 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 "ProfileGenerator.h"
 
 static cl::opt<std::string> OutputFilename("output", cl::value_desc("output"),
                                            cl::Required,
                                            cl::desc("Output profile file"));
-
+static cl::alias OutputA("o", cl::desc("Alias for --output"),
+                         cl::aliasopt(OutputFilename));
 static cl::opt<SampleProfileFormat> OutputFormat(
     "format", cl::desc("Format of output profile"), cl::init(SPF_Text),
     cl::values(
         clEnumValN(SPF_Binary, "binary", "Binary encoding (default)"),
         clEnumValN(SPF_Compact_Binary, "compbinary", "Compact binary encoding"),
         clEnumValN(SPF_Ext_Binary, "extbinary", "Extensible binary encoding"),
         clEnumValN(SPF_Text, "text", "Text encoding"),
         clEnumValN(SPF_GCC, "gcc",

 }
 
 void ProfileGenerator::write(std::unique_ptr<SampleProfileWriter> Writer,
                              StringMap<FunctionSamples> &ProfileMap) {
   Writer->write(ProfileMap);
 }
 
 void ProfileGenerator::write() {
   auto WriterOrErr = SampleProfileWriter::create(OutputFilename, OutputFormat);
   if (std::error_code EC = WriterOrErr.getError())
     exitWithError(EC, OutputFilename);
   write(std::move(WriterOrErr.get()), ProfileMap);
 }
 
 void ProfileGenerator::findDisjointRanges(RangeSample &DisjointRanges,
                                           const RangeSample &Ranges) {
 

llvm/tools/llvm-profgen/ProfiledBinary.h

const AddressProbesMap &getAddress2ProbesMap() const {
     return ProbeDecoder.getAddress2ProbesMap();
   }
   const PseudoProbeFuncDesc *getFuncDescForGUID(uint64_t GUID) {
     return ProbeDecoder.getFuncDescForGUID(GUID);
   }
   const PseudoProbeFuncDesc *getInlinerDescForProbe(const PseudoProbe *Probe) {
     return ProbeDecoder.getInlinerDescForProbe(Probe);
   }
+  static bool isShowDisassemblyEnable();
+  static bool isShowSourceLocationsEnable();
 };
 
 } // end namespace sampleprof
 } // end namespace llvm
 
 #endif

llvm/tools/llvm-profgen/ProfiledBinary.cpp

 #include "llvm/Demangle/Demangle.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 
 #define DEBUG_TYPE "load-binary"
 
 using namespace llvm;
 using namespace sampleprof;
 
 static cl::opt<bool> ShowDisassembly("show-disassembly", cl::ReallyHidden,
                                      cl::init(false), cl::ZeroOrMore,
                                      cl::desc("Print disassembled code."));
 
 static cl::opt<bool> ShowSourceLocations("show-source-locations",
                                          cl::ReallyHidden, cl::init(false),
                                          cl::ZeroOrMore,
                                          cl::desc("Print source locations."));
 
 static cl::opt<bool> ShowPseudoProbe(
     "show-pseudo-probe", cl::ReallyHidden, cl::init(false), cl::ZeroOrMore,
     cl::desc("Print pseudo probe section and disassembled info."));
 
 namespace llvm {
 namespace sampleprof {
 
 static const Target *getTarget(const ObjectFile *Obj) {
   Triple TheTriple = Obj->makeTriple();
LineLocation Line(CallerFrame.Line - CallerFrame.StartLine,
                       CallerFrame.Discriminator);
     FrameLocation Callsite(FunctionName.str(), Line);
     CallStack.push_back(Callsite);
   }
 
   return CallStack;
 }
 
+bool ProfiledBinary::isShowDisassemblyEnable() { return ShowDisassembly; }
+
+bool ProfiledBinary::isShowSourceLocationsEnable() {
+  return ShowSourceLocations;
+}
+
 InstructionPointer::InstructionPointer(ProfiledBinary *Binary, uint64_t Address,
                                        bool RoundToNext)
     : Binary(Binary), Address(Address) {
   Index = Binary->getIndexForAddr(Address);
   if (RoundToNext) {
     // we might get address which is not the code
     // it should round to the next valid address
     this->Address = Binary->getAddressforIndex(Index);

llvm/tools/llvm-profgen/llvm-profgen.cpp

int main(int argc, const char *argv[]) {
   // Initialize targets and assembly printers/parsers.
   InitializeAllTargetInfos();
   InitializeAllTargetMCs();
   InitializeAllDisassemblers();
 
   cl::ParseCommandLineOptions(argc, argv, "llvm SPGO profile generator\n");
 
   // Load binaries and parse perf events and samples
-  PerfReader Reader(BinaryFilenames);
+  PerfReader Reader(BinaryFilenames, PerfTraceFilenames);
   Reader.parsePerfTraces(PerfTraceFilenames);
 
   std::unique_ptr<ProfileGenerator> Generator = ProfileGenerator::create(
       Reader.getBinarySampleCounters(), Reader.getPerfScriptType());
   Generator->generateProfile();
   Generator->write();
 
   return EXIT_SUCCESS;
 }