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

[Haohai, Wen via Phabricator via cfe-commits]

HaohaiWen added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:1167
 // GCC style -dumpdir. We intentionally don't implement the less useful -dumpbase{,-ext}.
-def dumpdir : Separate<["-"], "dumpdir">, Flags<[CC1Option]>,
+def dumpdir : Separate<["-"], "dumpdir">, Flags<[CC1Option, CoreOption]>,
   MetaVarName<"<dumppfx>">,
----------------
MaskRay wrote:
> This exposes `-dumpdir ` to clang-cl which may not be useful.
clang -gdwarf -gsplit-dwarf foo.cpp -o foo.exe -### will be expanded to
clang -cc1 ... "-dumpdir" "foo.exe-" .... "-split-dwarf-file" "foo.exe-foo.dwo"

dwo name will be prefixed by dumpdir if it's specified.

This dumpdir is required for clang-cl to have same dwo filename behavior as clang.


================
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(
----------------
MaskRay wrote:
> Add a test to show how OPT__SLASH_o passes a `-dumpdir `to CC1.
Already tested in (-o is OPT__SLASH_o in cl mode)

```
clang/test/Driver/split-debug.c: 62
// RUN: %clang_cl -### --target=x86_64-unknown-windows-msvc -gsplit-dwarf -g %s -o obj/out 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK
/ SPLIT_LINK:      "-dumpdir" "obj/out-"
// SPLIT_LINK:      "-debug-info-kind=constructor"
// SPLIT_LINK-SAME: "-split-dwarf-file" "obj/out-split-debug.dwo" "-split-dwarf-output" "obj/out-split-debug.dwo"
```


================
Comment at: clang/test/Driver/split-debug.c:19
 // RUN: %clang -### -c -target amdgcn-amd-amdhsa -gsplit-dwarf -g %s 2>&1 | FileCheck %s --check-prefix=SPLIT
+// RUN: %clang_cl -### -c -target x86_64-unknown-windows-msvc -gsplit-dwarf -g %s 2>&1 | FileCheck %s --check-prefix=SPLIT
 
----------------
MaskRay wrote:
> MaskRay wrote:
> > Use `--target=` for new tests. `-target ` has been deprecated since Clang 3.x
> You can use `-gno-split-dwarf -gsplit-dwarf` to test `CoreOption` on `-gno-split-dwarf`.
Good suggestion.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1895
+      if (RelocatedSection != Obj.section_end() && Name.contains(".dwo")) {
+        // Each section in COFF can directly contain relocations.
+        if (isa<COFFObjectFile>(&Obj) && Section.relocations().empty())
----------------
MaskRay wrote:
> skan wrote:
> > The comment is confusing according to line 1900. Could you refine it?
> I'll change this to `ends_with(".dwo")` separately.
> 
> `check-llvm check-clang` passes even if I remove `if (isa<COFFObjectFile>(&Obj) && Section.relocations().empty()) continue`. What does it do?
For ELF, if section a.dwo has relocations, it'll have a a.dwo.rela section, this section is RelocatedSection.
All dwo section should not have relocations, so if this is a dwo file, then RelocatedSection should be Obj.section_end(). Otherwise it will report warning.

For COFF, relocations of section A are directly stored in section A. RelocatedSection is Section itself so it will never be Obj.section_end(). Then this warning will always be reported. What we need to do is to check Section.relocations().empty() for COFF sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152785