[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson added a comment.

The naming convention used here for COFF is actually very similar to what is done for other ThinLTO save-temps output files, which are placed at the input module locations. We may want to just do this across the board. @MaskRay wdyt? A few other questions/comments below.



================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1104
 
-  auto AddStream = [&](size_t Task) {
+  auto AddStream = [&](size_t Task, Twine File) {
     return std::make_unique<CachedFileStream>(std::move(OS),
----------------
I think you might need to mark the new parameter as unused here and in other cases where it isn't used via LLVM_ATTRIBUTE_UNUSED, to avoid build warnings - I can't recall how strictly that is enforced.


================
Comment at: lld/COFF/LTO.cpp:229
+    StringRef ltoObjName;
+    if (bitcodeFilePath == "ld-temp.o") {
+      ltoObjName =
----------------
This case should always be i==0 I think?


================
Comment at: lld/COFF/LTO.cpp:245
+      saveBuffer(buf[i].second, ltoObjName);
     ret.push_back(make<ObjFile>(ctx, MemoryBufferRef(objBuf, ltoObjName)));
   }
----------------
The above changes affect both the MemoryBufferRef name as well as the saveTemps output file name. I assume the change to the former is what is required for PDB, is that correct?


================
Comment at: lld/MachO/LTO.cpp:132
+        "ThinLTO", "Thin", config->thinLTOCacheDir,
+        [&](size_t task, Twine originFile, std::unique_ptr<MemoryBuffer> mb) {
+          files[task] = std::move(mb);
----------------
In some files this parameter is called "filed" and in others "originFile". Better to be consistent, and perhaps to use a more descriptive name, something like moduleName ?


================
Comment at: llvm/include/llvm/Support/Caching.h:53
 ///
 /// if (AddStreamFn AddStream = Cache(Task, Key))
 ///   ProduceContent(AddStream);
----------------
This and possibly other header file descriptions need updates.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217