[PATCH] Implement -Wframe-larger-than backend diagnostic

[Richard Smith]

On Mon, Jun 2, 2014 at 9:38 PM, Alp Toker <alp at nuanti.com> wrote:

> Updated patch attached. Responding to Rafael and Richard's reviews
> inline...
>
>
> On 03/06/2014 00:11, Richard Smith wrote:
>
>> +        if (isa<BlockDecl>(DC)) {
>> +          OS << "block function";
>> +          break;
>> +        }
>> +        if (isLambdaCallOperator(DC)) {
>> +          OS << "lambda function";
>> +          break;
>> +        }
>>
>> 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?
>>
>
> Good idea, changes made as suggested.
>
>
>  Also, have you considered using "lambda expression within <name of
>> surrounding entity>" or similar?
>>
>
> 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.
>
> I'm satisfied that the source location alone is sufficient to pinpoint
> subject declarations like we do in Sema diagnostics for now.
>
>
>
>> On Mon, Jun 2, 2014 at 5:51 AM, Rafael Espíndola <
>> rafael.espindola at gmail.com <mailto:rafael.espindola at gmail.com>> wrote:
>>
>>     Just some quick notes:
>>
>>     const Decl *Demangle
>>
>>     should probably be demangle, or maybe a getDeclForMangledName, since
>>     it does more than demangle a string.
>>
>>
> Right, but I'm still going with uppercase to match the other functions in
> that class.
>
>
>
>
>>     +  bool LookupRepresentativeDecl(
>>
>>     should start with a lowercase.
>>
>>
> Yep.
>
>
>
>>     And a big thanks. This should let us produce far better diagnostics
>>     for things that involve mangled names, like aliases!
>>
>>
> It's quite neat watching these harmoniously orchestrated backend/frontend
> diagnostics with -Wframe-larger-than=0 on a bootstrap build :-)
>
>
>
>>     btw, this also improves things a bit for pr18188, right?
>>
>>
> Indeed, switching DeferredDecls to std::map without its own string
> allocator resolves that PR by sharing central string storage for all
> mangled strings.
>
> Patch updated!
>
>
>>
>>     On 30 May 2014 12:23, Alp Toker <alp at nuanti.com
>>     <mailto:alp at nuanti.com>> wrote:
>>     > The attached patch adds driver and frontend support for the GCC
>>     > -Wframe-larger-than=bytes warning.
>>     >
>>     > As the first GCC-compatible backend diagnostic built around
>>     LLVM's reporting
>>     > feature the patch adds infrastructure to perform reverse lookup
>>     from mangled
>>     > names emitted after LLVM code generation. Using that we resolve
>>     precise
>>     > locations and originating AST functions, lambdas or block
>>     declarations to
>>     > provide rich codegen-guided diagnostics.
>>     >
>>     > Supporting changes:
>>     >   * Get rid of the old MangleBuffer class that was only used for
>>     blocks
>>
>>
>> Can you split out / commit this separately?
>>
>
> r210058
>
>
>      >   * Let StringMap maintain unique mangled name strings instead
>>     of allocating
>>     > copies
>>
>>
>> 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.
>>
>
> I measure a 42% reduction in memory allocated for mangled name storage
> building the LLVM sources. The updated patch also reduces malloc churn.


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?)


>     > The test intentionally conflates driver, frontend and backend
>>     facilities for
>>     > now, and I expect parts of the approach to evolve as we get a
>>     feel for how
>>     > it'll play ball with LTO etc.
>>
>>
>> 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?
>>
>
> 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.
>
> Thanks for the feedback!


Patch LGTM with one minor tweak:

 --- a/lib/CodeGen/CodeGenModule.cpp
+++ b/lib/CodeGen/CodeGenModule.cpp
@@ -498,47 +498,39 @@ void CodeGenModule::setTLSMode(llvm::GlobalVariable
*GV,
 }

 StringRef CodeGenModule::getMangledName(GlobalDecl GD) {
-  const auto *ND = cast<NamedDecl>(GD.getDecl());
-
   StringRef &Str = MangledDeclNames[GD.getCanonicalDecl()];
   if (!Str.empty())
     return Str;

-  if (!getCXXABI().getMangleContext().shouldMangleDeclName(ND)) {
+  const auto *ND = cast<NamedDecl>(GD.getDecl());
+  SmallString<256> Buffer;
+  if (getCXXABI().getMangleContext().shouldMangleDeclName(ND)) {
+    llvm::raw_svector_ostream Out(Buffer);
+    if (const auto *D = dyn_cast<CXXConstructorDecl>(ND))
+      getCXXABI().getMangleContext().mangleCXXCtor(D, GD.getCtorType(),
Out);
+    else if (const auto *D = dyn_cast<CXXDestructorDecl>(ND))
+      getCXXABI().getMangleContext().mangleCXXDtor(D, GD.getDtorType(),
Out);
+    else
+      getCXXABI().getMangleContext().mangleName(ND, Out);
+    Str = Out.str();

Instead of changing the MangledDeclNames entry to temporarily refer to a
stack buffer here...

[...]
+  auto &Mangled = Manglings.GetOrCreateValue(Str);
+  Mangled.second = GD;
+  return Str = Mangled.first();

... and then overwriting it here, please use a separate StringRef variable
for the temporary result.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140603/0d47734e/attachment.html>