[Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

[MaskRay]

Summary

When -gsplit-dwarf is used together with other -g options, in most cases
the computed debug info level is decided by the last -g option, with one
special case (see below). This patch drops that special case and thus
makes it easy to reason about:

-gsplit-dwarf -g0 => 0
-gsplit-dwarf -gline-directives-only => DebugDirectivesOnly
-gsplit-dwarf -gmlt -fsplit-dwarf-inlining => 1
-gsplit-dwarf -gmlt -fno-split-dwarf-inlining => 1 + split

-g0 -gsplit-dwarf => 2 + split
-gline-directives-only -gsplit-dwarf => 2 + split
-gmlt -gsplit-dwarf -fsplit-dwarf-inlining => 2 + split
-gmlt -gsplit-dwarf -fno-split-dwarf-inlining => 1 + split (before) 2 + split (after)

The last case has been changed. In general, if the user intends to lower
debug info level, place that -g option after -gsplit-dwarf.

The new rules:

If a lower debug level -g comes after -gsplit-dwarf, in some cases
-gsplit-dwarf is cancelled.
-gsplit-dwarf -g0 => 0
-gsplit-dwarf -gline-directives-only => DebugDirectivesOnly
-gsplit-dwarf -gmlt -fsplit-dwarf-inlining => 1
-gsplit-dwarf -gmlt -fno-split-dwarf-inlining => 1 + split

// If -gsplit-dwarf comes after -g options, the net effect is 2 + split
-g* -gsplit-dwarf => 2 + split

Some context:

In gcc, the last of -gsplit-dwarf -g0 -g1 -g2 -g3 -ggdb[0-3] -gdwarf-*
... decides the debug info level (-gsplit-dwarf -gdwarf-* have level 2).
It is a bit unfortunate that -gsplit-dwarf -gdwarf-* ... participate in
the level computation but that is the status quo.

[MaskRay]

update description

[MaskRay]

dedent

[MaskRay]

Update description

[probinson]

Commentary questions.

lib/Driver/ToolChains/Clang.cpp

3166 ↗

(On Diff #192589)

Regarding But -gsplit-dwarf is not a g_Group option, hence we have to check the order explicitly. It looks like when you added OPT_gsplit_dwarf to the if-statement above, we no longer have to check it explicitly?

3167 ↗

(On Diff #192589)

Regarding If -gsplit-dwarf wins, we fix DebugInfoKind later.
Is that still true? It looks like -gsplit-dwarf is handled here now, because you removed the compensating code from lines 3268-3269.

3253 ↗

(On Diff #192589)

This block of code no longer turns on -g?

[MaskRay]

Simplify comments

[MaskRay]

lib/Driver/ToolChains/Clang.cpp

3167 ↗

(On Diff #192589)

Deleted.

[probinson]

LGTM. I wish it had occurred to me to pass both OPT_g_Group and OPT_gsplit_dwarf to the same getLastArgs call in the first place.

[dblaikie]

What's the general motivation for this work/changes?

-gmlt -gsplit-dwarf -fno-split-dwarf-inlining => special: 1 (before) 2 (after)

This ^ is the only semantic change due to this refactoring?

There's a desire to be able to compose gmlt+gsplit-dwarf, /if/ -fno-split-dwarf-inlining is enabled. (for context, -fno-split-dwarf-inlining is the default in Google's optimized builds, since split-dwarf-inlining was never implemented in GCC & we didn't want to regress object size when switching from GCC to Clang (& no one had complained about that missing functionality))

So with -fno-split-dwarf-inlining, there's value in using gmlt with gsplit-dwarf (it reduces the size of the .dwo files - reducing the inputs/size of dwp, etc).

& it sounds like this change breaks that scenario?

[MaskRay]

In D59923#1446508, @dblaikie wrote:

What's the general motivation for this work/changes?

The current -gsplit-dwarf handling is a bit complex and the motivation is to make it less confusing.

-g0 -gsplit-dwarf => 2
-gmlt -gsplit-dwarf -fsplit-dwarf-inlining => 2
-gmlt -gsplit-dwarf -fno-split-dwarf-inlining => 1 (before) 2 (after)

It is confusing because the composition of -gmlt -gsplit-dwarf is different from -g0 -gsplit-dwarf when SplitDWARFInlining is false.

-gmlt -gsplit-dwarf -fno-split-dwarf-inlining => special: 1 (before) 2 (after)

This ^ is the only semantic change due to this refactoring?

This is the only semantic change.

There's a desire to be able to compose gmlt+gsplit-dwarf, /if/ -fno-split-dwarf-inlining is enabled. (for context, -fno-split-dwarf-inlining is the default in Google's optimized builds, since split-dwarf-inlining was never implemented in GCC & we didn't want to regress object size when switching from GCC to Clang (& no one had complained about that missing functionality))

So with -fno-split-dwarf-inlining, there's value in using gmlt with gsplit-dwarf (it reduces the size of the .dwo files - reducing the inputs/size of dwp, etc).

& it sounds like this change breaks that scenario?

Acknowledged the desire to compose -gsplit-dwarf -gmlt -fno-split-dwarf-inlining. After this patch, users should place -gmplt after -gsplit-dwarf.

In Bazel, the order of the command line options from left to right: feature flags < BUILD copts= < --copt=. So for targets specifying -g0/-gmlt in their copts / explict --copts= options, they will override -gsplit-dwarf added by the debug feature.

[MaskRay]

Handle -gsplit-dwarf=

[dblaikie]

In D59923#1447198, @MaskRay wrote:

In D59923#1446508, @dblaikie wrote:

What's the general motivation for this work/changes?

The current -gsplit-dwarf handling is a bit complex and the motivation is to make it less confusing.

-g0 -gsplit-dwarf => 2
-gmlt -gsplit-dwarf -fsplit-dwarf-inlining => 2
-gmlt -gsplit-dwarf -fno-split-dwarf-inlining => 1 (before) 2 (after)

It is confusing because the composition of -gmlt -gsplit-dwarf is different from -g0 -gsplit-dwarf when SplitDWARFInlining is false.

-gmlt -gsplit-dwarf -fno-split-dwarf-inlining => special: 1 (before) 2 (after)

This ^ is the only semantic change due to this refactoring?

This is the only semantic change.

There's a desire to be able to compose gmlt+gsplit-dwarf, /if/ -fno-split-dwarf-inlining is enabled. (for context, -fno-split-dwarf-inlining is the default in Google's optimized builds, since split-dwarf-inlining was never implemented in GCC & we didn't want to regress object size when switching from GCC to Clang (& no one had complained about that missing functionality))

So with -fno-split-dwarf-inlining, there's value in using gmlt with gsplit-dwarf (it reduces the size of the .dwo files - reducing the inputs/size of dwp, etc).

& it sounds like this change breaks that scenario?

Acknowledged the desire to compose -gsplit-dwarf -gmlt -fno-split-dwarf-inlining. After this patch, users should place -gmplt after -gsplit-dwarf.

In Bazel, the order of the command line options from left to right: feature flags < BUILD copts= < --copt=. So for targets specifying -g0/-gmlt in their copts / explict --copts= options, they will override -gsplit-dwarf added by the debug feature.

OK - could you include/describe which sets of options disable split-dwarf, then? (adding that to the patch description along with the matrix of g1/2, etc? (specifically, I'd expect "-gmlt -gsplit-dwarf" means 2+split, "-gsplit-dwarf -gmlt" means 1+non-split, and "-fno-split-dwarf-inlining -gsplit-dwarf -gmlt" (with -fno-split-dwarf-inlining anywhere in the command line (so long as it's after an -fsplit-dwarf-inlining) is 1+split)

I'm still not quite sure changing the meaning of "-gmlt -gsplit-dwarf -fno-split-dwarf-inlining" is important. It feels to me like in that mode, -gmlt and -gsplit-dwarf compose naturally in either order. Is it code complexity or user interface complexity you're trying to address? I'm still a bit curious/trying to better understand the motivation here.

[MaskRay]

In D59923#1447245, @dblaikie wrote:

OK - could you include/describe which sets of options disable split-dwarf, then? (adding that to the patch description along with the matrix of g1/2, etc?

I've already updated the description to mention the computed debug info level for some composition: (didn't mention -gline-directives-only though)

-g0, -gline-directives-only, and -gmlt -fsplit-dwarf-inling will disable -gsplit-dwarf. The rule hasn't changed.

(specifically, I'd expect "-gmlt -gsplit-dwarf" means 2+split, "-gsplit-dwarf -gmlt" means 1+non-split, and "-fno-split-dwarf-inlining -gsplit-dwarf -gmlt" (with -fno-split-dwarf-inlining anywhere in the command line (so long as it's after an -fsplit-dwarf-inlining) is 1+split)

The following two composition rules (as you expect) haven't changed:

• -gmlt -gsplit-dwarf -> 2 + split
• -fno-split-dwarf-inlining -gsplit-dwarf -gmlt -> 1 + split

I'm still not quite sure changing the meaning of "-gmlt -gsplit-dwarf -fno-split-dwarf-inlining" is important. It feels to me like in that mode, -gmlt and -gsplit-dwarf compose naturally in either order. Is it code complexity or user interface complexity you're trying to address? I'm still a bit curious/trying to better understand the motivation here.

Just the insistent handling of -g0 -gsplit-dwarf -gmlt -gsplit-dwarf -fsplit-dwarf-inlining and -gmlt -gsplit-dwarf -fno-split-dwarf-inlining motivated me to create this patch. I don't have a specific use case.

The new logic is equivalent to the following patch. It reorders code to make it tighter, thoguh.

        if (DwarfFission != DwarfFissionKind::None) {
          if (A->getIndex() > SplitDWARFArg->getIndex()) {
            if (DebugInfoKind == codegenoptions::NoDebugInfo ||
                DebugInfoKind == codegenoptions::DebugDirectivesOnly ||
                (DebugInfoKind == codegenoptions::DebugLineTablesOnly &&
                 SplitDWARFInlining))
              DwarfFission = DwarfFissionKind::None;
          }
-         else if (SplitDWARFInlining)
-           DebugInfoKind = codegenoptions::NoDebugInfo;
        }

[MaskRay]

Update description

[MaskRay]

Ping :)

[dblaikie]

I'm still not entirely clear on how this is all changing, sorry - in the patch description summary, the first block of examples doesn't mention which of those disables split DWARF (the second block of examples all say "+ split" - is that to imply that the first block all do not use split DWARF?

In a previous message I think you said that the only change was "-gmlt -gsplit-dwarf -fno-split-dwarf-inlining => 1 (before) 2 (after)" - which I'm not sure is an improvement. You mentioned that the inconsistency between "-g0 -gsplit-dwarf" and "-gmlt -gsplit-dwarf -fno-split-dwarf-inlining" was confusing. But still there will be an inconsistency between "-gsplit-dwarf -g0" and "-gsplit-dwarf -gmlt -fno-split-dwarf-inlining", yes?

I think that under -fno-split-dwarf-inlining, -gmlt and -gsplit-dwarf should be order independent and compositional rather than overriding. Having them compose in one order but not the other seems confusing to me.

[MaskRay]

is that to imply that the first block all do not use split DWARF?

The first block do not use split DWARF.

In a previous message I think you said that the only change was "-gmlt -gsplit-dwarf -fno-split-dwarf-inlining => 1 (before) 2 (after)" - which I'm not sure is an improvement.

Yes, this is the only behavioral change.

You mentioned that the inconsistency between "-g0 -gsplit-dwarf" and "-gmlt -gsplit-dwarf -fno-split-dwarf-inlining" was confusing. But still there will be an inconsistency between "-gsplit-dwarf -g0" and "-gsplit-dwarf -gmlt -fno-split-dwarf-inlining", yes?

The debug info level will be consistent after this patch: the last of -gsplit-dwarf -g0 -g1 -g2 -g3 -ggdb[0-3] -gdwarf-* will decide the debug info level (-gsplit-dwarf -gdwarf-* have level 2). Next, a separate rule decides if the -gsplit-dwarf takes effect (not if DebugInfoKind == codegenoptions::NoDebugInfo || DebugInfoKind == codegenoptions::DebugDirectivesOnly || (DebugInfoKind == codegenoptions::DebugLineTablesOnly && SplitDWARFInlining))

I think that under -fno-split-dwarf-inlining, -gmlt and -gsplit-dwarf should be order independent and compositional rather than overriding. Having them compose in one order but not the other seems confusing to me.

The existence of -fno-split-dwarf-inlining changing the position dependence makes me confused:

• Without it, the latter of -gmlt and -gsplit-dwarf decides the debug info level
• With it, -gmlt decides the debug info level

I think the order of different options relative to one another should not matter, unless the options are documented as mutually-exclusive and such an option is documented to override any incompatible options preceding it. (POSIX Utility Syntax Guideline 11) Unfortunately GCC -gsplit-dwarf disobeys the guideline and makes the semantics not orthogonal. Considering the need to preserve its semantics, I think it'd be better to make -gsplit-dwarf consistently positional dependent, rather than make it sometimes positional dependent and sometimes (in the presence of -fno-split-dwarf-inlining) positional independent.

[dblaikie]

In D59923#1460696, @MaskRay wrote:

is that to imply that the first block all do not use split DWARF?

The first block do not use split DWARF.

That doesn't sound like what I'd expect (& would represent a change in behavior as well). The first block reads:

-gsplit-dwarf -g0 => 0
-gsplit-dwarf -gline-directives-only => DebugDirectivesOnly
-gsplit-dwarf -gmlt -fsplit-dwarf-inlining => 1
-gsplit-dwarf -gmlt -fno-split-dwarf-inlining => 1

This last one currently produces split-dwarf (if there's any DWARF worth splitting - if there are any subprogram descriptions, etc, otherwise it saves the indirection and produces an empty .dwo file).

In a previous message I think you said that the only change was "-gmlt -gsplit-dwarf -fno-split-dwarf-inlining => 1 (before) 2 (after)" - which I'm not sure is an improvement.

Yes, this is the only behavioral change.

You mentioned that the inconsistency between "-g0 -gsplit-dwarf" and "-gmlt -gsplit-dwarf -fno-split-dwarf-inlining" was confusing. But still there will be an inconsistency between "-gsplit-dwarf -g0" and "-gsplit-dwarf -gmlt -fno-split-dwarf-inlining", yes?

The debug info level will be consistent after this patch: the last of -gsplit-dwarf -g0 -g1 -g2 -g3 -ggdb[0-3] -gdwarf-* will decide the debug info level (-gsplit-dwarf -gdwarf-* have level 2). Next, a separate rule decides if the -gsplit-dwarf takes effect (not if DebugInfoKind == codegenoptions::NoDebugInfo || DebugInfoKind == codegenoptions::DebugDirectivesOnly || (DebugInfoKind == codegenoptions::DebugLineTablesOnly && SplitDWARFInlining))

I think that under -fno-split-dwarf-inlining, -gmlt and -gsplit-dwarf should be order independent and compositional rather than overriding. Having them compose in one order but not the other seems confusing to me.

The existence of -fno-split-dwarf-inlining changing the position dependence makes me confused:

• Without it, the latter of -gmlt and -gsplit-dwarf decides the debug info level
• With it, -gmlt decides the debug info level

Seems there's going to be confusion either way, though - either the presence/absence of -fno-split-dwarf-inlining changes whether -gsplit-dwarf is respected/ignored (in the presence of -gmlt), or changes whethe -gmlt composes with -gsplit-dwarf or overrides it? Seems these are both a bit confusing, no?

[MaskRay]

Update the description

[MaskRay]

-gsplit-dwarf -gmlt -fno-split-dwarf-inlining => 1

This last one currently produces split-dwarf (if there's any DWARF worth splitting - if there are any subprogram descriptions, etc, otherwise it saves the indirection and produces an empty .dwo file).

Thanks for catching this. It should be 1 + split instead of 1. I've updated the description. This patch doesn't change the behavior of this rule.

Seems there's going to be confusion either way, though - either the presence/absence of -fno-split-dwarf-inlining changes whether -gsplit-dwarf is respected/ignored (in the presence of -gmlt), or changes whethe -gmlt composes with -gsplit-dwarf or overrides it? Seems these are both a bit confusing, no?

See the updated description. The lower half of the rules can be summaries as -g* -gsplit-dwarf => 2 + split with this patch. I think that is how this patch makes the rules less confusing.

[MaskRay]

Update description

[MaskRay]

Ping :)

[dblaikie]

Looks ok - thanks!

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

static void RenderDebugOptions(const ToolChain &TC, const Driver &D,
   DwarfFission = getDebugFissionKind(D, Args, SplitDWARFArg);
 
   if (DwarfFission != DwarfFissionKind::None &&
       !checkDebugInfoOption(SplitDWARFArg, Args, D, TC)) {
     DwarfFission = DwarfFissionKind::None;
     SplitDWARFInlining = false;
   }
 
-  if (const Arg *A = Args.getLastArg(options::OPT_g_Group)) {
-    if (checkDebugInfoOption(A, Args, D, TC)) {
-      // If the last option explicitly specified a debug-info level, use it.
-      if (A->getOption().matches(options::OPT_gN_Group)) {
-        DebugInfoKind = DebugLevelToInfoKind(*A);
-        // If you say "-gsplit-dwarf -gline-tables-only", -gsplit-dwarf loses.
-        // But -gsplit-dwarf is not a g_group option, hence we have to check the
-        // order explicitly. If -gsplit-dwarf wins, we fix DebugInfoKind later.
-        // 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. And if you just turned off debug info,
-        // (-gsplit-dwarf -g0) - do that.
-        if (DwarfFission != DwarfFissionKind::None) {
-          if (A->getIndex() > SplitDWARFArg->getIndex()) {
-            if (DebugInfoKind == codegenoptions::NoDebugInfo ||
-                DebugInfoKind == codegenoptions::DebugDirectivesOnly ||
-                (DebugInfoKind == codegenoptions::DebugLineTablesOnly &&
-                 SplitDWARFInlining))
-              DwarfFission = DwarfFissionKind::None;
-          } else if (SplitDWARFInlining)
-            DebugInfoKind = codegenoptions::NoDebugInfo;
-        }
-      } else {
-        // For any other 'g' option, use Limited.
-        DebugInfoKind = codegenoptions::LimitedDebugInfo;
-      }
-    } else {
-      DebugInfoKind = codegenoptions::LimitedDebugInfo;
+  if (const Arg *A =
+          Args.getLastArg(options::OPT_g_Group, options::OPT_gsplit_dwarf,
+                          options::OPT_gsplit_dwarf_EQ)) {
+    DebugInfoKind = codegenoptions::LimitedDebugInfo;
+
+    if (checkDebugInfoOption(A, Args, D, TC) &&
+        A->getOption().matches(options::OPT_gN_Group)) {
+      // 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.
     }
   }
 
   // If a debugger tuning argument appeared, remember it.
   if (const Arg *A =
           Args.getLastArg(options::OPT_gTune_Group, options::OPT_ggdbN_Group)) {
     if (checkDebugInfoOption(A, Args, D, TC)) {
       if (A->getOption().matches(options::OPT_glldb))
if (checkDebugInfoOption(A, Args, D, TC)) {
       if (DebugInfoKind != codegenoptions::DebugLineTablesOnly &&
           DebugInfoKind != codegenoptions::DebugDirectivesOnly) {
         DebugInfoKind = codegenoptions::LimitedDebugInfo;
         CmdArgs.push_back("-dwarf-ext-refs");
         CmdArgs.push_back("-fmodule-format=obj");
       }
     }
 
-  // -gsplit-dwarf should turn on -g and enable the backend dwarf
-  // splitting and extraction.
+  // -gsplit-dwarf enables the backend dwarf splitting and extraction.
   if (T.isOSBinFormatELF()) {
     if (!SplitDWARFInlining)
       CmdArgs.push_back("-fno-split-dwarf-inlining");
 
     if (DwarfFission != DwarfFissionKind::None) {
-      if (DebugInfoKind == codegenoptions::NoDebugInfo)
-        DebugInfoKind = codegenoptions::LimitedDebugInfo;
-
       if (DwarfFission == DwarfFissionKind::Single)
         CmdArgs.push_back("-enable-split-dwarf=single");
       else
         CmdArgs.push_back("-enable-split-dwarf");
     }
   }
 
   // After we've dealt with all combinations of things that could

cfe/trunk/test/Driver/split-debug.c

 // CHECK-NOINLINE-WITHOUT-SPLIT-NOT: "-enable-split-dwarf"
 // CHECK-NOINLINE-WITHOUT-SPLIT: "-fno-split-dwarf-inlining"
 // CHECK-NOINLINE-WITHOUT-SPLIT: "-debug-info-kind=limited"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gmlt -gsplit-dwarf -fno-split-dwarf-inlining -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SPLIT-WITH-GMLT < %t %s
 //
 // CHECK-SPLIT-WITH-GMLT: "-enable-split-dwarf"
-// CHECK-SPLIT-WITH-GMLT: "-debug-info-kind=line-tables-only"
+// CHECK-SPLIT-WITH-GMLT: "-debug-info-kind=limited"
 // CHECK-SPLIT-WITH-GMLT: "-split-dwarf-file"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -fno-split-dwarf-inlining -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SPLIT-WITH-NOINL < %t %s
 //
 // CHECK-SPLIT-WITH-NOINL: "-enable-split-dwarf"
 // CHECK-SPLIT-WITH-NOINL: "-debug-info-kind=limited"
 // CHECK-SPLIT-WITH-NOINL: "-split-dwarf-file"

 // RUN: FileCheck -check-prefix=CHECK-G0-OVER-SPLIT < %t %s
 //
 // CHECK-G0-OVER-SPLIT-NOT: "-enable-split-dwarf"
 // CHECK-G0-OVER-SPLIT-NOT: "-debug-info-kind
 // CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-file"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -g0 -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-G0-OVER-SPLIT < %t %s
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=split -g0 -S -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-G0-OVER-SPLIT < %t %s
 //
 // CHECK-G0-OVER-SPLIT-NOT: "-enable-split-dwarf"
 // CHECK-G0-OVER-SPLIT-NOT: "-debug-info-kind
 // CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-file"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -g0 -gsplit-dwarf -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SPLIT-OVER-G0 < %t %s
+// RUN: %clang -target x86_64-unknown-linux-gnu -g0 -gsplit-dwarf=split -S -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-SPLIT-OVER-G0 < %t %s
 //
 // CHECK-SPLIT-OVER-G0: "-enable-split-dwarf" "-debug-info-kind=limited"
 // CHECK-SPLIT-OVER-G0: "-split-dwarf-file"