[Lldb-commits] [PATCH] D54357: [NativePDB] Improved support for nested type reconstruction

[Zachary Turner via lldb-commits]

On Mon, Nov 12, 2018 at 6:51 AM Aleksandr Urakov via Phabricator <
reviews at reviews.llvm.org> wrote:

> aleksandr.urakov added a comment.
>
> This change looks reasonable to me for solving the problem with the
> current `LF_NESTTYPE` approach. But I'm not sure... Now we all the same
> need to analyze mangled names and make a decision based on this. Does the
> current `LF_NESTTYPE` approach still has advantages over the "parse-scope"
> approach? I'm afraid that we will have to fully analyze a mangled name in
> the future for scoped types, so if the `LF_NESTTYPE` approach doesn't have
> a significant advantages over the "parse-scope" approach, do we really need
> to support them both?

I think we need to support both.  Certain pieces of information are not
represented in the mangling at all, so if we rely purely on the mangling we
will never be able to perfectly reconstruct the DeclContext hierarchy.  So
I think each approach by itself would be imperfect, but combined it will be
very good.



>
>
>
> ================
> Comment at:
> lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:560-576
> +  // If it's an inner definition, then treat whatever name we have here
> as a
> +  // single component of a mangled name.  So we can inject it into the
> parent's
> +  // mangled name to see if it matches.
> +  CVTagRecord child = CVTagRecord::create(tpi.getType(Record.Type));
> +  std::string qname = parent.asTag().getUniqueName();
> +  if (qname.size() < 4 || child.asTag().getUniqueName().size() < 4)
> +    return llvm::None;
> ----------------
> May be it would be a good idea to make the demangler (or to implement a
> mangler) more flexible to hide these details there? I do not mean to do it
> exactly in this commit, but what do you think about this at all?
>
>
> ================
> Comment at:
> lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:881-887
> +    // If there is no parent in the debug info, but some of the scopes
> have
> +    // template params, then this is a case of bad debug info.  See, for
> +    // example, llvm.org/pr39607.  We don't want to create an ambiguity
> between
> +    // a NamespaceDecl and a CXXRecordDecl, so instead we create a class
> at
> +    // global scope with the fully qualified name.
> +    if (AnyScopesHaveTemplateParams(scopes))
> +      return {context, record.Name};
> ----------------
> Is this a clang only bug, or MSVC also does so? I do not have anything
> against this solution for now, I just mean, may be we'll replace it with an
> `assert` when the bug will be fixed?


Clang only.  But I don’t think we can ever replace it with an assert, debug
info is basically user input, so we have to be able to handle every manner
of malformed input.  In fact, when this bug is fixed in clang, i will
probably try to keep a test case around that manually generates the bad
debug info using llvm-mc or something, just to make sure it will not
break.  Because, for example, someone could be debugging a program that was
built with the buggy compiler version.

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20181112/c7d6b8a8/attachment.html>