[Lldb-commits] [lldb] cb81e66 - [lldb] Reject redefinitions of persistent variables

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-10-14T10:24:35+02:00
New Revision: cb81e662a58908913f342520e4c010564a68126a

URL: https://github.com/llvm/llvm-project/commit/cb81e662a58908913f342520e4c010564a68126a
DIFF: https://github.com/llvm/llvm-project/commit/cb81e662a58908913f342520e4c010564a68126a.diff

LOG: [lldb] Reject redefinitions of persistent variables

Currently one can redefine a persistent variable and LLDB will just silently
ignore the second definition:

```
(lldb) expr int $i = 1
(lldb) expr int $i = 2
(lldb) expr $i
(int) $i = 1
```

This patch makes this an error and rejects the expression with the second
definition.

A nice follow up would be to refactor LLDB's persistent variables to not just be
a pair of type and name, but also contain some way to obtain the original
declaration and source code that declared the variable. That way we could
actually make a full diagnostic as we would get from redefining a variable twice
in the same expression.

Reviewed By: labath, shafik, JDevlieghere

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

Added: 
    

Modified: 
    lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
    lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
    lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
    lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
index 8c49898e1d6c..7d8cd85500ae 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -19,6 +19,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Core/ValueObjectVariable.h"
+#include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/Expression/Materializer.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/CompilerDecl.h"
@@ -125,6 +126,12 @@ void ClangExpressionDeclMap::InstallCodeGenerator(
   m_parser_vars->m_code_gen = code_gen;
 }
 
+void ClangExpressionDeclMap::InstallDiagnosticManager(
+    DiagnosticManager &diag_manager) {
+  assert(m_parser_vars);
+  m_parser_vars->m_diagnostics = &diag_manager;
+}
+
 void ClangExpressionDeclMap::DidParse() {
   if (m_parser_vars && m_parser_vars->m_persistent_vars) {
     for (size_t entity_index = 0, num_entities = m_found_entities.GetSize();
@@ -196,6 +203,17 @@ bool ClangExpressionDeclMap::AddPersistentVariable(const NamedDecl *decl,
   if (ast == nullptr)
     return false;
 
+  // Check if we already declared a persistent variable with the same name.
+  if (lldb::ExpressionVariableSP conflicting_var =
+          m_parser_vars->m_persistent_vars->GetVariable(name)) {
+    std::string msg = llvm::formatv("redefinition of persistent variable '{0}'",
+                                    name).str();
+    m_parser_vars->m_diagnostics->AddDiagnostic(
+        msg, DiagnosticSeverity::eDiagnosticSeverityError,
+        DiagnosticOrigin::eDiagnosticOriginLLDB);
+    return false;
+  }
+
   if (m_parser_vars->m_materializer && is_result) {
     Status err;
 

diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
index 6974535a8993..0c81d46c6c52 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -102,6 +102,8 @@ class ClangExpressionDeclMap : public ClangASTSource {
 
   void InstallCodeGenerator(clang::ASTConsumer *code_gen);
 
+  void InstallDiagnosticManager(DiagnosticManager &diag_manager);
+
   /// Disable the state needed for parsing and IR transformation.
   void DidParse();
 
@@ -330,6 +332,8 @@ class ClangExpressionDeclMap : public ClangASTSource {
     clang::ASTConsumer *m_code_gen = nullptr; ///< If non-NULL, a code generator
                                               ///that receives new top-level
                                               ///functions.
+    DiagnosticManager *m_diagnostics = nullptr;
+
   private:
     ParserVars(const ParserVars &) = delete;
     const ParserVars &operator=(const ParserVars &) = delete;

diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 8ad39ecd2707..bd0d831f0ca7 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1078,6 +1078,7 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
   ClangExpressionDeclMap *decl_map = type_system_helper->DeclMap();
   if (decl_map) {
     decl_map->InstallCodeGenerator(&m_compiler->getASTConsumer());
+    decl_map->InstallDiagnosticManager(diagnostic_manager);
 
     clang::ExternalASTSource *ast_source = decl_map->CreateProxy();
 

diff  --git a/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py b/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py
index ebe180998c63..bdd1ccc59696 100644
--- a/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py
+++ b/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py
@@ -41,3 +41,19 @@ def test_persistent_variables(self):
         # Test that $200 wasn't created by the previous expression.
         self.expect("expr $200", error=True,
             substrs=["use of undeclared identifier '$200'"])
+
+        # Try redeclaring the persistent variable with the same type.
+        # This should be rejected as we treat them as if they are globals.
+        self.expect("expr int $i = 123", error=True,
+                    substrs=["error: redefinition of persistent variable '$i'"])
+        self.expect_expr("$i", result_type="int", result_value="5")
+
+        # Try redeclaring the persistent variable with another type. Should
+        # also be rejected.
+        self.expect("expr long $i = 123", error=True,
+                    substrs=["error: redefinition of persistent variable '$i'"])
+        self.expect_expr("$i", result_type="int", result_value="5")
+
+        # Try assigning the persistent variable a new value.
+        self.expect("expr $i = 55")
+        self.expect_expr("$i", result_type="int", result_value="55")