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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
