<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 13, 2015 at 3:02 PM, Romanova, Katya <span dir="ltr"><<a href="mailto:Katya_Romanova@playstation.sony.com" target="_blank">Katya_Romanova@playstation.sony.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">





<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> David Blaikie [mailto:<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>]
<br>
<b>Sent:</b> Wednesday, May 13, 2015 11:06 AM<br>
<b>To:</b> <a href="mailto:reviews%2BD7895%2Bpublic%2B7827be49c0b04087@reviews.llvm.org" target="_blank">reviews+D7895+public+7827be49c0b04087@reviews.llvm.org</a><br>
<b>Cc:</b> Romanova, Katya; Eric Christopher; Frédéric Riss; Duncan P. N. Exon Smith; Robinson, Paul; Adrian Prantl; <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><span class=""><br>
<b>Subject:</b> Re: [PATCH] Anonymous namespaces are missing import DW_TAG_imported_module.<u></u><u></u></span></span></p>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Wed, May 13, 2015 at 10:56 AM, Katya Romanova <<a href="mailto:Katya_Romanova@playstation.sony.com" target="_blank">Katya_Romanova@playstation.sony.com</a>> wrote:<u></u><u></u></p><div><div class="h5">
<p class="MsoNormal">Hi David,<br>
Sorry for the delay answering your question about AST.<br>
<br>
> > One question, if you're up for it - there is actually an implicit UsingDirectiveDecl in the AST<br>
>..<br>
> > It's possible we could figure out why that decl is being skipped normally? & avoid skipping it/filter it out later for non-ps4 while leaving it in for ps4, rather than synthesizing the extra<br>
><br>
> >  imported module from scratch in the debug info code gen?<br>
><br>
> >  I took a glance & didn't immediately see why we weren't already visiting this implicit decl - I guess something is filtering out implicit decls somewhere?<br>
><br>
<br>
<br>
Implicit UsingDirectiveDecl is generated by Sema::ActOnStartNamespaceDef() when it’s given an anonymous namespace.<br>
The UsingDirectedDecl is added to the Parent (top level) which is why you see it in AST dump.<br>
<br>
Above function is called from ParseNamespace(), which is called by ParseDeclaration(). ParseDeclaration() returns DeclGroup.<br>
The problem is that group is populated with a SingleDecl that ParseNamespace returns ([anonymous] NamespaceDecl).<br>
<br>
ParseDeclaration doesn’t know anything about the UsingNamespaceDecl that ParseNamespace implicitly generated<br>
and added to the parent.. This implicit declaration is not added to the DeclGroup and thus you don’t generate<br>
debug node for it.<br>
<br>
If we want ParseNamespace to generate both NamespaceDecl AND an implicit UsingNamespaceDecl<br>
then the following piece of code:<br>
<br>
  case tok::kw_namespace:<br>
    ProhibitAttributes(attrs);<br>
    SingleDecl = ParseNamespace(Context, DeclEnd);<br>
    break;<br>
<br>
should return not the SingleDecl (converted to DeclGroup), but a DeclGroup containing both NamespaceDecl AND UsingNamespaceDecl.<br>
<br>
I think that this fix (conditional for PS4 only) will be more complicated and more invasive solution than FE solution that I implemented before.<br>
<br>
I saw that you already accepted my latest FE patch. Please confirm that you didn't change your mind about how this bug should get fixed after your saw this explanation about AST.<u></u><u></u></p>
</div></div><div><div><div class="h5">
<p class="MsoNormal"><br>
After seeing Clang's output on some other code, I'm not sure this fix is correct.<br>
<br>
Could you add a test case with an anonymous namespace inside another namespace? When I tried that with ToT, we /do/ emit the (implicit) using directive today. So your change might cause us to emit two using directives (one marked artificial, and one not).<br>
<br>
<span style="color:#1f497d"><u></u><u></u></span></p>
</div></div><p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">[Katya Romanova]
<u></u><u></u></span></i></b></p>
<p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I have tried anonymous namespace inside another anonymous namespace like you suggested . You are right. I can see that implicit using directive is always
 emitted for a nested namespace. That means, that for PS4 we will emit duplicate using directive for a nested anonymous namespace.<u></u><u></u></span></i></b></p><span class="">
<p class="MsoNormal"><br>
So it might be worth figuring out how to plumb through the implicit using directive for the top level namespace so we treat anonymous namespaces the same whether or not they are at the top level, and when making this change - then deliberately suppress the
 implicit using directive for an anonymous namespace except when targeting PS4/SCE.<br>
<br>
<span style="color:#1f497d"><u></u><u></u></span></p>
</span><p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">[Katya Romanova]
<u></u><u></u></span></i></b></p>
<p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">So the current plan is to add an implicit using directive for the top level anonymous namespace to the DeclGroup containing top level decls. Hopefully,
 it could be done by changing ParseNamespace to return a DeclGroup containing both NamespaceDecl and UsingDirectiveDecl.</span></i></b></p></div></div></div></div></div></div></div></blockquote><div><br>Sounds plausible - though it may need some extra review from Richard Smith or someone to check that that's a good thing to do given the broader surface area. But given that the nested case causes similar behavior to the behavior this change would create in the non-nested case, I'm fairly confident.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"><div><div><div><div><p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> If not, I will figure out a different way to do it.</span></i></b></p></div></div></div></div></div></div></div></blockquote><div><br>Feel free to reach out via email on the lists or IRC to talk more if you run into any hiccups.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"><div><div><div><div><p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></i></b></p>
<p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">After that, I need to suppress generation of a using directive for *top level* implicit anonymous namespace (except for PS4). I assume we still want to
 keep generating  implicit using directive for the *nested* anonymous namespace. Rright?</span></i></b></p></div></div></div></div></div></div></div></blockquote><div><br>Nah - just suppress them all for non-PS4 I think (perhaps we'll be surprised and GDB tests will fail when we do this - in which case we probably shouldn't suppress the imported modules at all - nested or non-nested). When I implemented this I clearly didn't test it enough & it seemed sufficient to fix the GDB test cases with the current behavior. I doubt GDB behaves correctly with top level non-imported anonymous namespaces and then would fail for non-top-level-non-imported anonymous namespaces.<br><br>If that all makes any sense... <br><br>(the other thing to do, after enabling this suppression (which you could do first, to fix the bug in non-top-level-anonymous namespace (the bug that we emit the imported directive when we needn't), conditionally on non-PS4 (so we continue to emit them on PS4) - then fix the DeclGroup thing and the conditional suppression should then fire for top level using directives and you'll get the behavior you're looking for?) is add the artificial attribute/flag/thing... but I'm not /too/ fussed about that part of it, if it's useful for you guys, do it, but if we're just leaving this as a workaround for your debugger then it doesn't seem too important if it requires changes to the metadata schema, etc (if it just requires using an existing flag, etc, then the cost is lower & it might be more worth it))<br><br>Sorry if that's a bit rambly... my brain's a bit all over the place today.<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"><div><div><div><div><p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">
<u></u><u></u></span></i></b></p>
<p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></i></b></p>
<p class="MsoNormal"><b><i><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Let me know if this solution sounds reasonable. I will implement the  patch.<u></u><u></u></span></i></b></p><span class="">
<p class="MsoNormal">ParseDeclaration doesn’t know anything about the UsingNamespaceDecl that ParseNamespace implicitly generated<br>
and added to the parent.. This implicit declaration is not added to the DeclGroup and thus you don’t generate<br>
debug node for it.<u></u><u></u></p>
<p class="MsoNormal">If we want ParseNamespace to generate both NamespaceDecl AND an implicit UsingNamespaceDecl<br>
then the following piece of code:<u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">case tok::kw_namespace:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">  ProhibitAttributes(attrs);<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">  SingleDecl = ParseNamespace(Context, DeclEnd);<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">  break;<u></u><u></u></span></p>
<p class="MsoNormal">should return not the SingleDecl (converted to DeclGroup), but a DeclGroup containing both NamespaceDecl AND UsingNamespaceDecl.<u></u><u></u></p>
<p class="MsoNormal"><span style="color:#1f497d"><u></u> <u></u></span></p>
</span><p class="MsoNormal"><br>
- David<br>
 <u></u><u></u></p>
</div><span class="">
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal"><br>
If everything is OK, I will rebase and commit my latest FE patch.<br>
Thanks for reviewing!<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
<br>
<a href="http://reviews.llvm.org/D7895" target="_blank">http://reviews.llvm.org/D7895</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">
http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<u></u><u></u></p>
</div>
</div>
</blockquote>
</span></div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div>
</div>
</div>

</blockquote></div><br></div></div>