This is an archive of the discontinued LLVM Phabricator instance.

Make CompilerInvocation's use of the debug options more understandable.
ClosedPublic

Authored by dougk on Sep 28 2015, 12:18 PM.

Details

Summary

Stop mucking with the semantics of debug options in CompilerInvocation.

This changes causes most of the 'g' group options to be rejected by CompilerInvocation. They remain only as Driver options.
The new way to tell cc1 what kind of debug info to emit is with "-di-kind={full|limited|line-tables}" "-dwarf-version={2|3|4}".

In the absence of any command-line option to specify Dwarf version, there is a new virtual method in the Toolchain rather than hiding Toolchain-specific logic in CompilerInvocation. This is important when the default depends on complicating factors that cc1 is not aware of.

Also fix a bug in the Windows compatibility argument parsing in which the "rightmost argument wins" principle failed.

Diff Detail

Repository
rL LLVM

Event Timeline

dougk updated this revision to Diff 35899.Sep 28 2015, 12:18 PM
dougk retitled this revision from to Make CompilerInvocation's use of the debug options more understandable..
dougk updated this object.
dougk added reviewers: chandlerc, jyknight.
dougk added a subscriber: cfe-commits.

I think the best thing to do is introduce new cc1 arguments that control the Opts explicitly, and cease accepting the equivalent driver arguments in cc1.
I would propose '-gdwarf-version={2|3|4}' and '-gdebug-info-kind={line-tables|limited|full}'

dougk updated this revision to Diff 35988.Sep 29 2015, 9:23 AM
dougk updated this object.

Replaced the cc1 arguments with two three-valued options. Fixed all the tests.

Reviewers: the "important" tests to review are:
test/Driver/clang-g-opts.c
test/Driver/debug-options.c
test/Driver/split-debug.c
test/Driver/cl-options.c

The rest are all mechanical and pretty much the minimally obvious change to avoid putting "-g" to in the cc1 invocation.

dougk updated this revision to Diff 36140.Sep 30 2015, 12:55 PM
dougk updated this object.

Fix CollectArgsForIntegratedAs and make sure that no other tests talk about "-g" after the Driver has produced the command invocation, except for xcore in which apparently xcc does accept -g.

aprantl added a subscriber: aprantl.Oct 2 2015, 3:23 PM
dougk added a comment.Oct 2 2015, 4:31 PM

A few more remarks:

  • The code which emits line-tables-only seems to understand dwarf2 versus dwarf4, but due solely to the way that arguments were parsed, the dwarf version did not propagate through to the compiler invocation if you also specified line-tables-only. The new pair of arguments does not have that issue.

So as mentioned on cfe-dev, this is another case of "yeah, I fixed that but wasn't really the intent". [All I want is MyriadToolchain to default to dwarf 2. That's it!]

  • It was suggested to me that it ought to have been possible to pull out all of the g group argument processing so that ClangAs::ConstructJob can share a little bit more code with Clang::ConstructJob but I'd really prefer not to do that in this change. It's possible that another small cleanup can be done, but the governing factor is that somehow or another you also have to deal with the case where the integrated-as tool has to parse '-gdwarf-N' flags (because people can pass those with -Xassembler,-gdwarf-). So you've got to be able to parse plain old StringRefs, as opposed to "Arg", which, absent that requirement, you'd just do with getOption().matches(). As it is, I think the code is well factored for this purpose.
jyknight edited edge metadata.Oct 2 2015, 6:43 PM

This certainly seems a big improvement.

However, I don't think that you actually fixed specifying both -gline-tables-only and a Dwarf version, because that requires actually handling multiple options from g_group, instead of just the last one. E.g. "-gdwarf-2 -g1" should cause the dwarf level to be set to 2, and enable line-tables. I expect the correct thing for Driver to do is to process *all* the args and apply them one at a time to the state variables, rather than just take the last arg.

(Not that I'm recommending that you fix that also in this commit, just not to claim it now works unless it does)

include/clang/Driver/CC1Options.td
135 ↗(On Diff #36140)

I'd suggest to keep the name of thevar and the string the same for easy searching.

lib/Driver/Tools.cpp
2353 ↗(On Diff #36140)

How about:

if (DwarfVersion > 0) CmdArgs.push_back("-dwarf-version=" + DwarfVersion);

Some bikeshedding about -di-kind={full|limited|line-tables}:

  1. Is there a good reason not to spell out -debug-info-kind?
  2. "full" is misleading as it will still only emit debug info for types actually used by the program. I think "standalone" would be more descriptive.
  3. Why not "line-tables-only" like in -gline-tables-only?
dougk marked an inline comment as done.Oct 7 2015, 8:03 AM

James, you're right, 'gdwarf-2' followed by 'line-tables-only' works, but the opposite order doesn't.
So it halfway works, which is better than not working at all.

lib/Driver/Tools.cpp
2353 ↗(On Diff #36140)

minor point: the "+" syntax you've used is actually pointer arithmetic, but giving the benefit of doubt there, and writing it as to_string(DwarfVersion), the annoying thing is that CmdArgs wants a 'const char*'.
To get one, you have to call MakeArgString which is a method on Args, not CmdArgs, so you have to pass both.
I did that in the latest patch, see what you think.

dougk updated this revision to Diff 36748.Oct 7 2015, 8:09 AM
dougk updated this object.
dougk edited edge metadata.

spell 'di-kind=' as 'debug-info-kind=', spell 'full' as 'standalone', spell 'line-tables' as 'line-tables-only', spell internal option name as debug_info_kind_EQ

dblaikie accepted this revision.Oct 7 2015, 9:13 AM
dblaikie added a reviewer: dblaikie.
dblaikie added a subscriber: dblaikie.
dblaikie added inline comments.
test/Driver/clang-g-opts.c
33 ↗(On Diff #36748)

Should this be -debug-info-kind instead?

This revision is now accepted and ready to land.Oct 7 2015, 9:13 AM
dougk marked an inline comment as done.Oct 7 2015, 10:59 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 3 2015, 10:08 AM
thakis added inline comments.
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
401

This lets cc1 crash if you pass -debug-info-kind=foo for any foo not explicitly handled in the stringswitch. (I don't see a nice and obvious fix, so leaving this to you.)