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

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Jonas,

Thanks for the review!


================
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
   )
   
----------------
Hahnfeld wrote:
> 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)
The bundler tool already depends on clang, so that would cause a circular dependency. I think that in general not building the bundler is fine - the user may not be interested in doing offloading, so if he attempts to do so, that would fail as, say, ld was not in the system.

I'm adding it only for testing because there are tests that will exercise the bundler that will fail if the driver does not detect the tool. 

Should we ask someone in specific for an opinion? Let me know your thoughts. 

================
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) {
----------------
Hahnfeld wrote:
> `to/from`?
Thanks for catching this! Fixed in the last diff, it should be `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) {
----------------
Hahnfeld wrote:
> Duplicate `and write`? `to/from`?
Fixed in the last diff, it should be `to`.

================
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;
----------------
Hahnfeld wrote:
> Better say that we haven't found the bundle for the host?
Makes sense, I changed the message in the last diff.


https://reviews.llvm.org/D13909