[PATCH] D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input

[Fangrui Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe3b9af92a482: [Driver] -gsplit-dwarf: Produce .dwo 
regardless of -gN for IR input (authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94655

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/split-debug.c


Index: clang/test/Driver/split-debug.c
===
--- clang/test/Driver/split-debug.c
+++ clang/test/Driver/split-debug.c
@@ -26,11 +26,12 @@
 
 /// ... unless -fthinlto-index= is specified.
 // RUN: echo > %t.bc
-// RUN: %clang -### -c -target x86_64 -fthinlto-index=dummy -gsplit-dwarf 
%t.bc 2>&1 | FileCheck %s --check-prefix=THINLTO
+// RUN: %clang -### -c -target x86_64 -fthinlto-index=dummy -gsplit-dwarf 
%t.bc 2>&1 | FileCheck %s --check-prefix=IR
+// RUN: %clang -### -c -target x86_64 -gsplit-dwarf -x ir %t.bc 2>&1 | 
FileCheck %s --check-prefix=IR
 
-// THINLTO-NOT:  "-debug-info-kind=
-// THINLTO:  "-ggnu-pubnames"
-// THINLTO-SAME: "-split-dwarf-file" "{{.*}}.dwo" "-split-dwarf-output" 
"{{.*}}.dwo"
+// IR-NOT:  "-debug-info-kind=
+// IR:  "-ggnu-pubnames"
+// IR-SAME: "-split-dwarf-file" "{{.*}}.dwo" "-split-dwarf-output" "{{.*}}.dwo"
 
 /// -gno-split-dwarf disables debug fission.
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf -g -gno-split-dwarf %s 
2>&1 | FileCheck %s --check-prefix=NOSPLIT
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3728,9 +3728,10 @@
   return DwarfFissionKind::None;
 }
 
-static void RenderDebugOptions(const ToolChain , const Driver ,
+static void renderDebugOptions(const ToolChain , const Driver ,
const llvm::Triple , const ArgList ,
-   bool EmitCodeView, ArgStringList ,
+   bool EmitCodeView, bool IRInput,
+   ArgStringList ,
codegenoptions::DebugInfoKind ,
DwarfFissionKind ) {
   if (Args.hasFlag(options::OPT_fdebug_info_for_profiling,
@@ -3754,12 +3755,10 @@
   Args.hasFlag(options::OPT_fsplit_dwarf_inlining,
options::OPT_fno_split_dwarf_inlining, false);
 
-  // Normally -gsplit-dwarf is only useful with -gN. For -gsplit-dwarf in the
-  // backend phase of a distributed ThinLTO which does object file generation
-  // and no IR generation, -gN should not be needed. So allow -gsplit-dwarf 
with
-  // either -gN or -fthinlto-index=.
-  if (Args.hasArg(options::OPT_g_Group) ||
-  Args.hasArg(options::OPT_fthinlto_index_EQ)) {
+  // Normally -gsplit-dwarf is only useful with -gN. For IR input, Clang does
+  // object file generation and no IR generation, -gN should not be needed. So
+  // allow -gsplit-dwarf with either -gN or IR input.
+  if (IRInput || Args.hasArg(options::OPT_g_Group)) {
 Arg *SplitDWARFArg;
 DwarfFission = getDebugFissionKind(D, Args, SplitDWARFArg);
 if (DwarfFission != DwarfFissionKind::None &&
@@ -4956,8 +4955,9 @@
 AddClangCLArgs(Args, InputType, CmdArgs, , );
 
   DwarfFissionKind DwarfFission = DwarfFissionKind::None;
-  RenderDebugOptions(TC, D, RawTriple, Args, EmitCodeView, CmdArgs,
- DebugInfoKind, DwarfFission);
+  renderDebugOptions(TC, D, RawTriple, Args, EmitCodeView,
+ types::isLLVMIR(InputType), CmdArgs, DebugInfoKind,
+ DwarfFission);
 
   // Add the split debug info name to the command lines here so we
   // can propagate it to the backend.


Index: clang/test/Driver/split-debug.c
===
--- clang/test/Driver/split-debug.c
+++ clang/test/Driver/split-debug.c
@@ -26,11 +26,12 @@
 
 /// ... unless -fthinlto-index= is specified.
 // RUN: echo > %t.bc
-// RUN: %clang -### -c -target x86_64 -fthinlto-index=dummy -gsplit-dwarf %t.bc 2>&1 | FileCheck %s --check-prefix=THINLTO
+// RUN: %clang -### -c -target x86_64 -fthinlto-index=dummy -gsplit-dwarf %t.bc 2>&1 | FileCheck %s --check-prefix=IR
+// RUN: %clang -### -c -target x86_64 -gsplit-dwarf -x ir %t.bc 2>&1 | FileCheck %s --check-prefix=IR
 
-// THINLTO-NOT:  "-debug-info-kind=
-// THINLTO:  "-ggnu-pubnames"
-// THINLTO-SAME: "-split-dwarf-file" "{{.*}}.dwo" "-split-dwarf-output" "{{.*}}.dwo"
+// IR-NOT:  "-debug-info-kind=
+// IR:  "-ggnu-pubnames"
+// IR-SAME: "-split-dwarf-file" "{{.*}}.dwo" "-split-dwarf-output" "{{.*}}.dwo"
 
 /// -gno-split-dwarf disables debug fission.
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf -g -gno-split-dwarf %s 2>&1 | FileCheck %s --check-prefix=NOSPLIT
Index: clang/lib/Driver/ToolChains/Clang.cpp
===

[PATCH] D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.

In D94655#2498811 , @MaskRay wrote:

> In D94655#2498669 , @dblaikie wrote:
>
>> In D94655#2498548 , @MaskRay wrote:
>>
>>> In D94655#2498504 , @dblaikie 
>>> wrote:
>>>
 Is there any way to condition this on the type of the output, rather than 
 the input? (or, more specifically, on whether machine code is being 
 generated)

 Or maybe we could always pass the split-dwarf-file down through LLVM and 
 not need to conditionalize it at all? It'd be a no-op if there's no DWARF 
 in the IR anyway?
>>>
>>> I tried replacing `if (IRInput || Args.hasArg(options::OPT_g_Group)) {` 
>>> with `if (1)`, -gsplit-dwarf may produce .dwo for regular non-g .c compile.
>>
>> Are you saying that if you make that change -gsplit-dwarf does cause .dwo 
>> files to be created for non-g .c compiles? Do the dwo files have anything in 
>> them? I had modified llvm to dynamically choose split or non-split based on 
>> whether there was enough data to be worth splitting into a .dwo file, but I 
>> guess that situation might still be producing an empty .dwo file which isn't 
>> ideal - I haven't tested that.
>
>
>
>   % clang a.c -gsplit-dwarf -c
>   % readelf -WS a.dwo
>   There are 2 section headers, starting at offset 0x50:
>   
>   Section Headers:
> [Nr] Name  TypeAddress  OffSize   ES 
> Flg Lk Inf Al
> [ 0]   NULL 00 00 00  
> 0   0  0
> [ 1] .strtab   STRTAB   40 09 00  
> 0   0  1

OK, thanks. Yeah, wouldn't mind if that got fixed at some point. It turns up in 
existing behavior/without patches with situations like `clang++ -gmlt 
-gsplit-dwarf x.cpp -c` if there's no inlining (that's the feature I 
mentioned/referenced in my previous comment) - since the split CU would be 
empty, LLVM avoids emitting a split CU at all - but does end up with that empty 
dwo situation you described. If that bug were fixed (lazily choosing not to 
create a dwo file at all, if no CUs were emitted into it) then we could pass 
the split dwarf request straight down unmodified all the time - making the 
architecture as orthogonal as the user-facing feature is intended to be.

>>> Since we already have the
>>>
>>>   // For -g0 or -gline-tables-only, drop -gsplit-dwarf. This gets a bit more
>>>   // complicated if you've disabled inline info in the skeleton CUs
>>>   // (SplitDWARFInlining) - then there's value in composing split-dwarf and
>>>   // line-tables-only, so let those compose naturally in that case.
>>>
>>> logic, I think altering `DwarfFission` in the driver is fine. If not, I'd 
>>> hope the backend to process `DwarfFission` ...
>>
>> Sorry, I'm not understanding this comment - could you describe/rephrase it 
>> in more detail?
>
> The driver already decides that DwarfFission should be disabled in -g0 and 
> -g1 cases. If the driver does not already do this, passing through 
> DwarfFission in the driver and letting CC1 handle it seems right to me.
>
> Since the driver already has some logic, I think adding more logic about IR 
> input types is fine.

My concern is that we'll miss other cases where code generation/dwo file 
creation is done when handling special cases like this & the code would be more 
robust if we didn't have to special case things like this.

But I guess it'll do for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D94655#2498669 , @dblaikie wrote:

> In D94655#2498548 , @MaskRay wrote:
>
>> In D94655#2498504 , @dblaikie wrote:
>>
>>> Is there any way to condition this on the type of the output, rather than 
>>> the input? (or, more specifically, on whether machine code is being 
>>> generated)
>>>
>>> Or maybe we could always pass the split-dwarf-file down through LLVM and 
>>> not need to conditionalize it at all? It'd be a no-op if there's no DWARF 
>>> in the IR anyway?
>>
>> I tried replacing `if (IRInput || Args.hasArg(options::OPT_g_Group)) {` with 
>> `if (1)`, -gsplit-dwarf may produce .dwo for regular non-g .c compile.
>
> Are you saying that if you make that change -gsplit-dwarf does cause .dwo 
> files to be created for non-g .c compiles? Do the dwo files have anything in 
> them? I had modified llvm to dynamically choose split or non-split based on 
> whether there was enough data to be worth splitting into a .dwo file, but I 
> guess that situation might still be producing an empty .dwo file which isn't 
> ideal - I haven't tested that.



  % clang a.c -gsplit-dwarf -c
  % readelf -WS a.dwo
  There are 2 section headers, starting at offset 0x50:
  
  Section Headers:
[Nr] Name  TypeAddress  OffSize   ES 
Flg Lk Inf Al
[ 0]   NULL 00 00 00
  0   0  0
[ 1] .strtab   STRTAB   40 09 00
  0   0  1



>> Since we already have the
>>
>>   // For -g0 or -gline-tables-only, drop -gsplit-dwarf. This gets a bit more
>>   // complicated if you've disabled inline info in the skeleton CUs
>>   // (SplitDWARFInlining) - then there's value in composing split-dwarf and
>>   // line-tables-only, so let those compose naturally in that case.
>>
>> logic, I think altering `DwarfFission` in the driver is fine. If not, I'd 
>> hope the backend to process `DwarfFission` ...
>
> Sorry, I'm not understanding this comment - could you describe/rephrase it in 
> more detail?

The driver already decides that DwarfFission should be disabled in -g0 and -g1 
cases. If the driver does not already do this, passing through DwarfFission in 
the driver and letting CC1 handle it seems right to me.

Since the driver already has some logic, I think adding more logic about IR 
input types is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D94655#2498548 , @MaskRay wrote:

> In D94655#2498504 , @dblaikie wrote:
>
>> Is there any way to condition this on the type of the output, rather than 
>> the input? (or, more specifically, on whether machine code is being 
>> generated)
>>
>> Or maybe we could always pass the split-dwarf-file down through LLVM and not 
>> need to conditionalize it at all? It'd be a no-op if there's no DWARF in the 
>> IR anyway?
>
> I tried replacing `if (IRInput || Args.hasArg(options::OPT_g_Group)) {` with 
> `if (1)`, -gsplit-dwarf may produce .dwo for regular non-g .c compile.

Are you saying that if you make that change -gsplit-dwarf does cause .dwo files 
to be created for non-g .c compiles? Do the dwo files have anything in them? I 
had modified llvm to dynamically choose split or non-split based on whether 
there was enough data to be worth splitting into a .dwo file, but I guess that 
situation might still be producing an empty .dwo file which isn't ideal - I 
haven't tested that.

> Since we already have the
>
>   // For -g0 or -gline-tables-only, drop -gsplit-dwarf. This gets a bit more
>   // complicated if you've disabled inline info in the skeleton CUs
>   // (SplitDWARFInlining) - then there's value in composing split-dwarf and
>   // line-tables-only, so let those compose naturally in that case.
>
> logic, I think altering `DwarfFission` in the driver is fine. If not, I'd 
> hope the backend to process `DwarfFission` ...

Sorry, I'm not understanding this comment - could you describe/rephrase it in 
more detail?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D94655#2498504 , @dblaikie wrote:

> Is there any way to condition this on the type of the output, rather than the 
> input? (or, more specifically, on whether machine code is being generated)
>
> Or maybe we could always pass the split-dwarf-file down through LLVM and not 
> need to conditionalize it at all? It'd be a no-op if there's no DWARF in the 
> IR anyway?

I tried replacing `if (IRInput || Args.hasArg(options::OPT_g_Group)) {` with 
`if (1)`, -gsplit-dwarf may produce .dwo for regular non-g .c compile.

Since we already have the

  // For -g0 or -gline-tables-only, drop -gsplit-dwarf. This gets a bit more
  // complicated if you've disabled inline info in the skeleton CUs
  // (SplitDWARFInlining) - then there's value in composing split-dwarf and
  // line-tables-only, so let those compose naturally in that case.

logic, I think altering `DwarfFission` in the driver is fine. If not, I'd hope 
the backend to process `DwarfFission` ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Is there any way to condition this on the type of the output, rather than the 
input? (or, more specifically, on whether machine code is being generated)

Or maybe we could always pass the split-dwarf-file down through LLVM and not 
need to conditionalize it at all? It'd be a no-op if there's no DWARF in the IR 
anyway?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: dblaikie, tejohnson.
MaskRay requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This generalizes D94647  to IR input, as 
suggested by @tejohnson.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94655

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/split-debug.c


Index: clang/test/Driver/split-debug.c
===
--- clang/test/Driver/split-debug.c
+++ clang/test/Driver/split-debug.c
@@ -26,11 +26,12 @@
 
 /// ... unless -fthinlto-index= is specified.
 // RUN: echo > %t.bc
-// RUN: %clang -### -c -target x86_64 -fthinlto-index=dummy -gsplit-dwarf 
%t.bc 2>&1 | FileCheck %s --check-prefix=THINLTO
+// RUN: %clang -### -c -target x86_64 -fthinlto-index=dummy -gsplit-dwarf 
%t.bc 2>&1 | FileCheck %s --check-prefix=IR
+// RUN: %clang -### -c -target x86_64 -gsplit-dwarf -x ir %t.bc 2>&1 | 
FileCheck %s --check-prefix=IR
 
-// THINLTO-NOT:  "-debug-info-kind=
-// THINLTO:  "-ggnu-pubnames"
-// THINLTO-SAME: "-split-dwarf-file" "{{.*}}.dwo" "-split-dwarf-output" 
"{{.*}}.dwo"
+// IR-NOT:  "-debug-info-kind=
+// IR:  "-ggnu-pubnames"
+// IR-SAME: "-split-dwarf-file" "{{.*}}.dwo" "-split-dwarf-output" "{{.*}}.dwo"
 
 /// -gno-split-dwarf disables debug fission.
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf -g -gno-split-dwarf %s 
2>&1 | FileCheck %s --check-prefix=NOSPLIT
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3728,9 +3728,10 @@
   return DwarfFissionKind::None;
 }
 
-static void RenderDebugOptions(const ToolChain , const Driver ,
+static void renderDebugOptions(const ToolChain , const Driver ,
const llvm::Triple , const ArgList ,
-   bool EmitCodeView, ArgStringList ,
+   bool EmitCodeView, bool IRInput,
+   ArgStringList ,
codegenoptions::DebugInfoKind ,
DwarfFissionKind ) {
   if (Args.hasFlag(options::OPT_fdebug_info_for_profiling,
@@ -3754,12 +3755,10 @@
   Args.hasFlag(options::OPT_fsplit_dwarf_inlining,
options::OPT_fno_split_dwarf_inlining, false);
 
-  // Normally -gsplit-dwarf is only useful with -gN. For -gsplit-dwarf in the
-  // backend phase of a distributed ThinLTO which does object file generation
-  // and no IR generation, -gN should not be needed. So allow -gsplit-dwarf 
with
-  // either -gN or -fthinlto-index=.
-  if (Args.hasArg(options::OPT_g_Group) ||
-  Args.hasArg(options::OPT_fthinlto_index_EQ)) {
+  // Normally -gsplit-dwarf is only useful with -gN. For IR input, Clang does
+  // object file generation and no IR generation, -gN should not be needed. So
+  // allow -gsplit-dwarf with either -gN or IR input.
+  if (IRInput || Args.hasArg(options::OPT_g_Group)) {
 Arg *SplitDWARFArg;
 DwarfFission = getDebugFissionKind(D, Args, SplitDWARFArg);
 if (DwarfFission != DwarfFissionKind::None &&
@@ -4956,8 +4955,9 @@
 AddClangCLArgs(Args, InputType, CmdArgs, , );
 
   DwarfFissionKind DwarfFission = DwarfFissionKind::None;
-  RenderDebugOptions(TC, D, RawTriple, Args, EmitCodeView, CmdArgs,
- DebugInfoKind, DwarfFission);
+  renderDebugOptions(TC, D, RawTriple, Args, EmitCodeView,
+ types::isLLVMIR(InputType), CmdArgs, DebugInfoKind,
+ DwarfFission);
 
   // Add the split debug info name to the command lines here so we
   // can propagate it to the backend.


Index: clang/test/Driver/split-debug.c
===
--- clang/test/Driver/split-debug.c
+++ clang/test/Driver/split-debug.c
@@ -26,11 +26,12 @@
 
 /// ... unless -fthinlto-index= is specified.
 // RUN: echo > %t.bc
-// RUN: %clang -### -c -target x86_64 -fthinlto-index=dummy -gsplit-dwarf %t.bc 2>&1 | FileCheck %s --check-prefix=THINLTO
+// RUN: %clang -### -c -target x86_64 -fthinlto-index=dummy -gsplit-dwarf %t.bc 2>&1 | FileCheck %s --check-prefix=IR
+// RUN: %clang -### -c -target x86_64 -gsplit-dwarf -x ir %t.bc 2>&1 | FileCheck %s --check-prefix=IR
 
-// THINLTO-NOT:  "-debug-info-kind=
-// THINLTO:  "-ggnu-pubnames"
-// THINLTO-SAME: "-split-dwarf-file" "{{.*}}.dwo" "-split-dwarf-output" "{{.*}}.dwo"
+// IR-NOT:  "-debug-info-kind=
+// IR:  "-ggnu-pubnames"
+// IR-SAME: "-split-dwarf-file" "{{.*}}.dwo" "-split-dwarf-output" "{{.*}}.dwo"
 
 /// -gno-split-dwarf disables debug fission.
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf -g -gno-split-dwarf %s 2>&1 | FileCheck %s --check-prefix=NOSPLIT
Index: clang/lib/Driver/ToolChains/Clang.cpp