[lldb] Fix crash due to unicode characters and dollars in variable names.

[teemperor]

Summary

Our variable loading optimization uses some handmade lexer that currently doesn't
properly handle characters such as dollar-symbols or unicode characters. This patch
replaces our own lexer with the Clang lexer which properly handles all these corner cases.

[teemperor]

I realized after revisiting this code that this can't handle Unicode and we probably should just use Clang's Lexer to get the tokens. Still, it would be nice if we can land this in case the Lexer rewrite takes longer than expected.

[davide]

lldb/packages/Python/lldbsuite/test/expression_command/dollar-in-variable/main.c

5–6

what if you have another variable in the same scope named just foo ?
What expr $foo will print?
I would expect this to be recognized as expr $$foo.

[davide]

lldb/packages/Python/lldbsuite/test/expression_command/dollar-in-variable/main.c

5–6

Actually, I think the previous example should be fine, on a second thought. The example that I think might break is the one where you have an identifier in your source code named $R0. Is that allowed?

[teemperor]

lldb/packages/Python/lldbsuite/test/expression_command/dollar-in-variable/main.c

5–6

Is $R0 supposed to access a register? Or is that somehow a special syntax?

In any case the current code will prefer a local variable with from what I can see.

[davide]

I thought about this a little more, and we discussed offline, but these are my two comments (we should understand what's the behaviour and add tests). We can address some of them as follow-up but I would like to understand what happens here.

1. For C++, lldb names expression results using the convention $0, $1, etc... Can you have a variable named $0 in your source code? If so, what happens when you evaluate expr patatino? Will lldb be smart enough to understand that $0 is already defined and start with $1 ? Or it will override the scope?
2. If you have $pat in your example, and you type expr int $pat = 234234, does that work?

[clayborg]

Do we really expect users to actually write code where variables start with '$'? We will need to make some clear rules of the lookup order if so. I would suggest the following order

• look for real variables by exact name match first
• look for expression global variables next
• look for expression results next ($<digit>)
• look for registers

lldb/packages/Python/lldbsuite/test/expression_command/dollar-in-variable/main.c

6

The idea was that a "$" prefix wouldn't be found in normal code. In LLDB $ has the following special use cases:

• all expression results are named $<digit> just like LLDB
• if you use $ it can be followed by a register name so you can access registers in expressions ($rax, $sp, etc)
• any definitions in your expression where the variable is prefixed with '$' will become an expression global variable available to all subsequent expressions:

(lldb) expr int $my_global = 12;
(lldb) expr ++$my_global

• All function names that start with '$' are currently thought to be reserved for expression and so that any functions we create don't interact with real functions

[teemperor]

So the actual code that hit his use case was when one steps into the expression wrapper function we create in LLDB (which uses these dollar signs). So we (luckily) don't have any user code that triggers this.

I should note that I anyway discovered that the variable loading optimization that caused this regression doesn't support any unicode characters (not that we encourage that), so I'll anyway rewrite this parsing code with something that reuses the Clang lexer instead of our own ad-hoc parsing (which probably wil save us from more weird corner cases in the future).

[teemperor]

• Rewrote the patch to use the Clang lexer, dropped our ad-hoc lexer.
• Make sure we only lex the expression once, not for each local variable.
• Added tests for unicode characters and a bunch of dollar-related corner cases. The current behavior seems reasonable and what people expect in the comments, so I just keep them around.

I would prefer if we could just consider this patch as a regression fix and not some official support for dollar-variables in user code (which seems like a mess to define properly). I added some tests that at least make sure the current behavior is tested, we can decide later if that's really what we want to do (but the current behavior seems quite reasonable and similar to what people suggest).

[JDevlieghere]

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

223

Can we write this as while(!lex.LexFromRawLexer(token))?

[teemperor]

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

223

LexFromRawLexer seems to return true when the current result is the last token (but still a valid token), so I think no :(

[JDevlieghere]

LGTM

[teemperor]

Closed by 365698

lldb/packages/Python/lldbsuite/test/expression_command/dollar-in-variable/Makefile

+LEVEL = ../../make
+C_SOURCES := main.c
+include $(LEVEL)/Makefile.rules

lldb/packages/Python/lldbsuite/test/expression_command/dollar-in-variable/TestDollarInVariable.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals(), None)
+

lldb/packages/Python/lldbsuite/test/expression_command/dollar-in-variable/main.c

+// Make sure we correctly handle $ in variable names.
+
+int main() {
+  // Some variables that might conflict with our variables below.
+  int __lldb_expr_result = 2;
+  int $$foo = 1;
+  int R0 = 2;
+
+  // Some variables with dollar signs that should work (and shadow
+  // any built-in LLDB variables).
+  int $__lldb_expr_result = 11;
+  int $foo = 12;
+  int $R0 = 13;
+  int $0 = 14;
+
+  //%self.expect("expr $__lldb_expr_result", substrs=['(int) $0 = 11'])
+  //%self.expect("expr $foo", substrs=['(int)', ' = 12'])
+  //%self.expect("expr $R0", substrs=['(int)', ' = 13'])
+  //%self.expect("expr int $foo = 123", error=True, substrs=["declaration conflicts"])
+  return 0; //%self.expect("expr $0", substrs=['(int)', ' = 14'])
+}

lldb/packages/Python/lldbsuite/test/expression_command/unicode-in-variable/Makefile

+LEVEL = ../../make
+CXX_SOURCES := main.cpp
+CXX_FLAGS_EXTRA := -finput-charset=UTF-8 -fextended-identifiers
+include $(LEVEL)/Makefile.rules

lldb/packages/Python/lldbsuite/test/expression_command/unicode-in-variable/TestUnicodeInVariable.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals(), None)
+

lldb/packages/Python/lldbsuite/test/expression_command/unicode-in-variable/main.cpp

+// Make sure we correctly handle unicode in variable names.
+
+struct A {
+  // We need a member variable in the context that could shadow our local
+  // variable. If our optimization code fails to handle this, then we won't
+  // correctly inject our local variable so that it won't get shadowed.
+  int foob\u00E1r = 2;
+  int foo() {
+    int foob\u00E1r = 3;
+    return foob\u00E1r; //%self.expect("expr foobár", substrs=['(int)', ' = 3'])
+  }
+};
+
+int main() {
+  A a;
+  return a.foo();
+}

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

 //===-- ClangExpressionSourceCode.cpp ---------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangExpressionSourceCode.h"
 
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringRef.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
 #include "Plugins/ExpressionParser/Clang/ClangPersistentVariables.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/DebugMacros.h"
 #include "lldb/Symbol/TypeSystem.h"
case DebugMacroEntry::INDIRECT:
       break;
     default:
       // This is an unknown/invalid entry. Ignore.
       break;
     }
   }
 }
 
-/// Checks if the expression body contains the given variable as a token.
-/// \param body The expression body.
-/// \param var The variable token we are looking for.
-/// \return True iff the expression body containes the variable as a token.
-static bool ExprBodyContainsVar(llvm::StringRef body, llvm::StringRef var) {
-  assert(var.find_if([](char c) { return !clang::isIdentifierBody(c); }) ==
-             llvm::StringRef::npos &&
-         "variable contains non-identifier chars?");
-
-  size_t start = 0;
-  // Iterate over all occurences of the variable string in our expression.
-  while ((start = body.find(var, start)) != llvm::StringRef::npos) {
-    // We found our variable name in the expression. Check that the token
-    // that contains our needle is equal to our variable and not just contains
-    // the character sequence by accident.
-    // Prevents situations where we for example inlcude the variable 'FOO' in an
-    // expression like 'FOObar + 1'.
-    bool has_characters_before =
-        start != 0 && clang::isIdentifierBody(body[start - 1]);
-    bool has_characters_after =
-        start + var.size() < body.size() &&
-        clang::isIdentifierBody(body[start + var.size()]);
-
-    // Our token just contained the variable name as a substring. Continue
-    // searching the rest of the expression.
-    if (has_characters_before || has_characters_after) {
-      ++start;
-      continue;
-    }
-    return true;
+namespace {
+/// Allows checking if a token is contained in a given expression.
+class TokenVerifier {
+  /// The tokens we found in the expression.
+  llvm::StringSet<> m_tokens;
+
+public:
+  TokenVerifier(std::string body);
+  /// Returns true iff the given expression body contained a token with the
+  /// given content.
+  bool hasToken(llvm::StringRef token) const {
+    return m_tokens.find(token) != m_tokens.end();
+};
+} // namespace
+
+TokenVerifier::TokenVerifier(std::string body) {
+  using namespace clang;
+
+  // We only care about tokens and not their original source locations. If we
+  // move the whole expression to only be in one line we can simplify the
+  // following code that extracts the token contents.
+  std::replace(body.begin(), body.end(), '\n', ' ');
+  std::replace(body.begin(), body.end(), '\r', ' ');
+
+  FileSystemOptions file_opts;
+  FileManager file_mgr(file_opts,
+                       FileSystem::Instance().GetVirtualFileSystem());
+
+  // Let's build the actual source code Clang needs and setup some utility
+  // objects.
+  llvm::IntrusiveRefCntPtr<DiagnosticIDs> diag_ids(new DiagnosticIDs());
+  llvm::IntrusiveRefCntPtr<DiagnosticOptions> diags_opts(
+      new DiagnosticOptions());
+  DiagnosticsEngine diags(diag_ids, diags_opts);
+  clang::SourceManager SM(diags, file_mgr);
+  auto buf = llvm::MemoryBuffer::getMemBuffer(body);
+
+  FileID FID = SM.createFileID(clang::SourceManager::Unowned, buf.get());
+
+  // Let's just enable the latest ObjC and C++ which should get most tokens
+  // right.
+  LangOptions Opts;
+  Opts.ObjC = true;
+  Opts.DollarIdents = true;
+  Opts.CPlusPlus17 = true;
+  Opts.LineComment = true;
+
+  Lexer lex(FID, buf.get(), SM, Opts);
+
+  Token token;
+  bool exit = false;
+  while (!exit) {
+    // Returns true if this is the last token we get from the lexer.
+    exit = lex.LexFromRawLexer(token);
+
+    // Extract the column number which we need to extract the token content.
+    // Our expression is just one line, so we don't need to handle any line
+    // numbers here.
+    bool invalid = false;
+    unsigned start = SM.getSpellingColumnNumber(token.getLocation(), &invalid);
+    if (invalid)
+      continue;
+    // Column numbers start at 1, but indexes in our string start at 0.
+    --start;
+
+    // Annotations don't have a length, so let's skip them.
+    if (token.isAnnotation())
+      continue;
+
+    // Extract the token string from our source code and store it.
+    m_tokens.insert(body.substr(start, token.getLength()));
   }
-  return false;
 }
 
 static void AddLocalVariableDecls(const lldb::VariableListSP &var_list_sp,
                                   StreamString &stream,
                                   const std::string &expr,
                                   lldb::LanguageType wrapping_language) {
+  TokenVerifier tokens(expr);
+
   for (size_t i = 0; i < var_list_sp->GetSize(); i++) {
     lldb::VariableSP var_sp = var_list_sp->GetVariableAtIndex(i);
 
     ConstString var_name = var_sp->GetName();
 
 
     // We can check for .block_descriptor w/o checking for langauge since this
     // is not a valid identifier in either C or C++.
     if (!var_name || var_name == ".block_descriptor")
       continue;
 
-    if (!expr.empty() && !ExprBodyContainsVar(expr, var_name.GetStringRef()))
+    if (!expr.empty() && !tokens.hasToken(var_name.GetStringRef()))
       continue;
 
     if ((var_name == "self" || var_name == "_cmd") &&
         (wrapping_language == lldb::eLanguageTypeObjC ||
          wrapping_language == lldb::eLanguageTypeObjC_plus_plus))
       continue;
 
     if (var_name == "this" &&