[PATCH] D152785: [COFF] Support -gsplit-dwarf for COFF on Windows

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:3898
     if (!Args.hasArg(options::OPT_dumpdir)) {
+      Arg *FinalOutput = Args.getLastArg(options::OPT_o, options::OPT__SLASH_o);
       Arg *Arg = Args.MakeSeparateArg(
----------------
Add a test to show how OPT__SLASH_o passes a `-dumpdir `to CC1.


================
Comment at: llvm/lib/MC/MCAsmBackend.cpp:71
         cast<MCWasmObjectTargetWriter>(std::move(TW)), OS, DwoOS);
+  case Triple::COFF:
+    return createWinCOFFDwoObjectWriter(
----------------
Move before ELF for an alphabetical order.


================
Comment at: llvm/lib/MC/MCAsmBackend.cpp:75
   default:
-    report_fatal_error("dwo only supported with ELF and Wasm");
+    report_fatal_error("dwo only supported with ELF Wasm and COFF");
   }
----------------



================
Comment at: llvm/lib/MC/WinCOFFObjectWriter.cpp:848
 
-  for (const MCSymbol &Symbol : Asm.symbols())
-    if (!Symbol.isTemporary())
-      DefineSymbol(Symbol, Asm, Layout);
+  if (Mode != DwoOnly) {
+    for (const MCSymbol &Symbol : Asm.symbols())
----------------
this cascading `if`/`for` with a single-line body doesn't need `{}`


================
Comment at: llvm/lib/MC/WinCOFFObjectWriter.cpp:1155
+                       (Mode == DwoOnly && !isDwoSection(*J))))
+      J++;
+    assert(J != JE && (**I).MCSection == &*J && "Wrong bound MCSection");
----------------
`++J`. See coding standards


================
Comment at: llvm/lib/MC/WinCOFFObjectWriter.cpp:124
 
+bool isDwoSection(const MCSection &Sec) {
+  return Sec.getName().endswith(".dwo");
----------------
HaohaiWen wrote:
> MaskRay wrote:
> > static
> It's already in anonymous namespace.
Perhaps that can be fixed separate, but the standard recommends `static` over an unnamed namespace for functions: 
https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152785