[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+    if (StringRef(A->getValue()) == "single")
+      return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
     SmallString<128> T(FinalOutput->getValue());
----------------
grimar wrote:
> grimar wrote:
> > dblaikie wrote:
> > > If this function now takes the output file name - could it be simplified to just always use that, rather than the conditional code that follows and tests whether -o is specified and builds up something like the output file name & uses the dwo suffix?
> > I am investigating this.
> So what I see now is that when the function becomes:
> 
> ```
> const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
>                                   const InputInfo &Output) {
>   SmallString<128> T(Output.getFilename());
>   llvm::sys::path::replace_extension(T, "dwo");
>   return Args.MakeArgString(T);
> }
> ```
> 
> Then no clang tests (check-clang) fail. This does not seem normal to me.
> I would expect that such a change should break some `-fdebug-compilation-dir` relative 
> logic at least and I suspect we just have no test(s) atm.
> 
> What about if I add test case(s) and post a follow-up refactor patch for this place?
> (I started work on it, but it will take some time).
Sounds good - thanks!


================
Comment at: test/CodeGen/split-debug-single-file.c:12
+//  RUN:   -enable-split-dwarf=split -split-dwarf-file %t.o -emit-obj -o %t.o %s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck --check-prefix=MODE-SPLIT %s
+//  MODE-SPLIT-NOT: .dwo
----------------
grimar wrote:
> dblaikie wrote:
> > This looks like an end-to-end test, which we don't usually do in Clang (or in the LLVM project in general).
> > 
> > For example, the previous testing for split-dwarf had a driver test (that tested only that the driver produced the right cc1 invocation) and a CodeGen test (that tested that the right IR was produced), but I don't see any test like this (that tested the resulting object file)?
> > 
> > I know there's a narrow gap in IR testing - things that don't go in the IR, but instead go through MCOptions  & that the SplitDwarfFile is one of those.
> > 
> > So, yeah, in this case it's a bit of a test hole - if you're going to keep this test, perhaps it could test assembly, rather than the object file? Since it doesn't need complex parsing, etc, perhaps?
> > So, yeah, in this case it's a bit of a test hole - if you're going to keep this test, perhaps it could test assembly, rather than the object file? Since it doesn't need complex parsing, etc, perhaps?
> 
> I am not sure assembly can help here. For example
> `clang main.c -S -g -gsplit-dwarf` would create single asm file output anyways
> and what I tried to check here is that we can either place .dwo sections into the
> same output or into a different one depending on new option.
> 
> > For example, the previous testing for split-dwarf had a driver test (that tested only that the driver produced the right cc1 invocation) and a CodeGen test (that tested that the right IR was produced), but I don't see any test like this (that tested the resulting object file)?
> 
> I think `split-debug-filename.c` do that?
> See: https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/split-debug-filename.c#L18
> 
> Also, `relax.c`: https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/relax.c
> And `mips-inline-asm-abi.c`:  https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/mips-inline-asm-abi.c
> 
> Seems it is not common, but still acceptable?
Ah, I see/good point, split-debug-filename.c tests both the IR & then also tests the object headers.

Sounds good


https://reviews.llvm.org/D52296