[all-commits] [llvm/llvm-project] 5ab9fa: [lldb][NFC] Make metadata tracking type safe

[Raphael Isemann via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: 5ab9fa44cd60d5bca7b6d809a86bf10be416eb5d
      https://github.com/llvm/llvm-project/commit/5ab9fa44cd60d5bca7b6d809a86bf10be416eb5d
  Author: Raphael Isemann <teemperor at gmail.com>
  Date:   2019-12-13 (Fri, 13 Dec 2019)

  Changed paths:
    M lldb/include/lldb/Symbol/ClangASTContext.h
    M lldb/include/lldb/Symbol/ClangExternalASTSourceCommon.h
    M lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
    M lldb/source/Symbol/ClangASTContext.cpp
    M lldb/source/Symbol/ClangExternalASTSourceCommon.cpp

  Log Message:
  -----------
  [lldb][NFC] Make metadata tracking type safe

Summary:
LLDB associates additional information with Types and Declarations which it calls ClangASTMetadata.
ClangASTMetadata is stored by the ClangASTSourceCommon which is implemented by having a large map of
`void *` keys to associated `ClangASTMetadata` values. To make this whole mechanism even unsafer
we also decided to use `clang::Decl *` as one of pointers we throw in there (beside `clang::Type *`).

The Decl class hierarchy uses multiple inheritance which means that not all pointers have the
same address when they are implicitly converted to pointers of their parent classes. For example
`clang::Decl *` and `clang::DeclContext *` won't end up being the same address when they
are implicitly converted from one of the many Decl-subclasses that inherit from both.

As we use the addresses as the keys in our Metadata map, this means that any implicit type
conversions to parent classes (or anything else that changes the addresses) will break our metadata tracking
in obscure ways.

Just to illustrate how broken this whole mechanism currently is:
```lang=cpp
  // m_ast is our ClangASTContext. Let's double check that from GetTranslationUnitDecl
  // in ClangASTContext and ASTContext return the same thing (one method just calls the other).
  assert(m_ast->GetTranslationUnitDecl() == m_ast->getASTContext()->getTranslationUnitDecl());
  // Ok, both methods have the same TU*. Let's store metadata with the result of one method call.
  m_ast->SetMetadataAsUserID(m_ast->GetTranslationUnitDecl(), 1234U);
  // Retrieve the same Metadata for the TU by using the TU* from the other method... which fails?
  EXPECT_EQ(m_ast->GetMetadata(m_ast->getASTContext()->getTranslationUnitDecl())->GetUserID(), 1234U);
  // Turns out that getTranslationUnitDecl one time returns a TranslationUnitDecl* but the other time
  // we return one of the parent classes of TranslationUnitDecl (DeclContext).
```

This patch splits up the `void *` API into two where one does the `clang::Type *` tracking and one the `clang::Decl *` mapping.
Type and Decl are disjoint class hierarchies so there is no implicit conversion possible that could influence
the address values.

I had to change the storing of `clang::QualType` opaque pointers to their `clang::Type *` equivalents as
opaque pointers are already `void *` pointers to begin with. We don't seem to ever set any qualifier in any of these
QualTypes to this conversion should be NFC.

Reviewers: labath, shafik, aprantl

Reviewed By: labath

Subscribers: JDevlieghere, lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D71409