[lldb] Fix evaluation of qualified global variables

[evgeny777]

Summary

Current revision contains bug, related to evaluation of global variables. Imagine you have the following code

int _g = 1;
int func(void) {
    int _g = 2;
    return _g; // BP here and evaluate ::_g
}

evaluation of ::_g will return 2, while the correct value is 1

Another example:

namespace test {
    int test_var = 1;
}

int test_var = 2;
int func(void) {
     using namespace test;
     return ::test_var;  // BP here and try to evaluate ::test_var
}

Evaluation will return error (multiple candidates), while correct behaviour is to return '2'

[labath]

Could you please put the examples you cite above into the test suite?

[zturner]

In D13350#257676, @labath wrote:

Could you please put the examples you cite above into the test suite?

Agreed, please make a test for this.

[sivachandra]

Also, this patch has changes in clang. There should be a clang reviewer I think [unless I am unaware something here.]

[zturner]

Good point. The two patches should actually be separate, and you will need a separate set of tests for the clang patch.

[evgeny777]

Thanks for your comments. I will certainly add the test cases to lldb. I was just curious if this is correct way of fixing the issue. Besides test case what else has to be done? Separate review for changes in clang linked to this review?

[sivachandra]

I suppose you should first post a separate self contained clang patch to the clang list. After landing it in clang, you should post an LLDB specific patch here.

About its correctness, I cannot comment on the clang changes. Commenting on the LLDB changes are probably meaningful after the clang change lands.

[dawn]

This patch fixes bugs:

• https://llvm.org/bugs/show_bug.cgi?id=24994 (::val gets NS::val inside NS since r247746)
• https://llvm.org/bugs/show_bug.cgi?id=24995 (shadowed var gets ambiguity since r247836)

See inline comments. Mostly had concerns about indentation - please always run patches through clang-format (both clang and lldb have their own config file in the root dir).

clang/include/clang/AST/DeclBase.h

1769

Indenting in clang is 2 spaces.

lldb/include/lldb/Symbol/CompilerDeclContext.h

17

clang includes should precede lldb ones.

lldb/include/lldb/Symbol/TypeSystem.h

121

I don't understand this comment - please clarify? Also, please start comments with capital letters.

123

It would be cleaner to have a default argument instead of this second overload. What are the guidelines on using default arguments in lldb? I've seen them both used and not used.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

1370

Looks like indentation was accidentally changed here?

lldb/source/Symbol/ClangASTContext.cpp

8984

Please run patch through clang-format to fix indentation issues.

[dawn]

Was this patch run on the entire set of lldb tests? Were there any regressions?

[clayborg]

DeclBase.h will need to be submitted against clang, not LLDB.

Then add Sean Callanan and Jim Ingham as the reviewers as they are the masters of the expression parser. I helped to get the CompilerDecl and CompilerDeclContext stuff in, but Sean and Jim should review expression parser changes.

Please add Jim Ingham and Sean Callanan as reviewers.

[evgeny777]

Hi folks!

The clang patch has been landed. I've added test cases, so please look at the new patch. Thanks

[evgeny777]

Minor code cleanups

[clayborg]

Resigning as I will let Jim Ingham and Sean Callanan review.

[spyffe]

Yes, we should follow the same rules as in regular lookup for these contexts. If the test suite is happy here, I'm happy.

[Eugene.Zelenko]

Looks like patch was not committed.

[evgeny777]

This patch is longer valid after more than 9 months on review due to changes in namespace handing in ClangExpressionDeclMap. It probably should be done in a different way, but I don't have time for this now.

clang/include/clang/AST/DeclBase.h

class DeclContext {
   /// \brief If \c true, this context may have local lexical declarations
   /// that are missing from the lookup table.
   mutable bool HasLazyLocalLexicalLookups : 1;
 
   /// \brief If \c true, the external source may have lexical declarations
   /// that are missing from the lookup table.
   mutable bool HasLazyExternalLexicalLookups : 1;
 
+  /// \brief If \c true, lookups should only return identifier from
+  /// DeclContext scope (for example TranslationUnit). Used in
+  /// LookupQualifiedName()
+  mutable bool UseQualifiedLookup : 1;
+
   /// \brief Pointer to the data structure used to lookup declarations
   /// within this context (or a DependentStoredDeclsMap if this is a
   /// dependent context). We maintain the invariant that, if the map
   /// contains an entry for a DeclarationName (and we haven't lazily
   /// omitted anything), then it contains all relevant entries for that
   /// name (modulo the hasExternalDecls() flag).
   mutable StoredDeclsMap *LookupPtr;
 
protected:
   static std::pair<Decl *, Decl *>
   BuildDeclChain(ArrayRef<Decl*> Decls, bool FieldsAlreadyLoaded);
 
   DeclContext(Decl::Kind K)
       : DeclKind(K), ExternalLexicalStorage(false),
         ExternalVisibleStorage(false),
         NeedToReconcileExternalVisibleStorage(false),
         HasLazyLocalLexicalLookups(false), HasLazyExternalLexicalLookups(false),
+        UseQualifiedLookup(false),
         LookupPtr(nullptr), FirstDecl(nullptr), LastDecl(nullptr) {}
 
 public:
   ~DeclContext();
 
   Decl::Kind getDeclKind() const {
     return static_cast<Decl::Kind>(DeclKind);
   }
public:
 
   /// \brief Determine whether the given declaration is stored in the list of
   /// declarations lexically within this context.
   bool isDeclInLexicalTraversal(const Decl *D) const {
     return D && (D->NextInContextAndBits.getPointer() || D == FirstDecl || 
                  D == LastDecl);
   }
 
+  bool setUseQualifiedLookup(bool use = true) {
+      bool old_value = UseQualifiedLookup;
+      UseQualifiedLookup = use;
+      return old_value;
+  }
+
+  bool shouldUseQualifiedLookup() const {
+      return UseQualifiedLookup;
+  }
+
   static bool classof(const Decl *D);
   static bool classof(const DeclContext *D) { return true; }
 
   void dumpDeclContext() const;
   void dumpLookups() const;
   void dumpLookups(llvm::raw_ostream &OS, bool DumpDecls = false) const;
 
 private:

clang/lib/Sema/SemaLookup.cpp

public:
 
     llvm::iterator_range<const_iterator>
     getNamespacesFor(DeclContext *DC) const {
       return llvm::make_range(std::equal_range(begin(), end(),
                                                DC->getPrimaryContext(),
                                                UnqualUsingEntry::Comparator()));
     }
   };
+
+  class Deinitializer {
+    std::function<void()> Deinit;
+  public:
+    Deinitializer(const std::function<void()>& d): Deinit(d) {}
+    ~Deinitializer() { Deinit(); }
+  };
 }
 
 // Retrieve the set of identifier namespaces that correspond to a
 // specific kind of name lookup.
 static inline unsigned getIDNS(Sema::LookupNameKind NameKind,
                                bool CPlusPlus,
                                bool Redeclaration) {
   unsigned IDNS = 0;
bool Sema::LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx,
   // Make sure that the declaration context is complete.
   assert((!isa<TagDecl>(LookupCtx) ||
           LookupCtx->isDependentContext() ||
           cast<TagDecl>(LookupCtx)->isCompleteDefinition() ||
           cast<TagDecl>(LookupCtx)->isBeingDefined()) &&
          "Declaration context must already be complete!");
 
   // Perform qualified name lookup into the LookupCtx.
+  bool oldval = LookupCtx->setUseQualifiedLookup();
+  Deinitializer deinit([&]() { LookupCtx->setUseQualifiedLookup(oldval); });
+
   if (LookupDirect(*this, R, LookupCtx)) {
     R.resolveKind();
     if (isa<CXXRecordDecl>(LookupCtx))
       R.setNamingClass(cast<CXXRecordDecl>(LookupCtx));
     return true;
   }
 
   // Don't descend into implied contexts for redeclarations.

lldb/include/lldb/Symbol/ClangASTContext.h

public:
 
     //----------------------------------------------------------------------
     // CompilerDeclContext override functions
     //----------------------------------------------------------------------
     
     std::vector<void *>
     DeclContextFindDeclByName (void *opaque_decl_ctx, ConstString name) override;
 
+    std::vector<void *>
+    DeclContextFindDeclByNameEx (void *opaque_decl_ctx, ConstString name, void* extra) override;
+
     bool
     DeclContextIsStructUnionOrClass (void *opaque_decl_ctx) override;
 
     ConstString
     DeclContextGetName (void *opaque_decl_ctx) override;
 
     bool
     DeclContextIsClassMethod (void *opaque_decl_ctx,

lldb/include/lldb/Symbol/CompilerDeclContext.h

 //===-- CompilerDeclContext.h -----------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_CompilerDeclContext_h_
 #define liblldb_CompilerDeclContext_h_
 
 #include <vector>
 
 #include "lldb/lldb-private.h"
 #include "lldb/Core/ConstString.h"
+#include "clang/AST/DeclBase.h"
 
 namespace lldb_private {
 
 class CompilerDeclContext
 {
 public:
     //----------------------------------------------------------------------
     // Constructors and Destructors
public:
     {
         return m_type_system != nullptr && m_opaque_decl_ctx != nullptr;
     }
 
     bool
     IsClang () const;
 
     std::vector<CompilerDecl>
-    FindDeclByName (ConstString name);
+    FindDeclByName (ConstString name, const clang::DeclContext* lookupCtx);
 
     //----------------------------------------------------------------------
     /// Checks if this decl context represents a method of a class.
     ///
     /// @param[out] language_ptr
     ///     If non NULL and \b true is returned from this function,
     ///     this will indicate if the language that respresents the method.
     ///

lldb/include/lldb/Symbol/TypeSystem.h

public:
 
     //----------------------------------------------------------------------
     // CompilerDeclContext functions
     //----------------------------------------------------------------------
     
     virtual std::vector<void *>
     DeclContextFindDeclByName (void *opaque_decl_ctx, ConstString name) = 0;
 
+    // extra is language dependant
+    virtual std::vector<void *>
+    DeclContextFindDeclByNameEx (void *opaque_decl_ctx, ConstString name, void* extra)
+    {
+        // By default fall back to old one
+        return DeclContextFindDeclByName(opaque_decl_ctx, name);
+    }
+
     virtual bool
     DeclContextIsStructUnionOrClass (void *opaque_decl_ctx) = 0;
 
     virtual ConstString
     DeclContextGetName (void *opaque_decl_ctx) = 0;
 
     virtual bool
     DeclContextIsClassMethod (void *opaque_decl_ctx,

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

else
             if (compiler_decl_context)
             {
                 // Make sure that the variables are parsed so that we have the declarations
                 VariableListSP vars = frame->GetInScopeVariableList(true);
                 for (size_t i = 0; i < vars->GetSize(); i++)
                     vars->GetVariableAtIndex(i)->GetDecl();
 
                 // Search for declarations matching the name
-                std::vector<CompilerDecl> found_decls = compiler_decl_context.FindDeclByName(name);
+                std::vector<CompilerDecl> found_decls =
+                        compiler_decl_context.FindDeclByName(name, context.m_decl_context);
                 
                 bool variable_found = false;
                 for (CompilerDecl decl : found_decls)
                 {
                     var = decl.GetAsVariable();
                     if (var)
                     {
                         variable_found = true;

lldb/source/Symbol/ClangASTContext.cpp

ClangASTContext::DeclGetName (void *opaque_decl)
     }
     return ConstString();
 }
 
 //----------------------------------------------------------------------
 // CompilerDeclContext functions
 //----------------------------------------------------------------------
 
+static inline bool
+NamedDeclDoesQualify (NamedDecl* nd, const DeclContext* lookupCtx)
+{
+    if(lookupCtx == nullptr || lookupCtx->shouldUseQualifiedLookup() == false)
+        return true;
+    return nd->getLexicalDeclContext()->getDeclKind() == lookupCtx->getDeclKind();
+}
+
 std::vector<void *>
 ClangASTContext::DeclContextFindDeclByName(void *opaque_decl_ctx, ConstString name)
 {
+    return DeclContextFindDeclByNameEx(opaque_decl_ctx, name, nullptr);
+}
+
+std::vector<void *>
+ClangASTContext::DeclContextFindDeclByNameEx (void *opaque_decl_ctx, ConstString name, void* extra)
+{
+    const DeclContext* lookupCtx = (const DeclContext*)(extra);
     std::vector<void *> found_decls;
     if (opaque_decl_ctx)
     {
         DeclContext *root_decl_ctx = (DeclContext *)opaque_decl_ctx;
         std::set<DeclContext *> searched;
         std::multimap<DeclContext *, DeclContext *> search_queue;
         SymbolFile *symbol_file = GetSymbolFile();
 
if (opaque_decl_ctx)
                     else if (clang::UsingDecl *ud = llvm::dyn_cast<clang::UsingDecl>(child))
                     {
                         for (clang::UsingShadowDecl *usd : ud->shadows())
                         {
                             clang::Decl *target = usd->getTargetDecl();
                             if (clang::NamedDecl *nd = llvm::dyn_cast<clang::NamedDecl>(target))
                             {
                                 IdentifierInfo *ii = nd->getIdentifier();
-                                if (ii != nullptr && ii->getName().equals(name.AsCString(nullptr)))
+                                if (ii != nullptr &&
+                                    ii->getName().equals(name.AsCString(nullptr)) &&
+                                    NamedDeclDoesQualify(nd, lookupCtx))
+                                {
                                     found_decls.push_back(nd);
                                 }
                             }
                         }
+                    }
                     else if (clang::NamedDecl *nd = llvm::dyn_cast<clang::NamedDecl>(child))
                     {
                         IdentifierInfo *ii = nd->getIdentifier();
-                        if (ii != nullptr && ii->getName().equals(name.AsCString(nullptr)))
+                        if (ii != nullptr &&
+                            ii->getName().equals(name.AsCString(nullptr)) &&
+                            NamedDeclDoesQualify(nd, lookupCtx))
+                        {
                             found_decls.push_back(nd);
                         }
                     }
                 }
             }
         }
+    }
     return found_decls;
 }
 
 bool
 ClangASTContext::DeclContextIsStructUnionOrClass (void *opaque_decl_ctx)
 {
     if (opaque_decl_ctx)
         return ((clang::DeclContext *)opaque_decl_ctx)->isRecord();

lldb/source/Symbol/CompilerDeclContext.cpp

 #include "lldb/Symbol/CompilerDeclContext.h"
 #include "lldb/Symbol/CompilerDecl.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include <vector>
 
 using namespace lldb_private;
 
 std::vector<CompilerDecl>
-CompilerDeclContext::FindDeclByName (ConstString name)
+CompilerDeclContext::FindDeclByName (ConstString name, const clang::DeclContext* lookupCtx)
 {
     std::vector<CompilerDecl> found_decls;
     if (IsValid())
     {
-        std::vector<void *> found_opaque_decls = m_type_system->DeclContextFindDeclByName(m_opaque_decl_ctx, name);
+        std::vector<void *> found_opaque_decls =
+            m_type_system->DeclContextFindDeclByNameEx(m_opaque_decl_ctx, name, (void*)lookupCtx);
         for (void *opaque_decl : found_opaque_decls)
             found_decls.push_back(CompilerDecl(m_type_system, opaque_decl));
     }
     return found_decls;
 }
 
 bool
 CompilerDeclContext::IsClang () const