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.
Details
Diff Detail
Event Timeline
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3246–3248 | 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)) | |
3247–3248 | 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). | |
6178–6180 | 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? |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3246–3248 | Oh, there's also clang/tools/clang-format/git-clang-format for formatting anything in a git revision range. |
clang/test/CodeGen/debug-default-version.c | ||
---|---|---|
2 | This looks like a clangDriver change. Maybe move the test to test/Driver ? |
clang/test/CodeGen/debug-default-version.c | ||
---|---|---|
3 | VER3 -> DWARF3 may be better, because you also check another debug info format codeview here. |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3247–3248 | 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?
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.
clang/include/clang/Driver/Options.td | ||
---|---|---|
2002 | Unexpected blank line here. | |
clang/lib/Driver/ToolChains/Clang.cpp | ||
3237 | 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 | ||
2 | +1. It should be sufficient to use -### and check what gets passed as cc1 options. | |
24 | 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?
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.
"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 | 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. |
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.
clang/include/clang/Driver/Options.td | ||
---|---|---|
1955 | 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 | ||
3148 | 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). | |
3250 | After setting DWARFVersion above, when nothing was requested, this looks peculiar. Is DWARFVersion == 0 a proxy for -gcodeview ? | |
6174 | 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? |
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.
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3250 | Yep, DWARFVersion shouldn't be set when none was requested - see discussion above between myself & @cmtice - that should leave this code fine/correct. | |
6174 | 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))? |
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...
Hmmm...I'm having upload issues.
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3250 | 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. | |
6174 | 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 |
clang/include/clang/Driver/Options.td | ||
---|---|---|
1955 | Can we haggle over this a bit? My thinking behind -fdebug-default-version was consistency with other DWARF related flags: We do have some -fdwarf: So I'm personally inclined to sticking with -fdebug* as being all DWARF related/consistent there. Thoughts? | |
clang/lib/Driver/ToolChains/Clang.cpp | ||
3233 | 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) | |
6174 | 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: (& 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.
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3237 | 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"); } | |
6172 | 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? |
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.
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 | ||
---|---|---|
1955 | Probably should have HelpText, maybe something like this: | |
clang/lib/Driver/ToolChains/Clang.cpp | ||
6173 | 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. | |
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. |
10 ↗ | (On Diff #227942) | indirectly |
23 ↗ | (On Diff #227942) | s/of/or/ |
clang/include/clang/Driver/Options.td | ||
---|---|---|
1955 | If the HelpText mentions DWARF, I'm okay with an -fdebug prefix. |
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).
Rename flag to -fdebug-default-version; add help text, mentioning DWARF. Update tests to use -### and add assembler tests.
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 | ||
---|---|---|
9–17 ↗ | (On Diff #227977) | 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) |
clang/test/Driver/debug-default-version.c | ||
---|---|---|
9–17 ↗ | (On Diff #227977) | We may need -Werror in some or all of the tests. |
clang/test/Driver/debug-default-version.c | ||
---|---|---|
66 ↗ | (On Diff #227977) | This is not correct. I suppose this to be "-dwarf-version=. The pattern "-dwarf-version=" does not match "-dwarf-version=5" |
clang/test/Driver/debug-default-version.c | ||
---|---|---|
37 ↗ | (On Diff #228115) | 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) |
LG after // NOCODEVIEW-NOT: "-gcodeview" and // NODWARF4-NOT: "-dwarf-version=4" are fixed.
Sigh, one last typo. I'm happy otherwise.
clang/test/Driver/debug-default-version.c | ||
---|---|---|
11 ↗ | (On Diff #228144) | s/of you/or you/ |
If this is specifically the default DWARF version, I think the word "dwarf" ought to be in the option name.