<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 21, 2014 at 3:23 PM, Frédéric Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On Oct 21, 2014, at 2:09 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><br><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Tue, Oct 21, 2014 at 2:01 PM, Frédéric Riss<span> </span><span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span><blockquote type="cite"><div>On Oct 20, 2014, at 2:06 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr">I've checked this out on the GDB test suite & it seems fine.<br><br>Looking at the code itself, I think it could be made a bit more obvious/uniform to have VariableDIE always refer to the definition and have just a single conditional to decide whether to attach the DW_AT_specification or not.<br><br>Actually this works pretty well & even adds another source fidelity benefit, allowing the definition, while out-of-line of the class, to be attributed to the scope in which the definition occurs (eg: namespace x { struct foo { static int i; }; int foo::i; } - putting the definition in namespace 'x' instead of just forcing it out to the global namespace) and exposes a bug in Clang where the definition is scoped to the decl context instead of the lexical context. (patch attached to show a slightly short-sighted fix there)<br></div></div></blockquote><div><br></div></span><div>Yeah, this looks an even nicer cleanup. I re-tested it just to be sure. Can you just commit what’s in your branch, or do you want me to reroll your reworked patch with a proper commit message?</div></div></div></blockquote><div><br></div><div>It'll need the clang change first, or we'll end up putting the static variable's definition inside the class (DW_AT_class_type with both a DW_AT_member and a DW_AT_variable as children, both describing the static member variable, with the latter pointing to the former via DW_AT_specification) & I'll need to fix/work up test coverage for it (so if you want to do those things instead, I won't complain - but I can do them).<br></div></div></div></blockquote><div><br></div></span><div>Oh that’s what it does, sorry, I thought you were just talking about file/line attribution of the definition… Interestingly it doesn’t seem to bother lldb much, but it’s obviously too ugly to live! The file/line issue is of course related. I thought it was the backend grabbing the wrong information from the Decls, but it’s in fact the frontend passing the wrong information down.</div><span class=""><div><br></div><blockquote type="cite"><div><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div>What was your thinking on the scoping issue in the frontend? It's certainly not the full solution - the full solution, imho would be to emit a declaration (potentially the DW_AT_member in the class) whenever the lexical context and the decl context differ. This would be the precise source fidelity. (& in the backend, do the right thing for fields that differ between the two - so we can describe the location of the definition in the .cpp file as different from the location of the declaration in the .h file, etc)<br></div></div></div></blockquote><div><br></div></span><div>Well, you have sent out the wrong patch for the frontend part, thus I misunderstood the exact issue you were trying to solve. If you send the right patch I can try to make sense of it and get an opinion on the matter.</div></div></div></blockquote><div><br></div><div>Oh, sorry - when you said "wrong patch" I thought you meant "wrong approach" rather than "some compeltely unrelated code"... I'm not sure where the original patch went, but here's what it looks like:<br><br><div>diff --git lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.cpp</div><div>index 723da71..1fcf5ba 100644</div><div>--- lib/CodeGen/CGDebugInfo.cpp</div><div>+++ lib/CodeGen/CGDebugInfo.cpp</div><div>@@ -3177,8 +3177,9 @@ void CGDebugInfo::EmitGlobalVariable(llvm::GlobalVariable *Var,</div><div>   if (LinkageName == DeclName)</div><div>     LinkageName = StringRef();</div><div> </div><div>-  llvm::DIDescriptor DContext =</div><div>-    getContextDescriptor(dyn_cast<Decl>(D->getDeclContext()));</div><div>+  llvm::DIDescriptor DContext = getContextDescriptor(</div><div>+      dyn_cast<Decl>(D->isStaticDataMember() ? D->getLexicalDeclContext()</div><div>+                                             : D->getDeclContext()));</div><div> </div><div>   // Attempt to store one global variable for the declaration - even if we</div><div>   // emit a lot of fields.</div></div><div><br><br>As I said, this is a bit of a hack to get the static data member stuff right, but in the end we should probably emit decl+def for any global variable who's decl context is different from its lexical context.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br></div><div>Fred</div><span class=""><br><blockquote type="cite"><div><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div> - David</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>Thanks!<br></div></div></div></blockquote><div><br>Thanks for your patience,<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div></div><div>Fred</div><br><blockquote type="cite"><div><span><div dir="ltr">If we did a few more smart things in the frontend, we could get better fidelity for declarations/definitions of namespace/global variables - removing the weird special case introduced with my patch. Probably the right thing here is to optimize in the frontend, if the definition appears in the decl context (eg: if the global variable's decl context is the same as the lexical context) then just produce the definition. If the decl and lexical context differ, produce both a declaration (in the decl context) and a definition (in the lexical context) - this would be the best source fidelity, but probably isn't too worthwhile.<br><br>Oh, and if we wanted to be really good here, we could make sure we emit any attributes onto the definition that differ from the declaration - note GCC does the right thing here, putting DW_AT_decl_line/file on the definitions if the line/file happens to be different from the declaration's line/file. We don't do that - we just put nothing on the definition (& I don't think we do this for function declarations/definitions either (again, here, GCC gets this right)... could, but probably not important)<br><br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 16, 2014 at 8:46 AM, Frederic Riss<span> </span><span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">ping<br><br><a href="http://reviews.llvm.org/D5457" target="_blank">http://reviews.llvm.org/D5457</a><br><br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></blockquote></div><br></div></span><span><static_var_def.diff></span><span><global_var.diff></span></div></blockquote></div></div></blockquote></div></div></blockquote></span></div><br></div></blockquote></div><br></div></div>