Page MenuHomePhabricator

[clang] Add new -fdebug-default-version flag.
ClosedPublic

Authored by cmtice on Mon, Nov 4, 1:14 PM.

Details

Summary

Want to decouple setting the DWARF version from enabling/disabling generation of debug info. This adds a new flag '-fdebug-default-version=N' that sets the version of DWARF to be generated, but does not turn on/off debug info generation. In particular this flag can be added to existing compilation commands to clarify which version of DWARF '-g' or '-gdwarf' will use, without overriding existing -gdwarf-N flags.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
dblaikie added inline comments.Mon, Nov 4, 1:28 PM
clang/lib/Driver/ToolChains/Clang.cpp
3436–3437

Looks like this should be on a single line to conform to LLVM convention (though might just be phabricator doing something weird)

If you can run clang-format over the change (not over the whole file) it should fix up issues like this. (there's various clang-format editor integrations - there's some google-internal documentation at go/clang-format that'll explain how to setup an auto-save hook that'll clang-format the changed lines so all your C++ code in the LLVM repository conforms to LLVM's coding conventions (well, those that can be expressed by clang-format))

3437–3438

Hmm, actually - why is this case necessary/what does it cover? I was hoping the case you added inside the "if (EmitDwarf)" case above would be all that was required (& the call to ParseDebugDefaultVersion would go inside there at the use, to reduce the variable scope/keep the code closer together).

6371–6373

Some clang-formatting required, but also could probably be written as:

if (DwarfVersion == 0)
  DwarfVersion = ParseDebugDefaultVersion(...);

if (DwarfVersion == 0)
  DwarfVersion = getToolChain().GetDefaultDwarfVersion();

To make these more symmetric?

dblaikie added inline comments.Mon, Nov 4, 1:37 PM
clang/lib/Driver/ToolChains/Clang.cpp
3436–3437

Oh, there's also clang/tools/clang-format/git-clang-format for formatting anything in a git revision range.

cmtice marked 3 inline comments as done.Mon, Nov 4, 1:39 PM
cmtice added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
3436–3437

Ok, will do.

3437–3438

This covers the case where you are setting the default dwarf version without actually turning on/off debug info at all (there's no -gdwarf or -g option, so EmitDwarf is false).

6371–6373

Ok will do.

MaskRay added a subscriber: MaskRay.Mon, Nov 4, 1:52 PM
MaskRay added inline comments.
clang/test/CodeGen/debug-default-version.c
1 ↗(On Diff #227768)

This looks like a clangDriver change. Maybe move the test to test/Driver ?

MaskRay added inline comments.Mon, Nov 4, 1:54 PM
clang/test/CodeGen/debug-default-version.c
2 ↗(On Diff #227768)

VER3 -> DWARF3 may be better, because you also check another debug info format codeview here.

cmtice updated this revision to Diff 227777.Mon, Nov 4, 1:54 PM

Made requested formatting changes.

dblaikie added inline comments.Mon, Nov 4, 2:07 PM
clang/lib/Driver/ToolChains/Clang.cpp
3437–3438

Ah, perhaps I'm missing something - why should the DWARFVersion be set if EmitDwarf is false? No DWARF was requested, so the DWARFVersion probably shouldn't be set, I think?

Want to decouple setting the DWARF version from enabling/disabling generation of debug info.

Um, why?

Want to decouple setting the DWARF version from enabling/disabling generation of debug info.

Um, why?

If you've got a big build system with various users opting in/out of DWARF and you want to migrate to DWARFv5, say, but you can't add "-gdwarf-5" to your build system, because that'd turn on debug info in cases that don't need it - but it's easier to change the default than to modify all cases that enable dwarf across the codebase.

Open to discussion about whether this is a good/bad idea - but it'd be useful for Google at least & seemed low-cost enough to go this route.

probinson added inline comments.Mon, Nov 4, 2:42 PM
clang/include/clang/Driver/Options.td
2009

Unexpected blank line here.

clang/lib/Driver/ToolChains/Clang.cpp
3427

precedence

clang/lib/Driver/ToolChains/CommonArgs.cpp
1151

Clang doesn't support DWARF v1. I expect nobody does at this point (DWARF 2 came out in 1993).

clang/test/CodeGen/debug-default-version.c
1 ↗(On Diff #227768)

+1. It should be sufficient to use -### and check what gets passed as cc1 options.

24 ↗(On Diff #227777)

Does that actually work? Last I checked, DWARF and COFF didn't play nicely, but I admit that was quite a while ago.

Since this sounds like it is hidden inside of some other tooling — is passing the frontend option -dwarf-version=5 not an option?

Want to decouple setting the DWARF version from enabling/disabling generation of debug info.

Um, why?

If you've got a big build system with various users opting in/out of DWARF and you want to migrate to DWARFv5, say, but you can't add "-gdwarf-5" to your build system, because that'd turn on debug info in cases that don't need it - but it's easier to change the default than to modify all cases that enable dwarf across the codebase.

Open to discussion about whether this is a good/bad idea - but it'd be useful for Google at least & seemed low-cost enough to go this route.

Because you want the default to be based on your corporate environment rather than the target platform. The maze of twisty -g passages gets a new secret door. Oh well.

Since this sounds like it is hidden inside of some other tooling — is passing the frontend option -dwarf-version=5 not an option?

"hidden inside of some other tooling" - in the same sense as most build flags are passed by a build system, yes. But generally we (Google specifically, all of us LLVM/Clang users in general) try not to use implementation details like that.

clang/test/CodeGen/debug-default-version.c
24 ↗(On Diff #227777)

Existing test cases cover scenarios like this (see clang/test/CodeGen/dwarf-version.c), so this seems consistent to preserve that sort of functionality, however well it currently works.

Want to decouple setting the DWARF version from enabling/disabling generation of debug info.

Um, why?

If you've got a big build system with various users opting in/out of DWARF and you want to migrate to DWARFv5, say, but you can't add "-gdwarf-5" to your build system, because that'd turn on debug info in cases that don't need it - but it's easier to change the default than to modify all cases that enable dwarf across the codebase.

Open to discussion about whether this is a good/bad idea - but it'd be useful for Google at least & seemed low-cost enough to go this route.

Because you want the default to be based on your corporate environment rather than the target platform.

Sure - if we know our users have access to a modern debugger, but we don't own a whole platform. You could imagine someone shipping clang+debugger in a 3rd party/non-platform IDE might want something like this too/similar to the Google situation. We can't dictate what's suitable for all of Linux, but for the subset of users that are using a specific toolchain/situation, we can.

The maze of twisty -g passages gets a new secret door. Oh well.

If you find this to be a significant complication, I'd really like to discuss it further - I know there are some twisty things (one of the worst I created when it comes to -fsplit-dwarf-inlining, -gsplit-dwarf, -gmlt combinations) - but this seems pretty simple/tidy/orthogonal to me.

probinson added inline comments.Mon, Nov 4, 3:11 PM
clang/include/clang/Driver/Options.td
1961

If this is specifically the default DWARF version, I think the word "dwarf" ought to be in the option name.

clang/lib/Driver/ToolChains/Clang.cpp
3338

I would rather see all the DWARF version weirdness all in one place (i.e., please move this down to the rest of it) particularly given that the "default" DWARF version now means two different things (something from the command line, and something based on the toolchain).

3439

After setting DWARFVersion above, when nothing was requested, this looks peculiar. Is DWARFVersion == 0 a proxy for -gcodeview ?

6366

I have to say, this sequence is a whole lot easier to follow than what's up in RenderDebugOptions. It would be nice if they were both so easy to understand.

Although it makes me wonder, does -fdebug-default-version go unclaimed here if there's a -gdwarf-2 on the command line?

The maze of twisty -g passages gets a new secret door. Oh well.

If you find this to be a significant complication, I'd really like to discuss it further - I know there are some twisty things (one of the worst I created when it comes to -fsplit-dwarf-inlining, -gsplit-dwarf, -gmlt combinations) - but this seems pretty simple/tidy/orthogonal to me.

No, no, if we can at least keep the twistiness in one place (or two I suppose, one for C/C++ and one for asm) it's not a real problem.

Maybe a strategic helper function could be useful to make those two places more obviously the same.

cmtice edited the summary of this revision. (Show Details)Mon, Nov 4, 3:20 PM
dblaikie added inline comments.Mon, Nov 4, 3:39 PM
clang/lib/Driver/ToolChains/Clang.cpp
3439

Yep, DWARFVersion shouldn't be set when none was requested - see discussion above between myself & @cmtice - that should leave this code fine/correct.

6366

Once the "else if (DefaultDWARFVersion != 0)" clause is removed, and the ParseDebugDefaultVersion call is sunk down into the "if (EmitDwarf)" case - hopefully that'll simplify the RenderDebugOptions code enough to be reasonable?

I think the complication there is that "-g" can imply CodeView if nothing explicitly requests DWARF & the target is windows, whereas there's no support for that kind of CV fallback here? So some of that might be inherent.

& agreed - worth testing that -fdebug-default-version doesn't get a -Wunused warning if someone says "-fdebug-default-version 4 -gdwarf-2". @cmtice is that tested? You might be able to add -Werror to the existing test case's RUN lines to demonstrate that no such warnings are produced?

clang/lib/Driver/ToolChains/CommonArgs.cpp
1151

You'd recommend/request moving this bound up to 2 (to be consistent with the fact that clang supports -gdwarf-2 technically (even though we probably don't produce fully conformant DWARFv2 these days, I would guess))?

probinson added inline comments.Tue, Nov 5, 7:11 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
1151

It wants to permit the exact same range as the -gdwarf-N options.

clang/test/CodeGen/debug-default-version.c
24 ↗(On Diff #227777)

Ok.

cmtice updated this revision to Diff 227910.Tue, Nov 5, 10:05 AM
cmtice marked 9 inline comments as done.

Made requested changes:

  • renamed option to be dwarf-specific
  • fixed spelling & blank line issues
  • only set version if emit-dwarf is true
  • move test to Driver directory

I *think* I got it all done...

Made requested changes:

  • renamed option to be dwarf-specific
  • fixed spelling & blank line issues
  • only set version if emit-dwarf is true
  • move test to Driver directory

    I *think* I got it all done...

It seems you uploaded the wrong diff, probably the original one...

cmtice updated this revision to Diff 227940.Tue, Nov 5, 11:46 AM

Try uploading correct diff file?

Hmmm...I'm having upload issues.

clang/lib/Driver/ToolChains/Clang.cpp
3439

DWARFVersion is (will be) only set when the debug info kind is DWARF, i.e. when EmitDwarf is True. I apologize for the confusion -- David Blaikie and I has some misunderstandings about the original implementation intent. DWARFVersion == 0 ought only to result in codeview being generated/used if the user specified -g and codeview is the default debug format for the platform.

6366

The thought is that if the user actually chose a specific dwarf version, e.g. -gdwarf-2, then that value should override the general default, specified with -fdebug-default-version (soon to be
-fdwarf-default-version).

cmtice updated this revision to Diff 227942.Tue, Nov 5, 11:49 AM

re-try upload again.

Ok, I think the upload was correct this time.

dblaikie added inline comments.Tue, Nov 5, 12:16 PM
clang/include/clang/Driver/Options.td
1961

Can we haggle over this a bit?

My thinking behind -fdebug-default-version was consistency with other DWARF related flags:
-fdebug-compilation-dir
-fdebug-info-for-profiling
-fdebug-macro
-fdebug-types-section
-fdebug-ranges-base-address
-fdebug-prefix-map

We do have some -fdwarf:
-fdwarf2-cfi-asm
-fdwarf-directory-asm
-fdwarf-exceptions

So I'm personally inclined to sticking with -fdebug* as being all DWARF related/consistent there.

Thoughts?

clang/lib/Driver/ToolChains/Clang.cpp
3421

Can you move this down to the narrower scope:

if (unsigned DefaultDWARFVersion = ParseDefaultVersion(TC, Args))
  DWARFVersion = DefaultDWARFVersion;

(but maybe that runs into the -Wunused warning issues discussed in the other thread in this review)

6366

That's the desired behavior, yes (-gdwarf-2 overrides this new flag) - what @probinson is asking about is the behavior of -Wunused.

Clang's built-in flag handling does some work to warn on unused flags - it does this based on which flags are queried by the API.

The risk is that, because ParseDwarfDefaultVersion is only called under the "if (DwarfVersion == 0)" condition, which will be false if the user specified -gdwarf-2, for instance, then the ParseDwarfDefaultVersion won't be called & clang's unused flag handling may see the new flag as "unused" and warn about it.

Basically the question is (could you test this manually & confirm the behavior, and check that the automated tests cover this - which I think they can cover this with minimal effort by adding -Werror to some RUN lines in the tests), does this produce a warning:
clang -gdwarf-2 -fdebug-default-version=5 x.s

(& if it doesn't warn, it might be worth understanding why it doesn't, I guess, given the above hypothesis)

dblaikie: The code, as written, parses the dwarf default version flag, whether or not DWARFVersion is 0, i.e. whether or not a -gdwarf-N flag was passed. It only USES the value if there's no overriding value.

MaskRay added inline comments.Tue, Nov 5, 12:49 PM
clang/lib/Driver/ToolChains/Clang.cpp
3427
if (DefaultDWARFVersion) {
  // If the user specified a default DWARF version, that takes precedence
  // over the platform default.
  DWARFVersion = DefaultDWARFVersion;
} else {
  // Start with the platform default DWARF version.
  DWARFVersion = TC.GetDefaultDwarfVersion();
  assert(DWARFVersion && "toolchain default DWARF version must be nonzero");
}
6364
if (DwarfVersion == 0) {
  DwarfVersion = ParseDwarfDefaultVersion(getToolChain(), Args);
  if (DwarfVersion == 0)
    DwarfVersion = getToolChain().GetDefaultDwarfVersion();
}

This part constructs a -cc1as command line. It is not covered by a test. Can you add one?

MaskRay removed subscribers: probinson, MaskRay.
MaskRay added inline comments.Tue, Nov 5, 12:53 PM
clang/test/Driver/dwarf-default-version.c
1 ↗(On Diff #227942)

-emit-llvm -> -###. @probinson mentioned that we just need to test -cc1 options.

We can use the following to check that codeview is not emitted.

// NOCODEVIEW-NOT: "-gcodeview"

For the record, I double-checked and if we use the flag and don't check for it (i.e. if we move the parsing inside the EmitDwarf is True block) then -Werror does indeed complain about an unused command line argument.

cmtice updated this revision to Diff 227954.Tue, Nov 5, 1:00 PM

Make second call to ParseDwarfDefaultVersion unconditional (to match the first and avoid errors with -Werror).

The test is in the right place, now it needs to behave more like other driver tests. Sorry if it feels like I'm whaling on you, but the driver is a bit of a peculiar beast with an atypical testing mode. Taming it is harder than it looks.

clang/include/clang/Driver/Options.td
1961

Probably should have HelpText, maybe something like this:
"The default DWARF version to use, if a -g option causes DWARF debug info to be produced"
(it's probably not helpful to talk about overriding the target's default version, I think)

clang/lib/Driver/ToolChains/Clang.cpp
6365

This is the path for parsing assembler options (when feeding clang a .s file) and I believe as written, the combination -fdwarf-default-debug=2 -gdwarf-3 will yield an unused-option warning. So I'm pretty sure the test doesn't cover this path.
You can fake an assembler driver test in the current test file by using -x assembler (especially if you use the -### tactic I mention in the comments there), or you can add a second test file with a .s extension.

clang/test/Driver/dwarf-default-version.c
1 ↗(On Diff #227942)

As a rule, driver tests should pass -### which causes the driver to print the command lines instead of running the commands. Then the checks would examine the command lines constructed by the driver, to verify they say what you want.
In this case, you'd look at the emitted -dwarf-version option and make sure it's what you want, and/or look for the option that says to do codeview.
The result is a test that is a more focused specifically on driver behavior, and also runs a bit faster (not starting any subprocesses etc).

10 ↗(On Diff #227942)

indirectly

23 ↗(On Diff #227942)

s/of/or/

probinson added inline comments.Tue, Nov 5, 1:09 PM
clang/include/clang/Driver/Options.td
1961

If the HelpText mentions DWARF, I'm okay with an -fdebug prefix.

probinson added inline comments.Tue, Nov 5, 1:14 PM
clang/lib/Driver/ToolChains/Clang.cpp
6364

Hi @MaskRay , your version would have the unclaimed-option problem.

dblaikie added inline comments.Tue, Nov 5, 2:19 PM
clang/include/clang/Driver/Options.td
1961

Yep, I'd expect/be fine with all these options having something about DWARF in their long-form description.

@cmtice - would you mind switching this back to the original name & adding the relevant help text?

cmtice marked an inline comment as done.Tue, Nov 5, 2:26 PM

Yes, I will change the flag name back to debug-default-version, and add help text mentioning dwarf. I'm in the process of trying to re-do the test cases as required (I'm a bit new to this so it's taking me a bit to figure this out).

cmtice updated this revision to Diff 227977.Tue, Nov 5, 3:51 PM

Rename flag to -fdebug-default-version; add help text, mentioning DWARF. Update tests to use -### and add assembler tests.

dblaikie accepted this revision.Tue, Nov 5, 5:13 PM

Looks good to me - but maybe give it a day to see if anyone else has further thoughts/that you've addressed their feedback too.

clang/test/Driver/debug-default-version.c
10–18

I'd probably skip these tests since overriding the platform default is already tested above (for the linux platform default) and I'm not sure there's much value add by testing that this overrides each platform default separately (given the platform default override is pretty orthogonal to which platform - it'd be pretty hard to introduce a bug that would break for one platform but not for all of them)

This revision is now accepted and ready to land.Tue, Nov 5, 5:13 PM
MaskRay added inline comments.Tue, Nov 5, 5:50 PM
clang/test/Driver/debug-default-version.c
10–18

We may need -Werror in some or all of the tests.

Just a couple of typos, and I'm happy. I agree with the other reviewers on the last needed test tweaks.

clang/include/clang/Driver/Options.td
1961

HelpText does not want a final period.

clang/test/Driver/debug-default-version.c
11

indirectly

MaskRay added inline comments.Wed, Nov 6, 9:29 AM
clang/test/Driver/debug-default-version.c
67

This is not correct. I suppose this to be "-dwarf-version=.

The pattern "-dwarf-version=" does not match "-dwarf-version=5"

cmtice updated this revision to Diff 228115.Wed, Nov 6, 12:01 PM
cmtice marked 3 inline comments as done.

Remove period from help text. Fix tests.

dblaikie added inline comments.Wed, Nov 6, 12:56 PM
clang/test/Driver/debug-default-version.c
38

Same issue as with the dwarf-version below.

It's probably easier to remove the "" (FileCheck arguments aren't quoted - these quotes are treated like any other character/as part of the match - so omitting them means FileCheck can match -debug-info-kind= no matter what comes before/after it, rather than specifically looking for a " immediately after the '='). (you could remove all the " if you like, I don't think they add a lot to the tests here)

cmtice updated this revision to Diff 228130.Wed, Nov 6, 1:20 PM

Fix NODEBUGINFO-NOT test.

MaskRay accepted this revision.Wed, Nov 6, 1:40 PM

LG after // NOCODEVIEW-NOT: "-gcodeview" and // NODWARF4-NOT: "-dwarf-version=4" are fixed.

cmtice updated this revision to Diff 228144.Wed, Nov 6, 2:18 PM

Remove quotes from -NOT tests.

Sigh, one last typo. I'm happy otherwise.

clang/test/Driver/debug-default-version.c
12

s/of you/or you/

cmtice updated this revision to Diff 228250.Thu, Nov 7, 7:52 AM

Fix small typo in comments.

This revision was automatically updated to reflect the committed changes.