PDBFPO: Refactor register reference resolution

[labath]

Summary

This refactors moves the register name->number resolution out of the
FPOProgramNodeRegisterRef class. Instead I create a special
FPOProgramNodeSymbol class, which holds unresolved symbols, and move the
resolution into the ResolveRegisterRefs visitor.

The background here is that I'd like to use this code for Breakpad
unwind info, which uses similar syntax to describe unwind info. For
example, a simple breakpad unwind program might look like:

.cfa: $esp 8 + $ebp: .cfa 8 - ^

To be able to do this, I need to be able to customize register
resolving, as that is presently hardcoded to use codeview register
names, but breakpad supports a lot more architectures with different
register names. Moving the resolution into a separate class will allow
each user to use a different resolution logic.

[amccarth]

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

287

nit: As long as you're correcting this comment, can you fix "look up" to be two words?

[aleksandr.urakov]

LGTM!

[labath]

Thanks.

BTW, do you have any thoughts where this code could live (it can't stay here because I need to access it from SymbolFileBreakpad too). I was thinking of the Symbol library, as both users are SymbolFiles.

I don't think this will have any external dependencies (that I can't get rid of), so another option might be Utility, but that seems a bit too low for this functionality (it will include code for generating dwarf expressions).

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

287

will do.

[aleksandr.urakov]

I think both approaches are acceptable, but I would prefer to include it into the Symbols library because this functionality is more symbols-related than general.

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

 using namespace lldb;
 using namespace lldb_private;
 
 namespace {
 
 class FPOProgramNode;
 class FPOProgramASTVisitor;
 
+class NodeAllocator {
+public:
+  template <typename T, typename... Args> T *makeNode(Args &&... args) {
+    void *new_node_mem = m_alloc.Allocate(sizeof(T), alignof(T));
+    return new (new_node_mem) T(std::forward<Args>(args)...);
+  }
+
+private:
+  llvm::BumpPtrAllocator m_alloc;
+};
+
 class FPOProgramNode {
 public:
   enum Kind {
+    Symbol,
     Register,
     IntegerLiteral,
     BinaryOp,
     UnaryOp,
   };
 
 protected:
   FPOProgramNode(Kind kind) : m_token_kind(kind) {}
 
 public:
   virtual ~FPOProgramNode() = default;
   virtual void Accept(FPOProgramASTVisitor *visitor) = 0;
 
   Kind GetKind() const { return m_token_kind; }
 
 private:
   Kind m_token_kind;
 };
 
-class FPOProgramNodeRegisterRef : public FPOProgramNode {
+class FPOProgramNodeSymbol: public FPOProgramNode {
 public:
-  FPOProgramNodeRegisterRef(llvm::StringRef name)
-      : FPOProgramNode(Register), m_name(name) {}
+  FPOProgramNodeSymbol(llvm::StringRef name)
+      : FPOProgramNode(Symbol), m_name(name) {}
 
   void Accept(FPOProgramASTVisitor *visitor) override;
 
   llvm::StringRef GetName() const { return m_name; }
-  uint32_t GetLLDBRegNum() const { return m_lldb_reg_num; }
-
-  bool ResolveLLDBRegisterNum(llvm::Triple::ArchType arch_type);
 
 private:
   llvm::StringRef m_name;
-  uint32_t m_lldb_reg_num = LLDB_INVALID_REGNUM;
 };
 
-bool FPOProgramNodeRegisterRef::ResolveLLDBRegisterNum(
-    llvm::Triple::ArchType arch_type) {
-
-  llvm::StringRef reg_name = m_name.slice(1, m_name.size());
-
-  // lookup register name to get lldb register number
-  llvm::ArrayRef<llvm::EnumEntry<uint16_t>> register_names =
-      llvm::codeview::getRegisterNames();
-  auto it = llvm::find_if(
-      register_names,
-      [&reg_name](const llvm::EnumEntry<uint16_t> &register_entry) {
-        return reg_name.compare_lower(register_entry.Name) == 0;
-      });
+class FPOProgramNodeRegisterRef : public FPOProgramNode {
+public:
+  FPOProgramNodeRegisterRef(uint32_t lldb_reg_num)
+      : FPOProgramNode(Register), m_lldb_reg_num(lldb_reg_num) {}
 
-  if (it == register_names.end()) {
-    return false;
-  }
+  void Accept(FPOProgramASTVisitor *visitor) override;
 
-  auto reg_id = static_cast<llvm::codeview::RegisterId>(it->Value);
-  m_lldb_reg_num = npdb::GetLLDBRegisterNumber(arch_type, reg_id);
+  uint32_t GetLLDBRegNum() const { return m_lldb_reg_num; }
 
-  return m_lldb_reg_num != LLDB_INVALID_REGNUM;
-}
+private:
+  uint32_t m_lldb_reg_num;
+};
 
 class FPOProgramNodeIntegerLiteral : public FPOProgramNode {
 public:
   FPOProgramNodeIntegerLiteral(uint32_t value)
       : FPOProgramNode(IntegerLiteral), m_value(value) {}
 
   void Accept(FPOProgramASTVisitor *visitor) override;
 
private:
   OpType m_op_type;
   FPOProgramNode *m_operand;
 };
 
 class FPOProgramASTVisitor {
 public:
   virtual ~FPOProgramASTVisitor() = default;
 
+  virtual void Visit(FPOProgramNodeSymbol *node) {}
   virtual void Visit(FPOProgramNodeRegisterRef *node) {}
   virtual void Visit(FPOProgramNodeIntegerLiteral *node) {}
   virtual void Visit(FPOProgramNodeBinaryOp *node) {}
   virtual void Visit(FPOProgramNodeUnaryOp *node) {}
 };
 
+void FPOProgramNodeSymbol::Accept(FPOProgramASTVisitor *visitor) {
+  visitor->Visit(this);
+}
+
 void FPOProgramNodeRegisterRef::Accept(FPOProgramASTVisitor *visitor) {
   visitor->Visit(this);
 }
 
 void FPOProgramNodeIntegerLiteral::Accept(FPOProgramASTVisitor *visitor) {
   visitor->Visit(this);
 }
 

 }
 void FPOProgramASTVisitorMergeDependent::Visit(FPOProgramNodeUnaryOp *node) {
   Merge(node->Operand());
 }
 
 void FPOProgramASTVisitorMergeDependent::TryReplace(
     FPOProgramNode *&node_ref) const {
 
-  while (node_ref->GetKind() == FPOProgramNode::Register) {
-    auto *node_register_ref =
-        static_cast<FPOProgramNodeRegisterRef *>(node_ref);
+  while (node_ref->GetKind() == FPOProgramNode::Symbol) {
+    auto *node_symbol_ref = static_cast<FPOProgramNodeSymbol *>(node_ref);
 
-    auto it = m_dependent_programs.find(node_register_ref->GetName());
+    auto it = m_dependent_programs.find(node_symbol_ref->GetName());
     if (it == m_dependent_programs.end()) {
       break;
     }
 
     node_ref = it->second;
   }
 }
 
 class FPOProgramASTVisitorResolveRegisterRefs : public FPOProgramASTVisitor {
 public:
   FPOProgramASTVisitorResolveRegisterRefs(
       const llvm::DenseMap<llvm::StringRef, FPOProgramNode *>
           &dependent_programs,
-      llvm::Triple::ArchType arch_type)
-      : m_dependent_programs(dependent_programs), m_arch_type(arch_type) {}
+      llvm::Triple::ArchType arch_type, NodeAllocator &alloc)
+      : m_dependent_programs(dependent_programs), m_arch_type(arch_type),
+        m_alloc(alloc) {}
 
-  bool Resolve(FPOProgramNode *program);
+  bool Resolve(FPOProgramNode *&program);
 
 private:
-  void Visit(FPOProgramNodeRegisterRef *node) override;
-  void Visit(FPOProgramNodeIntegerLiteral *node) override {}
   void Visit(FPOProgramNodeBinaryOp *node) override;
   void Visit(FPOProgramNodeUnaryOp *node) override;
 
-private:
+  bool TryReplace(FPOProgramNode *&node_ref);
+
   const llvm::DenseMap<llvm::StringRef, FPOProgramNode *> &m_dependent_programs;
   llvm::Triple::ArchType m_arch_type;
+  NodeAllocator &m_alloc;
   bool m_no_error_flag = true;
 };
 
-bool FPOProgramASTVisitorResolveRegisterRefs::Resolve(FPOProgramNode *program) {
+bool FPOProgramASTVisitorResolveRegisterRefs::Resolve(FPOProgramNode *&program) {
+  if (!TryReplace(program))
+    return false;
   program->Accept(this);
   return m_no_error_flag;
 }
 
-void FPOProgramASTVisitorResolveRegisterRefs::Visit(
-    FPOProgramNodeRegisterRef *node) {
+static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, llvm::Triple::ArchType arch_type) {
+  // lookup register name to get lldb register number
+  llvm::ArrayRef<llvm::EnumEntry<uint16_t>> register_names =
+      llvm::codeview::getRegisterNames();
+  auto it = llvm::find_if(
+      register_names,
+      [&reg_name](const llvm::EnumEntry<uint16_t> &register_entry) {
+        return reg_name.compare_lower(register_entry.Name) == 0;
+      });
+
+  if (it == register_names.end())
+    return LLDB_INVALID_REGNUM;
+
+  auto reg_id = static_cast<llvm::codeview::RegisterId>(it->Value);
+  return npdb::GetLLDBRegisterNumber(arch_type, reg_id);
+}
+
+bool FPOProgramASTVisitorResolveRegisterRefs::TryReplace(
+    FPOProgramNode *&node_ref) {
+  if (node_ref->GetKind() != FPOProgramNode::Symbol)
+    return true;
 
-  // lookup register reference as lvalue in predecedent assignments
-  auto it = m_dependent_programs.find(node->GetName());
+  auto *symbol = static_cast<FPOProgramNodeSymbol *>(node_ref);
+
+  // lookup register reference as lvalue in preceding assignments
+  auto it = m_dependent_programs.find(symbol->GetName());
   if (it != m_dependent_programs.end()) {
-    // dependent programs are already resolved and valid
-    return;
+    // dependent programs are handled elsewhere
+    return true;
   }
-  // try to resolve register reference as lldb register name
-  m_no_error_flag = node->ResolveLLDBRegisterNum(m_arch_type);
+
+  uint32_t reg_num =
+      ResolveLLDBRegisterNum(symbol->GetName().drop_front(1), m_arch_type);
+
+  if (reg_num == LLDB_INVALID_REGNUM)
+    return false;
+
+  node_ref = m_alloc.makeNode<FPOProgramNodeRegisterRef>(reg_num);
+  return true;
 }
 
 void FPOProgramASTVisitorResolveRegisterRefs::Visit(
     FPOProgramNodeBinaryOp *node) {
   m_no_error_flag = Resolve(node->Left()) && Resolve(node->Right());
 }
 
 void FPOProgramASTVisitorResolveRegisterRefs::Visit(
void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeUnaryOp *node) {
 
   switch (node->GetOpType()) {
   case FPOProgramNodeUnaryOp::Deref:
     m_out_stream.PutHex8(DW_OP_deref);
     break;
   }
 }
 
-class NodeAllocator {
-public:
-  template <typename T, typename... Args> T *makeNode(Args &&... args) {
-    void *new_node_mem = m_alloc.Allocate(sizeof(T), alignof(T));
-    return new (new_node_mem) T(std::forward<Args>(args)...);
-  }
-
-private:
-  llvm::BumpPtrAllocator m_alloc;
-};
-
 } // namespace
 
 static bool ParseFPOSingleAssignmentProgram(llvm::StringRef program,
                                             NodeAllocator &alloc,
                                             llvm::StringRef &register_name,
                                             FPOProgramNode *&ast) {
   llvm::SmallVector<llvm::StringRef, 16> tokens;
   llvm::SplitString(program, tokens, " ");
if (ops_unary_it != ops_unary.end()) {
       FPOProgramNode *operand = eval_stack.pop_back_val();
       FPOProgramNode *node =
           alloc.makeNode<FPOProgramNodeUnaryOp>(ops_unary_it->second, operand);
       eval_stack.push_back(node);
       continue;
     }
 
     if (cur.startswith("$")) {
-      // token is register ref
-      eval_stack.push_back(alloc.makeNode<FPOProgramNodeRegisterRef>(cur));
+      eval_stack.push_back(alloc.makeNode<FPOProgramNodeSymbol>(cur));
       continue;
     }
 
     if (cur == ra_search_keyword) {
       // TODO: .raSearch is unsupported
       return false;
     }
 
if (!ParseFPOSingleAssignmentProgram(assignment_program, alloc, lvalue_name,
                                          rvalue_ast)) {
       return nullptr;
     }
 
     lldbassert(rvalue_ast);
 
     // check & resolve assignment program
     FPOProgramASTVisitorResolveRegisterRefs resolver(dependent_programs,
-                                                     arch_type);
+                                                     arch_type, alloc);
     if (!resolver.Resolve(rvalue_ast)) {
       return nullptr;
     }
 
     if (lvalue_name == register_name) {
       // found target assignment program - no need to parse further
 
       // emplace valid dependent subtrees to make target assignment independent