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

Jonas Hahnfeld via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 12 01:46:20 PDT 2016

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 @@
   clang clang-headers
   c-index-test diagtool
+  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;

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) {

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?


More information about the cfe-commits mailing list