[PATCH] D21987: [XRay] Implement `llvm-xray extract`, start of the llvm-xray tool

[David Blaikie via llvm-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D21987#576231, @dberris wrote:

> This is ready for another look @dblaikie -- added some failure tests that were easy to catch (not found, no instrumentation map) and provided some sample binaries to use instead of creating one on the fly (as it seemed harder to generate a final linked executable from a .ll).
>
> If you have ideas about how to make the sequence:
>
>   llc ...; <link the .o into a final binary> ;  llvm-xray extract <final binary>
>   
>
> work in a `.ll` test, I'd be very interested to try.


Could you explain why you need a linked binary rather than just an object file? What changes? Relocations, I guess? All the addresses look like zero?

Yeah, doubt there's much to do about that - no way to produce linked binaries with the LLVM tools available to the test suite, and no point implementing support in the tool to handle unapplied relocations (unlike dwarfdump which does handle them, and there's more reason to run dwarfdump on unlinked objects, so it's worth having that support for more than just testing).



================
Comment at: tools/llvm-xray/xray-extract.cc:89-92
+  if (Filename.empty())
+    return make_error<StringError>(
+        "Provided filename is empty.",
+        std::make_error_code(std::errc::no_such_file_or_directory));
----------------
Untested (do we need this test? Presumably we just won't be able to open the file)


================
Comment at: tools/llvm-xray/xray-extract.cc:97-98
+  // FIXME: Maybe support other ELF formats. For now, 64-bit Little Endian only.
+  if (!ObjectFile)
+    return ObjectFile.takeError();
+
----------------
I'm guessing this is the path we take for "no such file or directory"? (ie: covered by the test for that)


================
Comment at: tools/llvm-xray/xray-extract.cc:100-103
+  if (!ObjectFile->getBinary()->isELF())
+    return make_error<StringError>(
+        "File format not supported (only does ELF).",
+        std::make_error_code(std::errc::not_supported));
----------------
Untested


================
Comment at: tools/llvm-xray/xray-extract.cc:118-121
+  if (I->getContents(Contents))
+    return make_error<StringError>(
+        "Failed to get contents of 'xray_instr_map' section.",
+        std::make_error_code(std::errc::executable_format_error));
----------------
Untested (though, granted - not sure quite how to test this, but could look further into libObject to see how getContents can fail)


================
Comment at: tools/llvm-xray/xray-extract.cc:127-131
+  if ((C - Contents.bytes_end()) % ELF64SledEntrySize != 0)
+    return make_error<StringError>(
+        "Instrumentation map entries not evenly divisible by size of an XRay "
+        "sled entry in ELF64.",
+        std::make_error_code(std::errc::executable_format_error));
----------------
Untested


================
Comment at: tools/llvm-xray/xray-extract.cc:206-208
+    EC =
+        make_error<StringError>("Input format type not supported yet.",
+                                std::make_error_code(std::errc::not_supported));
----------------
Guess this should just be an assert for now? (represents a programmer error/isn't reachable/testable/etc?)


================
Comment at: tools/llvm-xray/xray-registry.cc:34-35
+  auto It = Commands->find(SC);
+  if (It == Commands->end())
+    llvm_unreachable("Attempting to dispatch on un-registered SubCommand.");
+  return It->second;
----------------
Replace branch-to-unreachable with assert.


https://reviews.llvm.org/D21987