[PATCH] D127246: [LinkerWrapper] Rework the linker wrapper and use owning binaries

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

I think this is probably OK. Smaller patches usually get reviewed faster so minimising the line noise in the browser is worthwhile.



================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:784
     };
-    Conf.PostInternalizeModuleHook = [&](size_t, const Module &M) {
+    Conf.PostInternalizeModuleHook = [&, Arch](size_t, const Module &M) {
       SmallString<128> TempFile;
----------------
jhuber6 wrote:
> JonChesterfield wrote:
> > Why call out Arch here? Passing a StringRef by reference via the & should be fine
> It apparently wasn't fine. For whatever reason when this function was called by the LTO engine this value was getting mutated and pointing to bad memory. If I capture it by-value everything is fine. I didn't care enough to look into it once I figured out the fix.
Interesting. The filename operator+ behaving differently for StringRef vs StringRef& seems suspicious. Passing by std::string is presumably safer again, but I guess this will do for now.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1136
+
+  Triple TheTriple = Triple(Images.front().StringData.lookup("triple"));
+  auto FileOrErr = nvptx::fatbinary(InputFiles, TheTriple);
----------------
jhuber6 wrote:
> JonChesterfield wrote:
> > This is close but not quite a copy of existing code, moving it around in the file at the same time as changing it makes the diff significantly harder to follow
> Sorry, I don't use a header file here so it's a little bit of a pain to make sure new functions can call the ones they need. This function is fundamentally different from the old one so I wouldn't worry about the old implementations.
You could probably move the old functions further up in the file as a precommit change so that they lined up in the functional diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127246