Page MenuHomePhabricator

Add option for emitting dbg info for call site parameters
ClosedPublic

Authored by djtodoro on Feb 11 2019, 3:09 AM.

Details

Summary

Generating of all info about call sites and call site parameters will be restricted with the option until all testing and patching is done.

Example of using the option:

clang -g -O2 -Xclang -femit-debug-entry-values test.c

In addition, when the option is set add flag all_call_sites
in a subprogram in order to support GNU extension of DWARF as well.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

Diff Detail

Event Timeline

djtodoro created this revision.Feb 11 2019, 3:09 AM
djtodoro retitled this revision from [clang] Add option for emitting dbg info for call sites to Add option for emitting dbg info for call sites.
djtodoro edited the summary of this revision. (Show Details)Feb 11 2019, 3:14 AM
djtodoro added a subscriber: NikolaPrica.
djtodoro added a project: debug-info.
djtodoro edited reviewers, added: cfe-commits; removed: llvm-commits.
djtodoro added a subscriber: petarj.
djtodoro edited the summary of this revision. (Show Details)Feb 11 2019, 4:48 AM
djtodoro updated this revision to Diff 195188.Apr 15 2019, 8:26 AM
djtodoro edited the summary of this revision. (Show Details)

-Rebase
-Add all_call_sites flag in the case of GNU extensions

aprantl added inline comments.Apr 15 2019, 8:28 PM
include/clang/Driver/Options.td
919

I assume that this is the same -f option that GCC uses?

djtodoro marked an inline comment as done.Apr 15 2019, 11:42 PM
djtodoro added inline comments.
include/clang/Driver/Options.td
919

Actually in GCC production of call_site and call_site_parameters debug info is enabled by default.
Production of entry_values is implemented on top of variable's value tracking system and there is no particular option just for entry_values.

Basically, we introduce this option as experimental one. As soon as we test everything we should get rid of this or turn it ON by default (and maybe add '-fno-emit-param-entry-values' in order to have an option for disabling the functionality). That will be ideal scenario.

I guess I'm not clear what your final goal is for the option. Keep it, even though GCC doesn't have one like it? Eliminate it? Please clearly state what you intend to have in the end, and what you might have in the short term in case that is different.

include/clang/Driver/CC1Options.td
362

I think this is not necessary, see comment on Options.td.

include/clang/Driver/Options.td
919

By convention, the name of the option should start with 'f' and match the option spelling (with hyphens changed to underscore).

921

I believe that by marking this with [CC1Option] cc1 will automatically understand it and you won't need to define a separate option in CC1Options.td.

And if we plan to enable it by default, too, perhaps not adding a driver option (CC1 only) is preferable, since we need to maintain compatibility with driver options indefinitely.

djtodoro marked 2 inline comments as done.Apr 17 2019, 7:31 AM

@probinson @aprantl Thanks a lot for your comments!

Let's clarify some things. I'm sorry about the confusion.

Initial patch for the functionality can be restricted by this option (like we pushed here), since LLDB doesn't read this type of debug info (yet).
The plan is, when LLDB has all necessary info and we are sure that everything works fine during using this info in debugging sessions, we can turn it to ON by default.

Does that make sense ? :)

include/clang/Driver/Options.td
921

I can't remember what problem we occurred with this on 4.0, but we definitely don't need both definitions.
Thanks for this!

djtodoro updated this revision to Diff 195568.Apr 17 2019, 7:35 AM
djtodoro retitled this revision from Add option for emitting dbg info for call sites to Add option for emitting dbg info for call site parameters.

-Refactor
-Remove CC1 def

djtodoro updated this revision to Diff 195571.Apr 17 2019, 7:54 AM

-Fix comments

@probinson @aprantl Thanks a lot for your comments!

Let's clarify some things. I'm sorry about the confusion.

Initial patch for the functionality can be restricted by this option (like we pushed here), since LLDB doesn't read this type of debug info (yet).
The plan is, when LLDB has all necessary info and we are sure that everything works fine during using this info in debugging sessions, we can turn it to ON by default.

Does that make sense ? :)

Yes. And I agree with Adrian, this should be a CC1-only option; it can be default-off for now and maybe debugger-tuning specific later.

djtodoro updated this revision to Diff 196053.Apr 22 2019, 4:47 AM

-Add only cc1 option
-Set up back end

Is there some kind of testcase?

Is there some kind of testcase?

@aprantl Usage of the option is tested within following patches from the stack. I am not sure if we need some additional test here?

probinson added inline comments.Apr 24 2019, 8:04 AM
lib/Driver/ToolChains/Clang.cpp
3402

If this is now a cc1-only option, this part goes away right?

djtodoro marked 2 inline comments as done.Apr 25 2019, 2:53 AM
djtodoro added inline comments.
lib/Driver/ToolChains/Clang.cpp
3402

Yes, thanks for this!

djtodoro updated this revision to Diff 196595.Apr 25 2019, 2:54 AM
djtodoro marked an inline comment as done.
djtodoro edited the summary of this revision. (Show Details)

-Remove unneeded code

I'm okay with this, but give @aprantl a chance to confirm.

aprantl accepted this revision.Apr 25 2019, 1:02 PM
This revision is now accepted and ready to land.Apr 25 2019, 1:02 PM
vsk added a comment.Apr 25 2019, 2:18 PM

I think a small extension to test/CodeGenCXX/dbg-info-all-calls-described.cpp for the 'Supported: DWARF4 + -femit_param_entry_values' case would be appropriate here.

@probinson @aprantl Thanks!

@vsk Thanks for the comment! Sure, we will add that.

djtodoro updated this revision to Diff 199213.May 13 2019, 1:33 AM
djtodoro edited the summary of this revision. (Show Details)

-Add an input in test/CodeGenCXX/dbg-info-all-calls-described.cpp
-Rename the option

djtodoro updated this revision to Diff 200485.May 21 2019, 6:33 AM

-Set EnableDebugEntryValues only for a higher level of optimizations (-O1 and higher)

aprantl accepted this revision.May 21 2019, 11:10 AM

LGTM, with an additional testcase.

test/CodeGenCXX/dbg-info-all-calls-described.cpp
23 ↗(On Diff #200485)

+ a negative test for -O0

djtodoro updated this revision to Diff 200943.May 23 2019, 6:04 AM

-Add a negative test for -O0

aprantl accepted this revision.May 23 2019, 9:32 AM
djtodoro updated this revision to Diff 203335.Jun 6 2019, 5:57 AM

-Explicitly enable the option only in the case of X86

probinson added inline comments.Jun 17 2019, 11:39 AM
lib/Frontend/CompilerInvocation.cpp
754

You want to disable entry-values for all targets other than X86?

djtodoro marked an inline comment as done.Jun 17 2019, 11:20 PM
djtodoro added inline comments.
lib/Frontend/CompilerInvocation.cpp
754

@probinson Thanks for the comment!

You want to disable entry-values for all targets other than X86?

Yes, for now. Because, if we want to support the entry-values feature for an architecture (other than X86), we would need to handle target specifics of call arguments lowering for the architecture.

For the initial set of patches, we have accomplished to test and add support only for X86. We will work on supporting more architectures.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 2:38 AM