[PATCH] D13909: clang-offload-bundler - offload files bundling/unbundling tool

[Jonas Hahnfeld via cfe-commits]

Hahnfeld accepted this revision.
Hahnfeld added a reviewer: Hahnfeld.
Hahnfeld added a comment.
This revision is now accepted and ready to land.

LGTM with only some minor nits on some of the comments and a CMake question


================
Comment at: test/CMakeLists.txt:27-33
@@ -26,8 +26,9 @@
 
 list(APPEND CLANG_TEST_DEPS
   clang clang-headers
   clang-format
   c-index-test diagtool
   clang-tblgen
+  clang-offload-bundler
   )
   
----------------
I think `clang-offload-bundler` needs to be added as dependency for the `clang` target because it will really need the bundler at runtime, not only when testing...

(Disclaimer: I'm no CMake expert)

================
Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:102-104
@@ +101,5 @@
+  virtual void ReadHeader(MemoryBuffer &Input) = 0;
+  /// Read the marker of the next bundled to be read in the file. The triple of
+  /// the target associated with that bundled is returned. An empty string is
+  /// returned if there are no more bundles to be read.
+  virtual StringRef ReadBundleStart(MemoryBuffer &Input) = 0;
----------------
s/bundled/bundle/?

================
Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:151
@@ +150,3 @@
+
+/// Read 8-byte integers to/from a buffer in little-endian format.
+static uint64_t Read8byteIntegerFromBuffer(StringRef Buffer, size_t pos) {
----------------
`to/from`?

================
Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:164
@@ +163,3 @@
+
+/// Write and write 8-byte integers to/from a buffer in little-endian format.
+static void Write8byteIntegerToBuffer(raw_fd_ostream &OS, uint64_t Val) {
----------------
Duplicate `and write`? `to/from`?

================
Comment at: tools/clang-offload-bundler/ClangOffloadBundler.cpp:568
@@ +567,3 @@
+  if (!FoundHostBundle) {
+    llvm::errs() << "error: Can't find bundles for all requested targets\n";
+    return true;
----------------
Better say that we haven't found the bundle for the host?


https://reviews.llvm.org/D13909