[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I think the biggest concern here is what to do about `/Fa` / `-save-temps`. If we do nothing, the S_OBJNAME record will change if you run the same compilation twice with and without those flags. Generally, we strive to ensure that the directly emitted object file is identical to one rinsed through textual assembly, but that doesn't always work. I'm not sure if `/Fa` works reliably since we switched to Intel style assembly syntax either.



================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:212
+  /// Output filename used in the COFF debug information.
+  std::string COFFOutputFilename;
+
----------------
Let's bikeshed the name a bit. This thing is the `-o`/`/Fo` value, plus some processing. It can be symbolic, as in `-`. It could be the name of a bitcode file with `-flto`. It could be the name of an assembly file if you do `clang -S --target=x86_64-windows-msvc -g t.cpp -o t.s`. It could be the name of an ELF file if you try hard. I think in any of these cases where the user directly emits something that isn't a COFF object, it's OK to use that name for the S_OBJNAME record.

What it is, is the name of the compiler's output file, as we would like it to appear in debug info. With that in mind, here are some ideas:
- CodeViewObjectName: very directly referring to S_OBJNAME
- ObjectFilename: very general
- ObjectFilenameForDebug: generalizing over cv/dwarf
- OutputFilenameForDebug: generalizing over assembly, bc, and obj

I think I like ObjectFilenameForDebug best. DebugObjectFilename seems like a no-go, since that sounds like the dwo file name.

---

This brings me to the case of `-save-temps` / `/Fa`. In these cases, the compile action is broken into pieces, and the assembler is invoked in a follow-up process. Does that mean the driver needs to pass an extra flag along to the -cc1 action? That would be a bummer.



================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:717
+
+  getCodeGenOpts().COFFOutputFilename = OutputPathName;
+
----------------
I think saving the output file name during output file creation seems like an unexpected side effect for this function. Can we handle it earlier, maybe near dwo file name handling?


================
Comment at: clang/test/CodeGenCXX/debug-info-objname.cpp:4
+
+// No output file provided, input file is relative, we emit an absolute path (MSVC behavior).
+// RUN: %clang_cl /c /Z7 -nostdinc debug-info-objname.cpp
----------------
I checked, this is also consistent with clang's regular /Z7 output for source filenames.


================
Comment at: clang/test/CodeGenCXX/debug-info-objname.cpp:20-21
+
+// The input file name is relative and we specify -fdebug-compilation-dir, we emit a relative path.
+// RUN: %clang_cl /c /Z7 -nostdinc -fdebug-compilation-dir=. debug-info-objname.cpp
+// RUN: llvm-pdbutil dump -all debug-info-objname.obj | FileCheck %s --check-prefix=RELATIVE
----------------
And this is the thing we rely on, so it should all work.


================
Comment at: llvm/include/llvm/MC/MCTargetOptions.h:63
   std::string SplitDwarfFile;
+  std::string COFFOutputFilename;
 
----------------
This should probably be in llvm/include/llvm/Target/TargetOptions.h. I think MCTargetOptions is intended to control assembler behavior, but this option is read while producing assembly in CodeGen, not during assembling.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:752
 
+static void unescapeSlashes(SmallVectorImpl<char> &Str) {
+  auto Read = Str.begin();
----------------
This isn't unescaping them, it's just canonicalizing double slashes to one slash, right? Would `llvm::sys::native` suffice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002