Page MenuHomePhabricator

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

Authored by MaskRay on Mar 28 2019, 2:32 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Mar 28 2019, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2019, 2:32 AM
MaskRay updated this revision to Diff 192585.Mar 28 2019, 2:35 AM
MaskRay edited the summary of this revision. (Show Details)

update description

MaskRay updated this revision to Diff 192589.Mar 28 2019, 2:46 AM
MaskRay edited the summary of this revision. (Show Details)

Update description

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 updated this revision to Diff 192637.Mar 28 2019, 7:52 AM

Simplify comments

MaskRay marked 4 inline comments as done.Mar 28 2019, 7:54 AM
MaskRay added inline comments.
lib/Driver/ToolChains/Clang.cpp
3167 ↗(On Diff #192589)

Deleted.

probinson accepted this revision.Mar 28 2019, 9:06 AM

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.

This revision is now accepted and ready to land.Mar 28 2019, 9:06 AM
dblaikie requested changes to this revision.Mar 28 2019, 11:22 AM

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?

This revision now requires changes to proceed.Mar 28 2019, 11:22 AM
MaskRay marked an inline comment as done.EditedMar 28 2019, 7:45 PM

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 updated this revision to Diff 192770.Mar 28 2019, 8:02 PM
MaskRay edited the summary of this revision. (Show Details)

Handle -gsplit-dwarf=

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.

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 updated this revision to Diff 192777.Mar 28 2019, 10:04 PM
MaskRay edited the summary of this revision. (Show Details)

Update description

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.

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.

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 updated this revision to Diff 194672.Apr 11 2019, 6:33 AM

Update the description

-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 updated this revision to Diff 194673.Apr 11 2019, 6:37 AM
MaskRay edited the summary of this revision. (Show Details)

Update description

dblaikie accepted this revision.Apr 16 2019, 12:59 PM

Looks ok - thanks!

This revision is now accepted and ready to land.Apr 16 2019, 12:59 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 6:44 PM