[PATCH] D21987: [XRay] Initial XRay Function Call Accounting Tool

[Dean Michael Berris via llvm-commits]

dberris added a comment.

> Tests generally go in with the functionality in the same commit - and tests for the <foo> tool live in the test/tools/<foo> directory.


Thanks -- yes, I'll add them to the change, but thought of holding off until I got a bit more feedback. :)

> If some of this functionality ends up sufficiently factored out into utility libraries, unit tests may be written - though unit tests are pretty rare in LLVM (just culturally - I'm actually more of a fan of them) outside of the general data structures in ADT and some of Support. Mostly it's just end-to-end testing of specific tools, optimization passes, etc.


Since this is a new tool, I'd like to be able to test the units in isolation, so I'll add both end-to-end tests and unit tests. This makes reasoning about the requirements and implementation a lot easier.

I'll ping when I have them lined up, it shouldn't take too long as long as I don't go too deep into the refactoring rabbit hole. :)


================
Comment at: tools/xray/xray-fc-account.cc:168-171
@@ +167,6 @@
+  // Find the section named "xray_instr_map".
+  StringRef Contents = "";
+  const auto &Sections = ObjectFile->getBinary()->sections();
+  auto I = llvm::find_if(Sections, [&](const object::SectionRef &Section) {
+    StringRef Name = "";
+    if (Section.getName(Name))
----------------
dblaikie wrote:
> Helps a bit - but generally LLVM tools are platform agnostic. It sounds like this currently is not - that the tool would only work on the platform that generated the file (& that when we port xray to other platforms, again, the tool would only work on the desired platform)
> 
> Seems pretty likely someone might take a report from some buildbot that they don't have local hardware for, then want to analyze that on their x86 developer machine. So I think it's probably a good idea to ensure the tools are not platform specific like this. But perhaps I'm overengineering this?
Good point -- no your suggestion isn't over-engineered.

I think a neutral format is much better for the tooling. I suspect we can provide a platform-dependent way of extracting the instrumentation map and exporting it in a neutral format. I'm thinking json is the most portable and easier format to consume/transport. So instead of doing this ELF-specific parsing/loading, we can make this step something that can be done in isolation.

I'll break this out into a specific tool that takes an XRay-instrumented binary, and extracts the instrumentation map and turns it into JSON. That way, we can implement this extraction mechanism for every platform supported and have one canonical format for the instrumentation map. This then becomes an alternative input (note that the instrumentation map isn't exactly required, since the XRay trace contains most/all the information that's useful, albeit in the form of function id's). If all we ever had was the function id's, then we can still provide useful information -- the instrumentation map helps only for providing human-readable metadata, like which function is called, and if we have debugging symbols where the function was defined.

================
Comment at: tools/xray/xray-fc-account.cc:237-267
@@ +236,33 @@
+
+  std::string FileLineAndColumn(int32_t FuncId) const {
+    auto It = FunctionAddresses.find(FuncId);
+    if (It == FunctionAddresses.end()) {
+      return "(unknown)";
+    }
+    std::ostringstream F;
+    auto ResOrErr = Symbolizer.symbolizeCode(BinaryInstrMap, It->second);
+    if (!ResOrErr || ResOrErr.get().FileName == "<invalid>")
+      return "(unknown)";
+    auto &DI = ResOrErr.get();
+    auto LastSlash = DI.FileName.find_last_of('/');
+    if (LastSlash != std::string::npos) {
+      F << DI.FileName.substr(LastSlash + 1);
+    } else {
+      F << DI.FileName;
+    }
+    F << ":" << DI.Line << ":" << DI.Column;
+    return F.str();
+  }
+};
+
+// We consolidate the data into a struct which we can output in various forms.
+struct ResultRow {
+  uint64_t Count;
+  double Min;
+  double Median;
+  double Pct90;
+  double Pct99;
+  double Max;
+  std::string DebugInfo;
+  std::string Function;
+};
----------------
dblaikie wrote:
> I'm not sure whether a generic CSV-style output library would be that important/useful, but maybe I'm not quite picturing what you have in mind.
> 
> As for an output system - I'd probably skip the array of variants and use a variadic function template to take all the fields.
> 
> I was thinking something more high level/closer to the format you have in mind here, but perhpas I'm having trouble picturing exactly what the output formats are and how generic an API to them would look.
> 
> Perhaps there is a reasonable high level API for this that could abstract away the differences between CSV and pretty printed output that wouldn't be specific to the contents.
> 
> Test cases might make it easier to see what the final output formats look like & thus what some common API to produce that information might look like.
> 
> 
I hadn't thought of just a variadic template -- yes, this would work, with dispatch for handling special kinds of values (i.e. if it's a string, wrap it in quotes and escape any quotes already in the string, etc.).

I'll add test for this specific API since I think it's important to get this right.


https://reviews.llvm.org/D21987