[Lldb-commits] [lldb] [LLDB] Add DIL code for handling plain variable names. (PR #120971)

[Pavel Labath via lldb-commits]

================
@@ -0,0 +1,106 @@
+//===-- DILEval.h -----------------------------------------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_VALUEOBJECT_DILEVAL_H_
+#define LLDB_VALUEOBJECT_DILEVAL_H_
+
+#include "lldb/ValueObject/DILAST.h"
+#include "lldb/ValueObject/DILParser.h"
+#include <memory>
+#include <vector>
+
+namespace lldb_private {
+
+namespace dil {
+
+/// Class used to store & manipulate information about identifiers.
+class IdentifierInfo {
+public:
+  enum class Kind {
+    eValue,
+    eContextArg,
+  };
+
+  static std::unique_ptr<IdentifierInfo> FromValue(ValueObject &valobj) {
+    CompilerType type;
+    type = valobj.GetCompilerType();
+    return std::unique_ptr<IdentifierInfo>(
+        new IdentifierInfo(Kind::eValue, type, valobj.GetSP(), {}));
+  }
+
+  static std::unique_ptr<IdentifierInfo> FromContextArg(CompilerType type) {
+    lldb::ValueObjectSP empty_value;
+    return std::unique_ptr<IdentifierInfo>(
+        new IdentifierInfo(Kind::eContextArg, type, empty_value, {}));
+  }
+
+  Kind GetKind() const { return m_kind; }
+  lldb::ValueObjectSP GetValue() const { return m_value; }
+
+  CompilerType GetType() { return m_type; }
+  bool IsValid() const { return m_type.IsValid(); }
+
+  IdentifierInfo(Kind kind, CompilerType type, lldb::ValueObjectSP value,
+                 std::vector<uint32_t> path)
+      : m_kind(kind), m_type(type), m_value(std::move(value)) {}
+
+private:
+  Kind m_kind;
+  CompilerType m_type;
+  lldb::ValueObjectSP m_value;
+};
+
+/// Given the name of an identifier (variable name, member name, type name,
+/// etc.), find the ValueObject for that name (if it exists) and create and
+/// return an IdentifierInfo object containing all the relevant information
+/// about that object (for DIL parsing and evaluating).
+std::unique_ptr<IdentifierInfo> LookupIdentifier(
+    const std::string &name, std::shared_ptr<ExecutionContextScope> ctx_scope,
+    lldb::DynamicValueType use_dynamic, CompilerType *scope_ptr = nullptr);
+
+class DILInterpreter : Visitor {
+public:
+  DILInterpreter(lldb::TargetSP target, llvm::StringRef expr,
+                 lldb::DynamicValueType use_dynamic,
+                 std::shared_ptr<ExecutionContextScope> exe_ctx_scope);
+
+  llvm::Expected<lldb::ValueObjectSP> DILEval(const DILASTNode *tree,
+                                              lldb::TargetSP target_sp);
+
+private:
+  lldb::ValueObjectSP DILEvalNode(const DILASTNode *node);
+
+  bool Success() { return m_error.Success(); }
+
+  void SetError(ErrorCode error_code, std::string error, uint32_t loc);
+
+  void Visit(const ErrorNode *node) override;
+  void Visit(const IdentifierNode *node) override;
+
+private:
+  // Used by the interpreter to create objects, perform casts, etc.
+  lldb::TargetSP m_target;
+
+  llvm::StringRef m_expr;
+
+  lldb::ValueObjectSP m_result;
+
+  lldb::ValueObjectSP m_scope;
+
+  lldb::DynamicValueType m_default_dynamic;
+
+  Status m_error;
----------------
labath wrote:

I agree with Adrian here. We'd like to avoid Statuses in new code, and I don't think we need one here. The argument about changing the return type in uncommitted code doesn't carry much weight with me. I don't think you *need* to do it (more on that later), but if you were to do it, here's what I have to say about that:

> I would have to do error type conversions in functions where they call something that fails with llvm::expected of type A but the calling function returns llvm::expected of type B

The way you normally do that is
```
if (!expected_a) return expected_a.takeError(); // automatically converted to Expected<B>
// Do stuff with `a`
```

> add code to many of these sites to also update the current token to EOF

The existence of the current token is something I want to get rid of anyway, but one of the "nice" things about explicitly error passing is that you don't have to bother with maintaining state like this. If you return an Error, then the caller has to check it, and if all that the caller does is return immediately, then it doesn't really matter what's the state of the parser object -- the error will be bubbled up to the top of the stack, and nobody will touch the parser object.

>  It will actually make the code much messier and more complicated than it is now.

I'm not sure if this is the case here, but I can see how it might be. There are situations where it's simpler to "plough ahead" and check for errors later. However, I don't think this means you can't use Error/Expected. While it is not common to have an Error object as a member, it's not unheard of either. The main thing we want to avoid is *long-lived* Error objects with unclear *destruction/check semantics*.

Long-livedness isn't really a problem here since the Parser object only exists while the expression is being parsed. That may be longer than usual but it has clear start and end points (unlike some of our other Status member objects). And your destruction semantics ale already quite clear, albeit not completely explicit -- the error object is checked and returned in the `Run` function, which is also the only public entry point of the parser class. The thing that's missing is that nothing actually forces the caller to call the `Run` function (exactly once) -- and this is where I'd like to return to my earlier suggestion of using static methods for this, as I think this dovetails very nicely with this. If parsing consistent of a single `Parser::Parse` call, then there would be no way to create long-lived Parser or Error objects, and the existence of the Parser object (and its Error member, if any) would just be an implementation detail of the `Parse` method. And the `Error` object probably doesn't even have to be a member, because the `Parse` method can do something like this:
```
Expected<AST> Parser::Parse(...) {
  Error error;
  AST ast = Parser(..., /*takes the error by reference*/error).Run();
  if (error) return error;
  return ast;
}
```

https://github.com/llvm/llvm-project/pull/120971