Page MenuHomePhabricator

debug-infoProject
ActivePublic

Recent Activity

Yesterday

shafik added a comment to D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs.

The difference between those two cases would be based on their usage - the types are the same (kinda, basically). In one case there's an anonymous member of the anonymous type (so it's transparent/all the nested names are as-if they were names of members in the outer type) and in the other case there's a named variable of the anonymous type (so the named members are members of that outer member variable).

Mon, Aug 19, 9:11 PM · debug-info
aganea updated the diff for D66328: [DebugInfo] Add debug location to dynamic atexit destructor.

As requested.

Mon, Aug 19, 7:46 PM · debug-info, Restricted Project
aprantl added inline comments to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..
Mon, Aug 19, 5:21 PM · Restricted Project, debug-info
dblaikie added a comment to D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs.

The difference between those two cases would be based on their usage - the types are the same (kinda, basically). In one case there's an anonymous member of the anonymous type (so it's transparent/all the nested names are as-if they were names of members in the outer type) and in the other case there's a named variable of the anonymous type (so the named members are members of that outer member variable).

Mon, Aug 19, 4:24 PM · debug-info
shafik added a comment to D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs.

Eh, I think it's still pretty unnecessary (& the DWARF spec is only suggestive, doesn't tend to have requirements in this regard) and so I'd probably suggest waiting until some consumer really wants this (& I'd still want to have a discussion with the consumer about why they find this to be needed). But it's really cheap (especially in DWARFv5 where it can use a const_value form & cost no bytes in debug_info (if/when that sort of thing is implemented in LLVM, if it isn't already).

@shafik, Can you outline the problem Clang has differentiating anonymous structs and lambdas that made this patch necessary?

Mon, Aug 19, 4:09 PM · debug-info
aprantl added a comment to D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs.

Eh, I think it's still pretty unnecessary (& the DWARF spec is only suggestive, doesn't tend to have requirements in this regard) and so I'd probably suggest waiting until some consumer really wants this (& I'd still want to have a discussion with the consumer about why they find this to be needed). But it's really cheap (especially in DWARFv5 where it can use a const_value form & cost no bytes in debug_info (if/when that sort of thing is implemented in LLVM, if it isn't already).

Mon, Aug 19, 3:57 PM · debug-info
dblaikie added a comment to D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs.

I'm not sure I agree with the DWARF issue here - name handling is necessarily language-specific, and there's no ambiguity about what an anonymous struct means in C or C++, there's only one way to do name resolution correctly there, and that's by treating them as transparent.

So I'd say rather than requiring all producers to put DW_AT_export_symbols on every anonymous struct in C or C++, consumers should assume its presence (if they want to model this name lookup this way - of course they don't need to model this in terms of DWARF concepts at all) for C and C++.

While the link quoted is a DWARF issue, that issue did actually make it into DWARF 5, (cf. 5.7.1 line 28ff). In case you were already aware of this, are you suggesting we should ignore the DWARF spec in LLVM here?

Mon, Aug 19, 3:47 PM · debug-info
aprantl added a comment to D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs.

I'm not sure I agree with the DWARF issue here - name handling is necessarily language-specific, and there's no ambiguity about what an anonymous struct means in C or C++, there's only one way to do name resolution correctly there, and that's by treating them as transparent.

So I'd say rather than requiring all producers to put DW_AT_export_symbols on every anonymous struct in C or C++, consumers should assume its presence (if they want to model this name lookup this way - of course they don't need to model this in terms of DWARF concepts at all) for C and C++.

Mon, Aug 19, 3:37 PM · debug-info
dblaikie added a comment to D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs.

I'm not sure I agree with the DWARF issue here - name handling is necessarily language-specific, and there's no ambiguity about what an anonymous struct means in C or C++, there's only one way to do name resolution correctly there, and that's by treating them as transparent.

Mon, Aug 19, 3:05 PM · debug-info
aprantl added inline comments to D66328: [DebugInfo] Add debug location to dynamic atexit destructor.
Mon, Aug 19, 2:43 PM · debug-info, Restricted Project
avl added inline comments to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..
Mon, Aug 19, 2:18 PM · Restricted Project, debug-info
avl added inline comments to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..
Mon, Aug 19, 2:08 PM · Restricted Project, debug-info
avl added a comment to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..

OK, I will modify testcase to be single pass.

Mon, Aug 19, 1:49 PM · Restricted Project, debug-info
rnk added a comment to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..
In D64595#1621077, @avl wrote:

Thus, for the current moment I think to implement your original suggestion with avoiding dbg.declare lowering in instcombiner for aggregates.

Mon, Aug 19, 1:43 PM · Restricted Project, debug-info
rnk added a comment to D66328: [DebugInfo] Add debug location to dynamic atexit destructor.

I'd be happy to take this patch, address the comments, and land it, if you don't want to deal with all the nits.

Mon, Aug 19, 1:17 PM · debug-info, Restricted Project
dstenb added a comment to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..

(A bit related to aprantl's latest comment.)

Mon, Aug 19, 12:10 PM · Restricted Project, debug-info
probinson added a comment to D66328: [DebugInfo] Add debug location to dynamic atexit destructor.

"debug-info-no-location.cpp" is an extremely generic name for a very specific case. "debug-info-atexit-stub.cpp" would be better.

Mon, Aug 19, 11:19 AM · debug-info, Restricted Project
aprantl added a comment to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..

Looking good, and this produces the ~3% increase in variable locations in clang-3.4 builds like the previous revisions did too. It looks like the two xfailed tests are testing for the behaviour you're explicitly disabling: it's probably better to just delete them, as this is a deliberate decision to change that behaviour.

Paging @aprantl and @rnk : this patch sounds great to me (avoids dropping struct locations after SROA of inlined functions, increases variable location coverage) by not lower dbg.declare of structs, which IMHO the comment in LowerDbgDeclare indicates wasn't supposed to happen anyway. However, this plays into the wider "escaped variables in memory" problem of PR34136... would either of you feel strongly that this shouldn't happen?

Mon, Aug 19, 9:45 AM · Restricted Project, debug-info
dstenb added inline comments to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..
Mon, Aug 19, 9:38 AM · Restricted Project, debug-info
jmorse added a comment to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..

Looking good, and this produces the ~3% increase in variable locations in clang-3.4 builds like the previous revisions did too. It looks like the two xfailed tests are testing for the behaviour you're explicitly disabling: it's probably better to just delete them, as this is a deliberate decision to change that behaviour.

Mon, Aug 19, 9:17 AM · Restricted Project, debug-info
aprantl added a comment to D66415: [DebugInfo@O2] Fix PR41992: LiveDebugVariables can drop DBG_VALUEs through misinterpreting fragments.

I think you may need at least one more test that involves fragments. Two non-overlapping fragments should be handled like DBG_VALUEs that describe different variables. Two (partially) overlapping fragments should be handled as if they are describing the same variable.

Mon, Aug 19, 9:12 AM · Restricted Project, debug-info
aprantl added a comment to D66415: [DebugInfo@O2] Fix PR41992: LiveDebugVariables can drop DBG_VALUEs through misinterpreting fragments.

The testcase makes perfect sense and that sounds like a good bug to fix!
I do wonder wonder why the patch goes out of its way to save fragment infos when fragments don't appear in the test at all?

Mon, Aug 19, 9:10 AM · Restricted Project, debug-info
Orlando created D66415: [DebugInfo@O2] Fix PR41992: LiveDebugVariables can drop DBG_VALUEs through misinterpreting fragments.
Mon, Aug 19, 5:44 AM · Restricted Project, debug-info
dstenb closed D66145: [DebugInfo] Allow bundled calls in the MIR's call site info.
Mon, Aug 19, 5:40 AM · Restricted Project, debug-info

Fri, Aug 16

avl updated the diff for D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..

I implemented the solution which avoids lowering dbg.declare for structures. Please check updated patch. Note, I marked two tests as XFAIL since they check for lowering which is not done with this patch. Though it looks to me that it would be better to make dbg.addr to work. I am thinking of changing LowerDbgDeclare in such a way that it would produce dbg.addr and dbg.value instead of dbg.value only. i.e. dbg.addr would be generated for cases when dbg.value with memory operand is generated currently...

Fri, Aug 16, 9:58 AM · Restricted Project, debug-info
shafik updated the diff for D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs.

Deleting some lines from the test as requested.

Fri, Aug 16, 9:17 AM · debug-info
aprantl added a project to D66328: [DebugInfo] Add debug location to dynamic atexit destructor: debug-info.
Fri, Aug 16, 9:11 AM · debug-info, Restricted Project
aprantl added inline comments to D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs.
Fri, Aug 16, 8:18 AM · debug-info
aprantl added reviewers for D66352: Debug Info: Support for DW_AT_export_symbols for anonymous structs: probinson, dblaikie.
Fri, Aug 16, 8:10 AM · debug-info
jmorse added a comment to D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass..

Sorry for the long delay --

Fri, Aug 16, 7:01 AM · Restricted Project, debug-info

Thu, Aug 15

dstenb added a comment to D66145: [DebugInfo] Allow bundled calls in the MIR's call site info.

Thanks for the reviews! I can let this sit in Phabricator for a few more days to see if there are any more comments before landing it.

Thu, Aug 15, 12:57 AM · Restricted Project, debug-info

Wed, Aug 14

twoh closed D66187: [DebugInfo] Consider debug label scope has an extra lexical block file.
Wed, Aug 14, 10:58 AM · debug-info, Restricted Project
aprantl added a comment to D66187: [DebugInfo] Consider debug label scope has an extra lexical block file.

(I'm approaching this from the point of "if the verifier accepts the IR, the backend has to deal with it", but David is right in pointing out that there may be a bug further up the pipeline and that perhaps it should be fixed and made illegal in IR.)

Wed, Aug 14, 10:56 AM · debug-info, Restricted Project
aprantl accepted D66187: [DebugInfo] Consider debug label scope has an extra lexical block file.

Seems reasonable to me.

Wed, Aug 14, 10:47 AM · debug-info, Restricted Project
aprantl accepted D66145: [DebugInfo] Allow bundled calls in the MIR's call site info.

I don't know enough about VLIW bundle handling to really decide, but mechanically, this seems good.

Wed, Aug 14, 8:55 AM · Restricted Project, debug-info
dstenb updated the diff for D66145: [DebugInfo] Allow bundled calls in the MIR's call site info.

Update test case according to comments.

Wed, Aug 14, 1:30 AM · Restricted Project, debug-info

Tue, Aug 13

djtodoro added a comment to D66145: [DebugInfo] Allow bundled calls in the MIR's call site info.

Thanks a lot for this! It looks reasonable to me.
(I am not sure if I can approve it, so please wait for @aprantl and @vsk too)

Tue, Aug 13, 11:30 PM · Restricted Project, debug-info
twoh added a comment to D66187: [DebugInfo] Consider debug label scope has an extra lexical block file.

@dblaikie Thank you for the comment. I encountered this problem while manipulating LLVM IR, and not sure how realistic the code can be if I synthesize one to reproduce this problem. Still, I think this is worth fixing unless we can guarantee that debug label scope won't have a DILexicalBlockFile.

Tue, Aug 13, 6:19 PM · debug-info, Restricted Project
dblaikie added a comment to D66187: [DebugInfo] Consider debug label scope has an extra lexical block file.

Might be helpful to have (at least for the review, doesn't necessarily need to be formalized into a lit test and checked in) a minimal source reproduction so we can more readily see what's happening here?

Tue, Aug 13, 5:05 PM · debug-info, Restricted Project
dblaikie added a project to D66187: [DebugInfo] Consider debug label scope has an extra lexical block file: debug-info.
Tue, Aug 13, 5:05 PM · debug-info, Restricted Project
rjmccall added inline comments to D66121: Debug Info: Nest Objective-C property function decls inside their container..
Tue, Aug 13, 2:29 PM · debug-info
vsk edited reviewers for D66121: Debug Info: Nest Objective-C property function decls inside their container., added: ahatanak, erik.pilkington; removed: vsk.
Tue, Aug 13, 2:06 PM · debug-info
aprantl added inline comments to D66121: Debug Info: Nest Objective-C property function decls inside their container..
Tue, Aug 13, 1:53 PM · debug-info
rjmccall added inline comments to D66121: Debug Info: Nest Objective-C property function decls inside their container..
Tue, Aug 13, 11:26 AM · debug-info
aprantl added inline comments to D66121: Debug Info: Nest Objective-C property function decls inside their container..
Tue, Aug 13, 10:48 AM · debug-info
arsenm added inline comments to D66145: [DebugInfo] Allow bundled calls in the MIR's call site info.
Tue, Aug 13, 9:02 AM · Restricted Project, debug-info
dstenb created D66145: [DebugInfo] Allow bundled calls in the MIR's call site info.
Tue, Aug 13, 8:43 AM · Restricted Project, debug-info
lisa6jones6 added a project to rL368639: Removed builders: llvm-clang-lld-x86_64-debian-fast, ubuntu-gcc7.1-werror.: debug-info.
Tue, Aug 13, 5:53 AM

Mon, Aug 12

rjmccall added inline comments to D66121: Debug Info: Nest Objective-C property function decls inside their container..
Mon, Aug 12, 11:30 PM · debug-info
vsk added a comment to D66121: Debug Info: Nest Objective-C property function decls inside their container..

Looks reasonable to me.

Mon, Aug 12, 4:56 PM · debug-info