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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits