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

Fri Jun 5 11:11:11 PDT 2020

aganea added a comment.

In D80833#2073458 <https://reviews.llvm.org/D80833#2073458>, @hans wrote:

> 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.

Understood, I wasn't using `-no-canonical-prefixes` that's why I was seeing full paths for argv[0].

================
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
----------------
hans wrote:
> 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).
Fixed, Reid already mentionned that a while ago, I'll remember next time!

================
Comment at: clang/tools/driver/cc1_main.cpp:184

-int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
   ensureSufficientStack();
----------------
hans wrote:
> 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?
Reverted.

================
Comment at: lld/COFF/PDB.cpp:1041
+  // Remap the contents of the LF_BUILDINFO record.
+  remapBuildInfo(tMerger.getIDTable());
+
----------------
hans wrote:
> 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".
The whole type merging machinery isn't designed for changing records on-the-fly. We only modify TypeIndex values inside the record, but for that we have hardcoded offsets (all the `discoverTypeIndices` functions in https://github.com/llvm/llvm-project/blob/cda7ff9ddcefe0051d173e7c126c679063d29fbb/llvm/lib/DebugInfo/CodeView/TypeIndexDiscovery.cpp). The rest of the record is emitted as-it-is in the output TPI or IPI streams.

It's certainly feaseable to modify records in `TypeStreamMerger` in the same way we examine the `LF_ENDPRECOMP` record. However that requires a lot more piping, and I figured it wasn't worth it (unless we have other record types to modify in the future).

================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:840
+  erase_if(Vec, [&](StringRef Arg) {
+    return Arg.data() == nullptr || Arg == MainFilename;
+  });
----------------
hans wrote:
> Does Arg.data() == nullptr really happen here?
Yes if passing `clang -cc1` directly. Both the `CC1Command` and `InitLLVM` are pushing a `nullptr` terminator as the last arg :-( This seems quite widespead behavior across the codebase.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:846
+#else
+  std::string FlatCmdLine = llvm::join(Vec, " ");
+#endif
----------------
hans wrote:
> 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?
The code for printing "clang -v" is/was entirely in Clang: https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/Job.cpp#L103

I've moved `Command::printArg` to `llvm::sys::printArg` and using that now.
This creates a slight difference from MSVC, in the sense that with Clang all arguments are quoted, no matter what. Whereas MSVC adds quotes when it's necessary (that's LLVM's `sys::flattenWindowsCommandLine` behavior). But I think that shouldn't matter much.

Please @stefan_reinalter @lantictac let me know if you think otherwise.

```
D:\llvm-project> buildninjaRel\bin\clang-cl /c b.cpp /Z7 -fdebug-compilation-dir . -no-canonical-prefixes
D:\llvm-project> buildninjaRel\bin\llvm-pdbutil.exe dump -all b.obj | grep LF_BUILDINFO -A 5
0x1007 | LF_BUILDINFO [size = 28]
         0x1003: `.`
         0x1005: `buildninjaRel\bin\clang-cl.exe`
         0x1004: `b.cpp`
         <no type>: ``
         0x1006: `"-cc1" "-triple" "x86_64-pc-windows-msvc19.26.28806" "-emit-obj" "-mrelax-all" "-mincremental-linker-compatible" "-disable-free" "-main-file-name" "b.cpp" "-mrelocation-model" "pic" "-pic-level" "2" "-mthread-model" "posix" "-mframe-pointer=none" "-relaxed-aliasing" "-fmath-errno" "-fno-rounding-math" "-mconstructor-aliases" "-munwind-tables" "-target-cpu" "x86-64" "-mllvm" "-x86-asm-syntax=intel" "-D_MT" "-flto-visibility-public-std" "--dependent-lib=libcmt" "--dependent-lib=oldnames" "-stack-protector" "2" "-fms-volatile" "-fdiagnostics-format" "msvc" "-gcodeview" "-debug-info-kind=limited" "-resource-dir" "buildninjaRel\\lib\\clang\\11.0.0" "-internal-isystem" "buildninjaRel\\lib\\clang\\11.0.0\\include" "-internal-isystem" "C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Professional\\VC\\Tools\\MSVC\\14.26.28801\\ATLMFC\\include" "-internal-isystem" "C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Professional\\VC\\Tools\\MSVC\\14.26.28801\\include" "-internal-isystem" "C:\\Program Files (x86)\\Windows Kits\\NETFXSDK\\4.8\\include\\um" "-internal-isystem" "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\ucrt" "-internal-isystem" "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\shared" "-internal-isystem" "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\um" "-internal-isystem" "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\winrt" "-internal-isystem" "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\cppwinrt" "-fdeprecated-macro" "-fdebug-compilation-dir" "." "-ferror-limit" "19" "-fmessage-length=146" "-fno-use-cxa-atexit" "-fms-extensions" "-fms-compatibility" "-fms-compatibility-version=19.26.28806" "-std=c++14" "-fdelayed-template-parsing" "-fcolor-diagnostics" "-faddrsig" "-x" "c++"`
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80833