Emit debug entry values using standard DWARF5 opcodes when the debugger
tuning is set to lldb.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
| llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | ||
|---|---|---|
| 793 ↗ | (On Diff #219572) | Let's remove the "For now only" part. That's a fairly large percentage of developers :-) |
Comment Actions
Thanks for this! I am so exciting to see this implemented. :)
LGTM as well.
| llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | ||
|---|---|---|
| 793 ↗ | (On Diff #219572) | +1 :) |
Comment Actions
An idea: After this got implemented, we can add a new sub-directory inside the debuginfo-tests project called DOC (Debugging Optimized Code) and start testing (and tracking) improvements in that area. WDYT?
Comment Actions
I don't think there is such a clear line between optimized and unoptimized code for this distinction to make sense. For example, the Swift compiler frontend at -Onone generates LLVM IR that looks much more like clang at -O1 and thus exercises the dbg.value pipeline that we typically think of as optimized code even though it isn't really.
Comment Actions
I'm very curious what the effect of this patch on our green dragon statistics bot will be (http://green.lab.llvm.org/green/view/LLDB/job/clang-3.4-debuginfo-statistics/).
Comment Actions
What sort of statistics do we track that would be impacted by this change?
(to me it doesn't seem like this feature would improve coverage, etc - partly mechanically (this adds new tags we might not have any stats for) and partly practically (knowing the value of the parameter in the caller doesn't necessarily mean you know the value of the parameter at various points in the callee - parameters are usually mutable, so the callee might've modified the parameter & the callee's value won't reflect that))
Comment Actions
None for now, because debug entry values are gated behind a CC1 flag the bot does not use. I'm not sure what the plan is here exactly, but what I'd like to do is turn this on by default for DWARF5, and leave the CC1 flag for anyone who wants call site information in DWARF4 mode.
(to me it doesn't seem like this feature would improve coverage, etc - partly mechanically (this adds new tags we might not have any stats for)
(assuming we're compiling for DWARF5 or are using the CC1 flag) -- Variable location coverage should go up, as llvm should be able to describe arguments that are never mutated. @djtodoro has also added some additional stats related to call site info in D66525.
and partly practically (knowing the value of the parameter in the caller doesn't necessarily mean you know the value of the parameter at various points in the callee - parameters are usually mutable, so the callee might've modified the parameter & the callee's value won't reflect that))
Yes, good point, parameters are generally mutable but clang doesn't request call site parameter info for mutable arguments right now. Doing so would require some extra debug info support (a bit that says "hey, not sure this value is up-to-date!"), and this just hasn't been implemented yet.
Comment Actions
This change is enabling them by default when tuning for LLDB, right? Or still only when using the cc1 flag /and/ tuning for LLDB?
Oh, I see - this is about whether to use the GNU forms or the DWARFv5 forms, not whether to enable the feature by default - I get it now.
I'm not sure what the plan is here exactly, but what I'd like to do is turn this on by default for DWARF5, and leave the CC1 flag for anyone who wants call site information in DWARF4 mode.
(to me it doesn't seem like this feature would improve coverage, etc - partly mechanically (this adds new tags we might not have any stats for)
(assuming we're compiling for DWARF5 or are using the CC1 flag) -- Variable location coverage should go up, as llvm should be able to describe arguments that are never mutated. @djtodoro has also added some additional stats related to call site info in D66525.
Does the variable location coverage statistic use these tags? If so, how? I wouldn't've expected that would be possible - because the variables are in the callee, but the tags are in the caller - so how would the statistic be represented (if 3 out of 5 call sites have the attributes, say - or if the call site is in one object file but the callee is in another object file - how much location coverage is provided by the call site attribute?)
and partly practically (knowing the value of the parameter in the caller doesn't necessarily mean you know the value of the parameter at various points in the callee - parameters are usually mutable, so the callee might've modified the parameter & the callee's value won't reflect that))
Yes, good point, parameters are generally mutable but clang doesn't request call site parameter info for mutable arguments right now. Doing so would require some extra debug info support (a bit that says "hey, not sure this value is up-to-date!"), and this just hasn't been implemented yet.
(also tricky if the caller's version of the parameter is mutable, but the callee's is not - eg: you pass a copy of the int "foo" - the local copy in the caller gets the call site parameter, but then during the execution of the callee, something causes the original "foo" to have its value changed (perhaps someone else has a pointer to foo, etc... ))
Comment Actions
The latter: cc1 flag + tuning for lldb.
Oh, I see - this is about whether to use the GNU forms or the DWARFv5 forms, not whether to enable the feature by default - I get it now.
The DWARFv5 forms were already the default choice when the tuning isn't set to GDB, this change is about allowing TAG_call_site_parameter to be emitted at all when the tuning is set to LLDB.
I'm not sure what the plan is here exactly, but what I'd like to do is turn this on by default for DWARF5, and leave the CC1 flag for anyone who wants call site information in DWARF4 mode.
(to me it doesn't seem like this feature would improve coverage, etc - partly mechanically (this adds new tags we might not have any stats for)
(assuming we're compiling for DWARF5 or are using the CC1 flag) -- Variable location coverage should go up, as llvm should be able to describe arguments that are never mutated. @djtodoro has also added some additional stats related to call site info in D66525.
Does the variable location coverage statistic use these tags? If so, how? I wouldn't've expected that would be possible - because the variables are in the callee, but the tags are in the caller - so how would the statistic be represented (if 3 out of 5 call sites have the attributes, say - or if the call site is in one object file but the callee is in another object file - how much location coverage is provided by the call site attribute?)
The idea is that, in the callee, we may now have variables which use DW_OP_entry_value, whereas before the patch that wouldn't have been the case. So the new entry value locations expand the ranges covered by AT_location.
and partly practically (knowing the value of the parameter in the caller doesn't necessarily mean you know the value of the parameter at various points in the callee - parameters are usually mutable, so the callee might've modified the parameter & the callee's value won't reflect that))
Yes, good point, parameters are generally mutable but clang doesn't request call site parameter info for mutable arguments right now. Doing so would require some extra debug info support (a bit that says "hey, not sure this value is up-to-date!"), and this just hasn't been implemented yet.
(also tricky if the caller's version of the parameter is mutable, but the callee's is not - eg: you pass a copy of the int "foo" - the local copy in the caller gets the call site parameter, but then during the execution of the callee, something causes the original "foo" to have its value changed (perhaps someone else has a pointer to foo, etc... ))
Comment Actions
*nod*
The idea is that, in the callee, we may now have variables which use DW_OP_entry_value, whereas before the patch that wouldn't have been the case. So the new entry value locations expand the ranges covered by AT_location.
This change (looking at the test changes, at least) looks to be about call_site tags, rather than entry_value?
Anyway, I haven't been paying close attention to the optimized debug info or these DWARFv5 location attributes - so don't mind me. Didn't mean to derail the review or anything.
Comment Actions
No worries. The call site tags and OP_entry_value are conceptually linked, I've written-up an example of how this all fits together in D67376 if you're interested.