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

[Alp Toker]

On 04/06/2014 01:39, Richard Smith wrote:
> On Mon, Jun 2, 2014 at 9:38 PM, Alp Toker <alp at nuanti.com 
> <mailto: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>
>         <mailto: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>
>             <mailto: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?)

C declarations do take a hit from the extra allocation to enable reverse 
lookup.

I'm not that worried because we've got a clearer idea of how mangled 
names are managed now and can start to investigate far bigger savings 
e.g. reusing LLVM's ValueSymbolTable storage (conveniently also a 
StringMap with equivalent lifetime).

>           > 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.

Done and landed in r210293!

I've also committed r210294 which uses the new infrastructure to provide 
approximated diagnostic locations in some optimization remark scenarios 
where they were missing.

Next steps will be to cover cases like thunks, explore ways to serialize 
the mapping to provide reverse lookup in tools, and bringing the 
enhanced diagnostics to LTO and modules.

Thanks Richard, Rafael and Quentin for the review.

Alp.

-- 
http://www.nuanti.com
the browser experts