[PATCH] Fixed debug info generation for function static variables, typedef, and records

[David Blaikie]

On Fri, May 29, 2015 at 9:52 AM, Frederic Riss <friss at apple.com> wrote:

> REPOSITORY
>   rL LLVM
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:341
> @@ -339,3 +340,3 @@
>      // Skip imported directives in gmlt-like data.
>      if (!includeMinimalInlineScopes()) {
>        // There is no need to emit empty lexical block DIE.
> ----------------
> aaboud wrote:
> > friss wrote:
> > > I think the local types and static vars should respect this flag like
> the imported entities does. If we go the way of unifying the 3 maps (se
> comments bellow), then everything can be handled inside that if.
> > I see no reason to add them to the minimal flag.
> > We did not skip emitting local variable due to this flag, did we?
> > Imported Entities should respect the flag, but you will need to explain
> why other should!
> Actually we do not emit locals in that case. If you lookup the function
> includeMinimalInlineScopes() you'll see that it is true in line-tables-only
> mode in which we do not emit locals.
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:354-355
> @@ -350,1 +353,4 @@
> +    if (Children.size() == ChildScopeCount &&
> +        GVRangeForScope.begin() == GVRangeForScope.end() &&
> +        RTRangeForScope.begin() == RTRangeForScope.end()) {
>        FinalChildren.insert(FinalChildren.end(),
> ----------------
> aaboud wrote:
> > friss wrote:
> > > This could just query .empty() on the containers.
> > I wish it have.
> > the iterator_range container has no "empty()" method.
> righto, I thought this returned a container and I didn't come back to it
> when reading the function definitions below.
>

Yeah - I'm wondering how many free functions over ranges we might like:

template<typename Range>
bool empty(const Range  &R) {
  return R.begin() == R.end();
}

and the like


>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:459
> @@ +458,3 @@
> +    for (auto *GV : CUNode->getGlobalVariables()) {
> +      if (CU.getOrCreateContextDIE(GV->getScope()))
> +        CU.getOrCreateGlobalVariableDIE(GV);
> ----------------
> aaboud wrote:
> > friss wrote:
> > > Is that condition always safe to check if the scope is function-local?
> Just looking at the name of the function, it looks like we could change
> that one day to do something different for local scope (like create them
> right away like the name implies) and this would break the logic here.
> > If this function would be able to create a lexical block DIE, I would
> not need to implement this patch from the first place, right?
> > Also, if you check line 490. when we construct the Imported Entities, it
> does the same check. Actually, I copied this from their.
> Sorry, you didn't get my point. You are no just calling function that
> looks up the scope DIE. As its name implies, it can create things. I'm
> asking if the side effects of that function are safe to apply here. And if
> they ever change to return a scope where they don't return any today would
> it break the logic in getOrCreateGlobalVariable()? Looking at the code
> there, it seems safe on both fronts because that function is called anyway
> as one the the very first things (and this also applies to the
> getOrCreateType() call below).
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:265-271
> @@ -265,4 +264,9 @@
>    typedef SmallVector<std::pair<const MDNode *, const MDNode *>, 32>
> -  ImportedEntityMap;
> +  ImportedEntityMap, GlobalVariableMap, RetainedTypeMap;
> +  // Holder for imported entities.
>    ImportedEntityMap ScopesWithImportedEntities;
> +  // Holder for global variables.
> +  GlobalVariableMap ScopesWithGlovalVariables;
> +  // Holder for retained types.
> +  RetainedTypeMap ScopesWithRetainedTypes;
>
> ----------------
> aaboud wrote:
> > friss wrote:
> > > Couldn't we just merge the 3 maps. That are all used only in one and
> same place, and their handling is really similar. The fundamental
> difference between the 2 new ones and the existing one is that the DIEs
> created by getOrCreate{Type,Variable}DIE are automatically added to their
> parents, which doesn't happen for the imported entities. Maybe we should
> just unify all that? (I think David has written this part, so he might have
> an opinion on this)
> > I do not mind merging them, just need some good ideas on how to do that.
> Well, I'd try to put everything in the same map (renaming it beforehand to
> something like 'LocalDeclMap'. Then in constructScopeDIE, discriminate
> between the different types using dyn_cast (and handle everything in the
> same loop under the includeMinimalInlineScopes() condition). Is there
> anything I'm missing that makes this impractical?
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:744-745
> @@ -743,1 +743,4 @@
>
> +  if (DIE *TyDIE = getDIE(Ty))
> +    return TyDIE;
> +
> ----------------
> aaboud wrote:
> > friss wrote:
> > > Why is that new shortcut needed?
> > Yes, I thought it is new, it is not. I just moved it from below.
> > 1. I see no good reason to create the context if we already have the DIE
> itself.
> > 2. For Type DIEs with lexical block context, the function ContextDIE
> from below will be nullptr at this point.
> >
> The comment a few lines below ("Construct the context before querying for
> the existence of the DIE in case such construction creates the DIE") makes
> it seem like this is deliberate. Anyway if it's not needed for the patch it
> shouldn't be here (if you think it is a good idea, it can be the subject of
> another patch).
>
> http://reviews.llvm.org/D9758
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150529/1570e78e/attachment.html>