<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 11, 2015 at 4:39 AM, Amjad Aboud <span dir="ltr"><<a href="mailto:amjad.aboud@intel.com" target="_blank">amjad.aboud@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aaboud added a comment.<br>
<br>
Thanks David for your comments. I tried to answer all of them.<br>
Hopefully, the answers are clear.<br>
<br>
Also, I agreed to fix some code according to your comments, but I prefer to hear your reply on the other comments before I upload a new patch.<br>
<span><br>
<br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:340-341<br>
@@ +339,4 @@<br>
+ if (!includeMinimalInlineScopes()) {<br>
+ auto LocalDeclNodeRangeForScope = DD->findLocalDeclNodesForScope(DS);<br>
+ for (const auto &DI : LocalDeclNodeRangeForScope) {<br>
+ if (auto *IE = dyn_cast<DIImportedEntity>(DI.second))<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> Could probably just roll the local variable into the for loop header.<br>
</span>I will.<br>
I just thought that "findLocalDeclNodesForScope()" function is not cheap, and was not sure that calling it in the "for" loop header is guaranteed to happen once.<br></blockquote><div><br></div><div>Ah, no worries. See the code sample in the "Explanation" section here: <a href="http://en.cppreference.com/w/cpp/language/range-for" target="_blank">http://en.cppreference.com/w/cpp/language/range-for</a> - that shows pretty much exactly the code the C++ range-for loop translates to.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If you think that it will not be a performance issue, I will move it to the "for" loop header.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:342-343<br>
@@ +341,4 @@<br>
+ for (const auto &DI : LocalDeclNodeRangeForScope) {<br>
+ if (auto *IE = dyn_cast<DIImportedEntity>(DI.second))<br>
+ Children.push_back(constructImportedEntityDIE(IE));<br>
+ else if (RegularScope) {<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> Should we be adding imported entity DIEs in inlined or abstract definitions? Not sure... I suppose maybe.<br>
><br>
> What does it mean not to add the types and static variables to these cases? (if we inline away all uses of a function - won't we lose the local types and static variables & the user won't be able to print/interact with them?)<br>
</span>First of all, notice that we agreed to keep the current behavior of how types/static variables are handled.<br>
Right now, we are emitting them only in the original function (the one that did not get inline, regardless if it has a code or not),</blockquote><div><br></div><div>"regardless of if it has a code or not" - I'm not quite sure what you mean by this, but I'll take a stab:<br><br>There are up to 3 different "functions" that we end up producing in the debug info:<br><br>1) the abstract definition<br>2) the concrete definition<br>3) the inline definitions<br><br>If any inlining happens, we always produce (1) and (3), and if not all call sites were inlined, we produce (2) as well.<br><br>You're saying that currently we only emit local types and local static variables into (2) (thus if all calls are inlined, and (2) is not emitted, we do not emit these types and static variables at all? What do we do if there's a variable of that type in the inlined code? wow, we produce a weird little pseudo-(2) that just has the nested type in it... huh)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> the only difference is that we used to emit them always in the function scope, and this patch assure emitting them in the right lexical scope.<br>
We will discuss how to improve that in a separate thread once we are done with this patch.<br></blockquote><div><br></div><div>Do you have a test case for a situation like that? (maybe it's not relevant to your patch - if it's just existing weird behavior, but if you had to write code to preserve it, we should test it - even if it's not the desirable end state (& maybe it is the right end state, I'm not sure)) Here's the test case I was just playing with, I'm not sure it adds anything compared to your existing tests:<br><br> void f1(void*)<br> inline __attribute__((always_inline)) void f2() {<br> struct foo {} f;<br> f1(&f);<br> }<br> void f3() {<br> f2();<br> }<br><br><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Imported entries however, are correctly supported (also with inline) and they will always be added to the lexical scope, no matter if it is inline, abstract or regular scope. Thus, there is no need to change that.<br>
If you ask me, once we handle the abstract/concrete static variable, we will end up emitting them too in all kind of lexical scopes.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:351-352<br>
@@ +350,4 @@<br>
+ getOrCreateGlobalVariableDIE(GV, /* SkipContext */ true));<br>
+ }<br>
+ else if (auto *RT = dyn_cast<DIType>(DI.second)) {<br>
+ if (getDIE(RT))<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> Formatting (run clang-format over your change?)<br>
</span>I will do before I upload a new patch.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:353-355<br>
@@ +352,5 @@<br>
+ else if (auto *RT = dyn_cast<DIType>(DI.second)) {<br>
+ if (getDIE(RT))<br>
+ // Type was already created, nothing to do.<br>
+ continue;<br>
+ Children.push_back(getOrCreateTypeDIE(RT, /* SkipContext */ true));<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> Doesn't "getOrCreateTypeDIE" already do the getDIE check? (that would be implied by the "get" part of "get or create" in the name, I would hope)<br>
</span>True, however, if the getDIE in the getOrCreateTypeDIE returns an entry we will not know that and will end up adding this entry twice to the parent scope. This could happen, because types might be created bottom-up and added to the lexical block before handling the lexical scope itself (top-down). </blockquote><div><br>When does this happen (that the type is created before the scope it is in)? Does one of your tests cover this (I guess the empty scope case in the PR24008.ll test exercises this - the scope is not created (because it is empty when the function's debug info is generated) but then later on we try to build the local type (to reference from the definition of Run()) and fail because there's no context to put it in?)?<br><br>Hmm, PR24008.ll exercises the case where the scope is built (& possibly non-existent) before the type, but not the other way around, which seems to be the case here. That the scope is created before we are generating code for this function and its scopes - what sort of source code leads to that situation? (I can think of some, but not sure if they're tested here, etc)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This is not the case for static variables, they will never be created bottom-up, that is why we have only an assertion there.<br>
This is why we need to check "getDIE" for types separately before calling "getOrCreateTypeDIE()" function. </blockquote><div><br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
If we can differ creating types till all lexical scopes are created, this will prevent the above case for types, but it is not trivial thing to do.<br>
<br>
Another thing, which I thought to discuss later with the abstract/concrete static variables, is to emit the type in all kind of lexical scope (inline, abstract, regular). However, we need to discuss it separately as type entry might not be small like the imported entry.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:377-389<br>
@@ -353,4 +376,15 @@<br>
}<br>
- ScopeDIE = constructLexicalScopeDIE(Scope);<br>
- assert(ScopeDIE && "Scope DIE should not be null.");<br>
+ // Lexical block of non-inline function is stored in the lexical scope map.<br>
+ if (RegularScope)<br>
+ ScopeDIE = getDIE(DS);<br>
+ if (ScopeDIE) {<br>
+ // Mapped lexical scope should be already attached to its parent scope.<br>
+ assert(ScopeDIE->getParent() && "Scope DIE was not attached to parent.");<br>
+ AddScopeDieToFinalChildren = false;<br>
+ } else {<br>
+ ScopeDIE = constructLexicalScopeDIE(Scope);<br>
+ assert(ScopeDIE && "Scope DIE should not be null.");<br>
+ if (RegularScope)<br>
+ insertDIE(Scope->getScopeNode(), ScopeDIE);<br>
+ }<br>
}<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> I'm somewhat confused about what this code is about/for. Could you help explain it further?<br>
</span>Let's start from the lines that was not changed (385-386), these are clear, we are creating a new lexical scope.<br>
And if we know that it is a regular lexical scope, we want to add it to the scope map (387-388).<br>
<br>
However, because now we are adding regular lexical scope to the map, we want to make sure we do not have this lexical block already created.<br>
It could be created already in case we created a type that belongs to this lexical scope (or to one of its children scopes), as I said before types are not differed till all lexical block are created!<br>
So, we want to check in the map for the lexical scope (379) but only for regular scope (378).<br>
And if we find the lexical scope in the map, this means that it was already added to its parent scope (380-383), so we mark that flag to prevent adding it twice to the parent scope (396-397).<br></blockquote><div><br></div><div>OK, so I think this & related parts of the change are still a source of confusion (for me) and complexity that I'm a bit concerned about.<br><br>It sounds like you're suggesting that scopes can be created both before a function is being processed (in the case of a nested type being needed prematurely - such as if that type is referenced by a function template, or the definition of one of its members, etc), and after (the same situation, but where the reference comes after the function and the scope had no code in it to begin with) - is that the case? Do you have clear test cases demonstrating both these situations?<br><br>Is that what causes all this complexity with regards to scope creation and access? Are there other weird corner cases? Can we simplify this at all?<br><br>Why doesn't the code end up looking more like:<br><br>/* find all the things for the scope */<br><br>if there are things<br> getOrCreateScope<br> add things to scope<br><br>It seems there are many more variants to this... <br><br>(do we create the local scope chain when we create the local type before the outer function? That seems difficult... (because we might not want the intermediate scopes - they may be empty))<br> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
When we discuss the abstract/concrete static variables and differing type creation, we might find a way to prevent this.<br>
But for now, we will need this code.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:742-743<br>
@@ +741,4 @@<br>
+ if (!ContextDIE && !SkipContext) {<br>
+ // Can reach this point when the parent scope is a lexical block.<br>
+ // Which was not created yet, or that does not contain code, create it.<br>
+ assert(isa<DILexicalBlockBase>(Context) && "Expected lexical block scope");<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> When do we do this while not skipping context?<br>
><br>
> (also - creating a lexical block that doesn't contain any code may be rather useless - the debugger will never be at a location inside that scope, so it may be difficult to access the variables, etc, there if the debugger implements correct scoping rules... - but I'm not sure. What do other compilers do here?)<br>
</span>We do that because for some reason LLVM decide to emit debug info for functions that has no code (see the test PR24008.ll below).<br>
even though the static member function "Run()" has no code and even when it was inline there is no instructions related to it, LLVM decided at endModule to create entry for this function, but it is a method, so it end up creating the class Bar type, which happens to be defined inside a lexical block that has no code!<br>
<br>
If it was not a type, I would agree with you and say we should not be doing that, but type entries are needed to complete other entries, and we should emit them even if the parent scope of these types has no code.<br></blockquote><div><br></div><div>Yes, that seems true - and we can't just remove their parent scope & collapse them into their grandeparent's scope (that would possibly shadow/interfere with other names present there).<br><br>I think it's quite right/the only thing to do, to emit them into an "empty" scope like that, when the type is needed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
GCC does that too, in his own way with his own gaps (if you ask me)...I still did not see a compiler that does this perfectly, hopefully we can eventually make LLVM do that the best.<br></blockquote><div><br></div><div>It'd be interesting to talk about what /is/ perfect here. I see a lot of tradeoffs, but nothing terribly elegant... but perhaps there's an option</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
================<br>
Comment at: test/DebugInfo/X86/lexical-block-inline.ll:20<br>
@@ +19,3 @@<br>
+;; This test was generated by running following command:<br>
+;; clang -cc1 -O2 -g -emit-llvm foo.cpp<br>
+;; Where foo.cpp<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> Rather than using -O2 which changes a bunch of other stuff in the debug info too, it would be preferable to use no optimizations and add __attribute__((alwaysinline)) (or is it forceinline? I always forget the name) to the function that must be inlined.<br>
</span>I am not sure if this will work.<br>
But I can run clang with -O0, then run inline/always-inline pass using opt if this sounds better to get less noise from other passes.<br></blockquote><div><br>Certainly worth a shot. I've usually done this with all my inlining tests, if it's just inlining you need - it's a little harder to make an empty scope, but not impossible (force inline an empty function as the only code in the scope, I suppose). Even if you have to run optimizations to make a scope empty, probably still worth having the inline attributes to make it clear what's necessary to be inlined, etc.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I will then comment that in the test description.<br>
<div><div><br>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D11180" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11180</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div></div>