[PATCH] D24376: [XRay] Implement `llvm-xray convert` -- trace file conversion

[David Blaikie via llvm-commits]

dblaikie added a comment.

Before I get into all the details - this seems similar to the extract tool, except it deals with the log rather than the instrumentation map, right?

So pretty much all the same feedback as there (& I don't necessarily remember the original answers). We ended up with a one way conversion last time (to yaml, but not back again), should this one be different? If we only convert one way, is 'convert' the right name? Should be be more explicit about the difference between these two things, or just detect based on file magic & use one command name for both kinds of input files? (check if it's an ELF file then assume the user is trying to extract an instrumentation map, and otherwise assume the file's a log?)



================
Comment at: tools/llvm-xray/func-id-helper.cc:40-42
+  if (It == FunctionAddresses.end()) {
+    return "(unknown)";
+  }
----------------
We don't usually bother putting {} on single-line if statements.


================
Comment at: tools/llvm-xray/func-id-helper.cc:49-53
+  if (LastSlash != std::string::npos) {
+    F << DI.FileName.substr(LastSlash + 1);
+  } else {
+    F << DI.FileName;
+  }
----------------
Probably drop the {} here too (maybe even use the conditional operator, if you like)


================
Comment at: tools/llvm-xray/xray-record.h:33
+
+struct alignas(32) XRayRecord {
+  uint16_t RecordType = 0;
----------------
Is the alignas attribute supported on all the platforms/compilers we care about?


================
Comment at: tools/llvm-xray/xray-record.h:53
+  char Buffer[4] = {};
+} __attribute__((packed));
+
----------------
There's LLVM_PACKED you may need to use to make this portable to MSVC, by the looks of it.

(though I'm still intrinsically suspicious of splatting in/out of memory, and would be more comfortable seeing just 'obvious' code to read bytes and shift them into values, etc - but perhaps I'm in the minority/an outlier here and this sort of code is OK with everyone else)


https://reviews.llvm.org/D24376