[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

> The only absolute paths that remain are: a. the compiler path (`D:\llvm-project\buildninjaDebMSVC\bin\clang-cl.exe` in the yaml below) and b. the `-internal-isystem` paths. However that is not an issue on our end, as we're building with `-nostdinc` + nuget packages installed in a fixed dir on all users' machines. Not sure how that works for you?

a. The compiler path is only absolute because it was absolute when put into argv[0] right? I don't see anything in the code that makes it absolute? I imagine most build systems will invoke the compiler using an absolute path so you'll get the desired result.

b. We pass -imsvc flags to specify relative paths to the msvc headers, so the -internal-isystem paths passed to cc1 are relative too.



================
Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:1
+// RUN: %clang_cl /c /Z7 %s /Fo%t.obj
+// RUN: llvm-pdbutil dump --types %t.obj | FileCheck %s
----------------
The %s arg needs to come after "--", otherwise it can be interpreted as a command line flag, e.g. on Mac files are often under /Users which will be interpreted as a /U flag (see other tests using %clang_cl for examples).


================
Comment at: clang/tools/driver/cc1_main.cpp:184
 
-int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
   ensureSufficientStack();
----------------
Maybe I'm missing something, but why is this changing? The current code with Argv0 and the rest of the args as separate parameters seems like it would fit in will with the rest of the patch?


================
Comment at: lld/COFF/PDB.cpp:1041
+  // Remap the contents of the LF_BUILDINFO record.
+  remapBuildInfo(tMerger.getIDTable());
+
----------------
Naive question because I'm not familiar with the PDB writing, but would it be possible to remap the LF_BUILDINFO entries earlier, e.g. when they're read in from the object files? It seems like a lot of code is now added to do the remapping "after the fact".


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:840
+  erase_if(Vec, [&](StringRef Arg) {
+    return Arg.data() == nullptr || Arg == MainFilename;
+  });
----------------
Does Arg.data() == nullptr really happen here?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:846
+#else
+  std::string FlatCmdLine = llvm::join(Vec, " ");
+#endif
----------------
I suppose this won't work with filenames that contain spaces.

I was hoping we could do whatever "clang -v" does when it prints arguments. It seems to do some basic quoting and escaping. Does it have some function we could call from here?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:873
       getStringIdTypeIdx(TypeTable, MainSourceFile->getFilename());
-  // FIXME: Path to compiler and command line. PDB is intentionally blank unless
-  // we implement /Zi type servers.
+  if (!StringRef(Asm->TM.Options.MCOptions.Argv0).empty() &&
+      !Asm->TM.Options.MCOptions.CommandLineArgs.empty()) {
----------------
Wouldn't it be simpler to just check Argv0 != nullptr here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80833