[lldb] Reject redefinitions of persistent variables

[teemperor]

Summary

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.

[labath]

Looks straight-forward enough.

[shafik]

makes sense, LGTM.

[JDevlieghere]

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

212

As a totally unnecessary micro-optimization, it seems like Diagnostic is taking a StringRef in it's ctor only to store it as a std::string. We should make that a std::string and then move that string into place instead of making two unnecessary copies. Of course that doesn't have to be part of this patch.

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

public:
   ///     the variables to use
   ///
   /// \return
   ///     True if parsing is possible; false if it is unsafe to continue.
   bool WillParse(ExecutionContext &exe_ctx, Materializer *materializer);
 
   void InstallCodeGenerator(clang::ASTConsumer *code_gen);
 
+  void InstallDiagnosticManager(DiagnosticManager &diag_manager);
+
   /// Disable the state needed for parsing and IR transformation.
   void DidParse();
 
   /// [Used by IRForTarget] Add a variable to the list of persistent
   ///     variables for the process.
   ///
   /// \param[in] decl
   ///     The Clang declaration for the persistent variable, used for
bool m_enable_lookups = false; ///< Set to true during parsing if we have
                                    ///found the first "$__lldb" name.
     TargetInfo m_target_info;      ///< Basic information about the target.
     Materializer *m_materializer = nullptr;   ///< If non-NULL, the materializer
                                               ///to use when reporting used
                                               ///variables.
     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;
   };
 
   std::unique_ptr<ParserVars> m_parser_vars;
 
   /// Activate parser-specific variables

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

 #include "ClangUtil.h"
 
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Core/Address.h"
 #include "lldb/Core/Module.h"
 #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"
 #include "lldb/Symbol/CompilerDeclContext.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/SymbolFile.h"

 }
 
 void ClangExpressionDeclMap::InstallCodeGenerator(
     clang::ASTConsumer *code_gen) {
   assert(m_parser_vars);
   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();
          entity_index < num_entities; ++entity_index) {
       ExpressionVariableSP var_sp(
           m_found_entities.GetVariableAtIndex(entity_index));
       if (var_sp)
         llvm::cast<ClangExpressionVariable>(var_sp.get())
bool ClangExpressionDeclMap::AddPersistentVariable(const NamedDecl *decl,
                                                    bool is_lvalue) {
   assert(m_parser_vars.get());
 
   TypeSystemClang *ast =
       llvm::dyn_cast_or_null<TypeSystemClang>(parser_type.GetTypeSystem());
   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;
 
     ExecutionContext &exe_ctx = m_parser_vars->m_exe_ctx;
     Target *target = exe_ctx.GetTargetPtr();
     if (target == nullptr)
       return false;
 

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

ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
   if (ast_context.getLangOpts().Modules) {
     m_compiler->createASTReader();
     m_ast_context->setSema(&m_compiler->getSema());
   }
 
   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();
 
     if (ast_context.getExternalSource()) {
       auto module_wrapper =
           new ExternalASTSourceWrapper(ast_context.getExternalSource());
 
       auto ast_source_wrapper = new ExternalASTSourceWrapper(ast_source);

lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py

def test_persistent_variables(self):
         self.expect_expr("$2", result_type="int", result_value="8")
 
         # Try assigning an non-existing persistent veriable with a numeric name.
         self.expect("expr int $200 = 3", error=True,
             substrs=["Error [IRForTarget]: Names starting with $0, $1, ... are reserved for use as result names"])
         # 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")