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).
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.
OK, I will modify testcase to be single pass.
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.
(A bit related to aprantl's latest comment.)
"debug-info-no-location.cpp" is an extremely generic name for a very specific case. "debug-info-atexit-stub.cpp" would be better.
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.
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.
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?
Fri, Aug 16
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...
Deleting some lines from the test as requested.
Sorry for the long delay --
Thu, Aug 15
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.
Wed, Aug 14
(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.)
Seems reasonable to me.
I don't know enough about VLIW bundle handling to really decide, but mechanically, this seems good.
Update test case according to comments.
Tue, Aug 13
@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.
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?
Mon, Aug 12
Looks reasonable to me.