Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline
Changeset View
Standalone View
clang/lib/Driver/ToolChains/Clang.cpp
| Show First 20 Lines • Show All 3,139 Lines • ▼ Show 20 Lines | static void RenderDebugOptions(const ToolChain &TC, const Driver &D, | ||||
| // about what to pass from the driver to the frontend, but by the time they | // about what to pass from the driver to the frontend, but by the time they | ||||
| // reach cc1 they've been factored into three well-defined orthogonal choices: | // reach cc1 they've been factored into three well-defined orthogonal choices: | ||||
| // * what level of debug info to generate | // * what level of debug info to generate | ||||
| // * what dwarf version to write | // * what dwarf version to write | ||||
| // * what debugger tuning to use | // * what debugger tuning to use | ||||
| // This avoids having to monkey around further in cc1 other than to disable | // This avoids having to monkey around further in cc1 other than to disable | ||||
| // codeview if not running in a Windows environment. Perhaps even that | // codeview if not running in a Windows environment. Perhaps even that | ||||
| // decision should be made in the driver as well though. | // decision should be made in the driver as well though. | ||||
| unsigned DWARFVersion = 0; | |||||
| llvm::DebuggerKind DebuggerTuning = TC.getDefaultDebuggerTuning(); | llvm::DebuggerKind DebuggerTuning = TC.getDefaultDebuggerTuning(); | ||||
probinson: I would rather see all the DWARF version weirdness all in one place (i.e., please move this… | |||||
| bool SplitDWARFInlining = | bool SplitDWARFInlining = | ||||
| Args.hasFlag(options::OPT_fsplit_dwarf_inlining, | Args.hasFlag(options::OPT_fsplit_dwarf_inlining, | ||||
| options::OPT_fno_split_dwarf_inlining, true); | options::OPT_fno_split_dwarf_inlining, true); | ||||
| Args.ClaimAllArgs(options::OPT_g_Group); | Args.ClaimAllArgs(options::OPT_g_Group); | ||||
| Arg* SplitDWARFArg; | Arg* SplitDWARFArg; | ||||
| ▲ Show 20 Lines • Show All 65 Lines • ▼ Show 20 Lines | case codegenoptions::DIF_CodeView: | ||||
| EmitCodeView = true; | EmitCodeView = true; | ||||
| break; | break; | ||||
| case codegenoptions::DIF_DWARF: | case codegenoptions::DIF_DWARF: | ||||
| EmitDwarf = true; | EmitDwarf = true; | ||||
| break; | break; | ||||
| } | } | ||||
| } | } | ||||
| unsigned DWARFVersion = 0; | |||||
| unsigned DefaultDWARFVersion = ParseDwarfDefaultVersion(TC, Args); | |||||
dblaikieUnsubmitted Not Done ReplyInline ActionsCan 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) dblaikie: Can you move this down to the narrower scope:
if (unsigned DefaultDWARFVersion =… | |||||
| if (EmitDwarf) { | if (EmitDwarf) { | ||||
| // Start with the platform default DWARF version | // Start with the platform default DWARF version | ||||
| DWARFVersion = TC.GetDefaultDwarfVersion(); | DWARFVersion = TC.GetDefaultDwarfVersion(); | ||||
| assert(DWARFVersion && "toolchain default DWARF version must be nonzero"); | assert(DWARFVersion && "toolchain default DWARF version must be nonzero"); | ||||
| // If the user specified a default DWARF version, that takes precedence | |||||
precedence probinson: precedence | |||||
MaskRayUnsubmitted Not Done ReplyInline Actionsif (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"); } MaskRay: lang=cpp
if (DefaultDWARFVersion) {
// If the user specified a default DWARF… | |||||
| // over the platform default. | |||||
| if (DefaultDWARFVersion) | |||||
| DWARFVersion = DefaultDWARFVersion; | |||||
| // Override with a user-specified DWARF version | // Override with a user-specified DWARF version | ||||
| if (GDwarfN) | if (GDwarfN) | ||||
| if (auto ExplicitVersion = DwarfVersionNum(GDwarfN->getSpelling())) | if (auto ExplicitVersion = DwarfVersionNum(GDwarfN->getSpelling())) | ||||
| DWARFVersion = ExplicitVersion; | DWARFVersion = ExplicitVersion; | ||||
| } | } | ||||
Not Done ReplyInline ActionsLooks 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)) dblaikie: Looks like this should be on a single line to conform to LLVM convention (though might just be… | |||||
Not Done ReplyInline ActionsOh, there's also clang/tools/clang-format/git-clang-format for formatting anything in a git revision range. dblaikie: Oh, there's also clang/tools/clang-format/git-clang-format for formatting anything in a git… | |||||
Ok, will do. cmtice: Ok, will do. | |||||
| // -gline-directives-only supported only for the DWARF debug info. | // -gline-directives-only supported only for the DWARF debug info. | ||||
Not Done ReplyInline ActionsHmm, 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). dblaikie: Hmm, actually - why is this case necessary/what does it cover? I was hoping the case you added… | |||||
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). cmtice: This covers the case where you are setting the default dwarf version without actually turning… | |||||
Not Done ReplyInline ActionsAh, 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? dblaikie: Ah, perhaps I'm missing something - why should the DWARFVersion be set if EmitDwarf is false? | |||||
| if (DWARFVersion == 0 && DebugInfoKind == codegenoptions::DebugDirectivesOnly) | if (DWARFVersion == 0 && DebugInfoKind == codegenoptions::DebugDirectivesOnly) | ||||
Not Done ReplyInline ActionsAfter setting DWARFVersion above, when nothing was requested, this looks peculiar. Is DWARFVersion == 0 a proxy for -gcodeview ? probinson: After setting DWARFVersion above, when nothing was requested, this looks peculiar. Is… | |||||
Not Done ReplyInline ActionsYep, DWARFVersion shouldn't be set when none was requested - see discussion above between myself & @cmtice - that should leave this code fine/correct. dblaikie: Yep, DWARFVersion shouldn't be set when none was requested - see discussion above between… | |||||
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. cmtice: DWARFVersion is (will be) only set when the debug info kind is DWARF, i.e. when EmitDwarf is… | |||||
| DebugInfoKind = codegenoptions::NoDebugInfo; | DebugInfoKind = codegenoptions::NoDebugInfo; | ||||
| // We ignore flag -gstrict-dwarf for now. | // We ignore flag -gstrict-dwarf for now. | ||||
| // And we handle flag -grecord-gcc-switches later with DWARFDebugFlags. | // And we handle flag -grecord-gcc-switches later with DWARFDebugFlags. | ||||
| Args.ClaimAllArgs(options::OPT_g_flags_Group); | Args.ClaimAllArgs(options::OPT_g_flags_Group); | ||||
| // Column info is included by default for everything except SCE and | // Column info is included by default for everything except SCE and | ||||
| // CodeView. Clang doesn't track end columns, just starting columns, which, | // CodeView. Clang doesn't track end columns, just starting columns, which, | ||||
| ▲ Show 20 Lines • Show All 2,904 Lines • ▼ Show 20 Lines | void ClangAs::ConstructJob(Compilation &C, const JobAction &JA, | ||||
| unsigned DwarfVersion = 0; | unsigned DwarfVersion = 0; | ||||
| Args.ClaimAllArgs(options::OPT_g_Group); | Args.ClaimAllArgs(options::OPT_g_Group); | ||||
| if (Arg *A = Args.getLastArg(options::OPT_g_Group)) { | if (Arg *A = Args.getLastArg(options::OPT_g_Group)) { | ||||
| WantDebug = !A->getOption().matches(options::OPT_g0) && | WantDebug = !A->getOption().matches(options::OPT_g0) && | ||||
| !A->getOption().matches(options::OPT_ggdb0); | !A->getOption().matches(options::OPT_ggdb0); | ||||
| if (WantDebug) | if (WantDebug) | ||||
| DwarfVersion = DwarfVersionNum(A->getSpelling()); | DwarfVersion = DwarfVersionNum(A->getSpelling()); | ||||
| } | } | ||||
| if (DwarfVersion == 0) | |||||
MaskRayUnsubmitted Not Done ReplyInline Actionsif (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: lang=cpp
if (DwarfVersion == 0) {
DwarfVersion = ParseDwarfDefaultVersion(getToolChain… | |||||
probinsonUnsubmitted Not Done ReplyInline ActionsHi @MaskRay , your version would have the unclaimed-option problem. probinson: Hi @MaskRay , your version would have the unclaimed-option problem. | |||||
| DwarfVersion = ParseDwarfDefaultVersion(getToolChain(), Args); | |||||
probinsonUnsubmitted Not Done ReplyInline ActionsThis 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. probinson: This is the path for parsing assembler options (when feeding clang a .s file) and I believe as… | |||||
Not Done ReplyInline ActionsI 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? probinson: I have to say, this sequence is a whole lot easier to follow than what's up in… | |||||
Not Done ReplyInline ActionsOnce 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? dblaikie: Once the "else if (DefaultDWARFVersion != 0)" clause is removed, and the… | |||||
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 cmtice: The thought is that if the user actually chose a specific dwarf version, e.g. -gdwarf-2, then… | |||||
Not Done ReplyInline ActionsThat'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: That's the desired behavior, yes (-gdwarf-2 overrides this new flag) - what @probinson is… | |||||
| if (DwarfVersion == 0) | if (DwarfVersion == 0) | ||||
| DwarfVersion = getToolChain().GetDefaultDwarfVersion(); | DwarfVersion = getToolChain().GetDefaultDwarfVersion(); | ||||
| codegenoptions::DebugInfoKind DebugInfoKind = codegenoptions::NoDebugInfo; | codegenoptions::DebugInfoKind DebugInfoKind = codegenoptions::NoDebugInfo; | ||||
| if (SourceAction->getType() == types::TY_Asm || | if (SourceAction->getType() == types::TY_Asm || | ||||
Not Done ReplyInline ActionsSome 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: Some clang-formatting required, but also could probably be written as:
if (DwarfVersion ==… | |||||
Ok will do. cmtice: Ok will do. | |||||
| SourceAction->getType() == types::TY_PP_Asm) { | SourceAction->getType() == types::TY_PP_Asm) { | ||||
| // You might think that it would be ok to set DebugInfoKind outside of | // You might think that it would be ok to set DebugInfoKind outside of | ||||
| // the guard for source type, however there is a test which asserts | // the guard for source type, however there is a test which asserts | ||||
| // that some assembler invocation receives no -debug-info-kind, | // that some assembler invocation receives no -debug-info-kind, | ||||
| // and it's not clear whether that test is just overly restrictive. | // and it's not clear whether that test is just overly restrictive. | ||||
| DebugInfoKind = (WantDebug ? codegenoptions::LimitedDebugInfo | DebugInfoKind = (WantDebug ? codegenoptions::LimitedDebugInfo | ||||
| : codegenoptions::NoDebugInfo); | : codegenoptions::NoDebugInfo); | ||||
| // Add the -fdebug-compilation-dir flag if needed. | // Add the -fdebug-compilation-dir flag if needed. | ||||
| ▲ Show 20 Lines • Show All 301 Lines • Show Last 20 Lines | |||||
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).