<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jun 2, 2014 at 9:38 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.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">
Updated patch attached. Responding to Rafael and Richard's reviews inline...<div class=""><br>
<br>
On 03/06/2014 00:11, Richard Smith 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">
+ if (isa<BlockDecl>(DC)) {<br>
+ OS << "block function";<br>
+ break;<br>
+ }<br>
+ if (isLambdaCallOperator(DC)) {<br>
+ OS << "lambda function";<br>
+ break;<br>
+ }<br>
<br>
Neither of these terms are used in any existing diagnostics, nor in the relevant specifications. Maybe "block literal" and "lambda expression" would be more appropriate?<br>
</blockquote>
<br></div>
Good idea, changes made as suggested.<div class=""><br>
<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">
Also, have you considered using "lambda expression within <name of surrounding entity>" or similar?<br>
</blockquote>
<br></div>
Yes, considered doing so but printQualifiedName() isn't up to the job and outputting the next getNonClosureAncestor() NamedDecl is uninformative, even misleading at first glance because it mentions an unrelated entity.<br>
<br>
I'm satisfied that the source location alone is sufficient to pinpoint subject declarations like we do in Sema diagnostics for now.<div class=""><br>
<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">
<br>
On Mon, Jun 2, 2014 at 5:51 AM, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a> <mailto:<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@<u></u>gmail.com</a>>> wrote:<br>
<br>
Just some quick notes:<br>
<br>
const Decl *Demangle<br>
<br>
should probably be demangle, or maybe a getDeclForMangledName, since<br>
it does more than demangle a string.<br>
<br>
</blockquote>
<br></div>
Right, but I'm still going with uppercase to match the other functions in that class.<div class=""><br>
<br>
<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">
<br>
+ bool LookupRepresentativeDecl(<br>
<br>
should start with a lowercase.<br>
<br>
</blockquote>
<br></div>
Yep.<div class=""><br>
<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">
<br>
And a big thanks. This should let us produce far better diagnostics<br>
for things that involve mangled names, like aliases!<br>
<br>
</blockquote>
<br></div>
It's quite neat watching these harmoniously orchestrated backend/frontend diagnostics with -Wframe-larger-than=0 on a bootstrap build :-)<div class=""><br>
<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">
<br>
btw, this also improves things a bit for pr18188, right?<br>
<br>
</blockquote>
<br></div>
Indeed, switching DeferredDecls to std::map without its own string allocator resolves that PR by sharing central string storage for all mangled strings.<br>
<br>
Patch updated!<br>
<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 class="">
<br>
<br>
On 30 May 2014 12:23, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br></div><div class="">
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
> The attached patch adds driver and frontend support for the GCC<br>
> -Wframe-larger-than=bytes warning.<br>
><br>
> As the first GCC-compatible backend diagnostic built around<br>
LLVM's reporting<br>
> feature the patch adds infrastructure to perform reverse lookup<br>
from mangled<br>
> names emitted after LLVM code generation. Using that we resolve<br>
precise<br>
> locations and originating AST functions, lambdas or block<br>
declarations to<br>
> provide rich codegen-guided diagnostics.<br>
><br>
> Supporting changes:<br>
> * Get rid of the old MangleBuffer class that was only used for<br>
blocks<br>
<br>
<br>
Can you split out / commit this separately?<br>
</div></blockquote>
<br>
r210058<div class=""><br>
<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">
> * Let StringMap maintain unique mangled name strings instead<br>
of allocating<br>
> copies<br>
<br>
<br>
Do you have an feel for / have you measured the memory usage before and after this patch? I'm not expecting any big surprises here, since we were retaining storage for the mangled names anyway, but it'd be nice to have the extra confidence.<br>
</blockquote>
<br></div>
I measure a 42% reduction in memory allocated for mangled name storage building the LLVM sources. The updated patch also reduces malloc churn.</blockquote><div><br></div><div>That sounds awesome. Is the story for C code as good? (It looks like we might be adding an extra allocation for the trivial mangled name of a C declaration?)</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 class=""><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">
> The test intentionally conflates driver, frontend and backend<br>
facilities for<br>
> now, and I expect parts of the approach to evolve as we get a<br>
feel for how<br>
> it'll play ball with LTO etc.<br>
<br>
<br>
We need to have a fallback codepath for the case where the lookup fails; am I right to assume that LTO (and IR from imported modules, and so on) would take that path and gracefully degrade?<br>
</blockquote>
<br></div>
The fallback is working fine with things we don't "demangle" yet like thunks. In those cases where we have no decl, the raw (and ungrammatical) LLVM diagnostic is emitted. Test case added.<br>
<br>
Thanks for the feedback!</blockquote><div><br></div><div>Patch LGTM with one minor tweak:</div><div><br></div><div> --- a/lib/CodeGen/CodeGenModule.cpp</div><div>+++ b/lib/CodeGen/CodeGenModule.cpp</div><div>@@ -498,47 +498,39 @@ void CodeGenModule::setTLSMode(llvm::GlobalVariable *GV,</div>
<div> }</div><div> </div><div> StringRef CodeGenModule::getMangledName(GlobalDecl GD) {</div><div>- const auto *ND = cast<NamedDecl>(GD.getDecl());</div><div>-</div><div> StringRef &Str = MangledDeclNames[GD.getCanonicalDecl()];</div>
<div> if (!Str.empty())</div><div> return Str;</div><div> </div><div>- if (!getCXXABI().getMangleContext().shouldMangleDeclName(ND)) {</div><div>+ const auto *ND = cast<NamedDecl>(GD.getDecl());</div><div>+ SmallString<256> Buffer;</div>
<div>+ if (getCXXABI().getMangleContext().shouldMangleDeclName(ND)) {</div><div>+ llvm::raw_svector_ostream Out(Buffer);</div><div>+ if (const auto *D = dyn_cast<CXXConstructorDecl>(ND))</div><div>+ getCXXABI().getMangleContext().mangleCXXCtor(D, GD.getCtorType(), Out);</div>
<div>+ else if (const auto *D = dyn_cast<CXXDestructorDecl>(ND))</div><div>+ getCXXABI().getMangleContext().mangleCXXDtor(D, GD.getDtorType(), Out);</div><div>+ else</div><div>+ getCXXABI().getMangleContext().mangleName(ND, Out);</div>
<div>+ Str = Out.str();</div><div><br></div><div>Instead of changing the MangledDeclNames entry to temporarily refer to a stack buffer here...</div><div><br></div><div>[...]</div><div>+ auto &Mangled = Manglings.GetOrCreateValue(Str);<br>
</div><div>+ Mangled.second = GD;</div><div>+ return Str = Mangled.first();</div><div><br></div><div>... and then overwriting it here, please use a separate StringRef variable for the temporary result.</div></div></div>
</div>