[PATCH] D108291: [clang-nvlink-wrapper] Wrapper around nvlink for archive files

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Discussed at the multicompany meeting today. Consensus reached is that the whole-archive/no-whole-archive distinction is unimportant - we can ship a toolchain that has whole-archive semantics and later change the default when doing more work in the linker. It's not my idea of backwards compatibility but I'm sympathetic to the argument that there's enough churn in gpu offloading that this particular point is lost in the noise.

The code itself looks basically OK to me, it extract files & passes them unchanged into nvlink along with other files. Error handling is incomplete - we detect some things going wrong, but end up returning 0 anyway. Bunch of comments inline.

It would be nicer from a unix perspective if the tool read the archives and returned a list of files to pass to nvlink, in pipeline fashion, but that'll make temporary file cleanup hazardous. Forking nvlink seems better for that reason.

Might be a nice usability feature for --help to print some stuff then invoke nvlink with --help itself. If this is ~ a perfect filter, aside from expanding archives, it could conceivably be named nvlink and used wherever nvlink is.



================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:27
+#if !defined(_MSC_VER) && !defined(__MINGW32__)
+#include <unistd.h>
+#endif
----------------
what's unistd used for here? can we drop this?


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:123
+  int ExecResult = -1;
+  // const char *NVLProgram = NVLinkPath.c_str();
+  std::vector<StringRef> NVLArgs;
----------------
should lose the commented out code


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:127
+  if (!nvlink) {
+    errs() << "Error: nvlink program not found.";
+    return;
----------------
this seems a fairly likely failure mode - perhaps we should find nvlink first, and only start writing archives members into temporary files etc after establishing that it exists


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:136
+  }
+  printNVLinkCommand(NVLArgs);
+  ExecResult = llvm::sys::ExecuteAndWait(NVLProgram, NVLArgs);
----------------
This unconditionally writes an nvlink invocation to stderr. Not good post debugging the first implementation, programs should execute silently when successful. We could look for a debugging flag / environment variable if necessary for debugging this in the field


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:137
+  printNVLinkCommand(NVLArgs);
+  ExecResult = llvm::sys::ExecuteAndWait(NVLProgram, NVLArgs);
+  if (ExecResult) {
----------------
there's a risk that the large number of arguments that results here exceeds the platform limitations. I don't know offhand if executeandwait handles that (e.g. by creating response files), or if nvlink understands response files


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:156
+
+  llvm::Error Err = llvm::Error::success();
+  auto ChildEnd = Archive.child_end();
----------------
there's a using namespace llvm above, can remove a lot of llvm:: prefixes


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:196
+    std::error_code EC = llvm::sys::fs::remove(TmpFile);
+    reportIfError(EC, "Unable to delete temporary file");
+  }
----------------
does this name the file it couldn't delete?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108291/new/

https://reviews.llvm.org/D108291