[Lldb-commits] [lldb] r358261 - PDBFPO: Improvements to the AST visitor
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Fri Apr 12 00:19:00 PDT 2019
Author: labath
Date: Fri Apr 12 00:19:00 2019
New Revision: 358261
URL: http://llvm.org/viewvc/llvm-project?rev=358261&view=rev
Log:
PDBFPO: Improvements to the AST visitor
Summary:
This patch attempts to solve two issues made this code hard to follow
for me.
The first issue was that a lot of what these visitors do is mutate the
AST. The visitor pattern is not particularly good for that because by
the time you have performed the dynamic type dispatch, it's too late to
go back to the parent node, and change its pointer. The previous code
dealt with that relatively elegantly, but it still meant that one had to
perform manual type checks, which is what the visitor pattern is
supposed to avoid.
The second issue was not being able to return values from the Visit
functions, which meant that one had to store function results in member
variables (a common problem with visitor patterns).
Here, I solve both problems by making the visitor use a type switch
instead of going through double dispatch on the visited object. This
allows one to parameterize the visitor based on the return type and pass
function results as function results. The mutation is fascilitated by
having each Visit function take two arguments -- a reference to the
object itself (with the correct dynamic type), and a reference to the
parent's pointer to this object.
Although this wasn't my explicit goal here, the fact that we're not
using virtual dispatch anymore allows us to make the AST nodes
trivially destructible, which is a good thing, since we were not
destroying them anyway.
Reviewers: aleksandr.urakov, amccarth
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D60410
Modified:
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
Modified: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp?rev=358261&r1=358260&r2=358261&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp Fri Apr 12 00:19:00 2019
@@ -25,12 +25,11 @@ using namespace lldb_private;
namespace {
-class FPOProgramNode;
-class FPOProgramASTVisitor;
-
class NodeAllocator {
public:
template <typename T, typename... Args> T *makeNode(Args &&... args) {
+ static_assert(std::is_trivially_destructible<T>::value,
+ "This object will not be destroyed!");
void *new_node_mem = m_alloc.Allocate(sizeof(T), alignof(T));
return new (new_node_mem) T(std::forward<Args>(args)...);
}
@@ -53,9 +52,6 @@ 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:
@@ -67,8 +63,6 @@ public:
FPOProgramNodeSymbol(llvm::StringRef name)
: FPOProgramNode(Symbol), m_name(name) {}
- void Accept(FPOProgramASTVisitor &visitor) override;
-
llvm::StringRef GetName() const { return m_name; }
static bool classof(const FPOProgramNode *node) {
@@ -84,8 +78,6 @@ public:
FPOProgramNodeRegisterRef(uint32_t lldb_reg_num)
: FPOProgramNode(Register), m_lldb_reg_num(lldb_reg_num) {}
- void Accept(FPOProgramASTVisitor &visitor) override;
-
uint32_t GetLLDBRegNum() const { return m_lldb_reg_num; }
static bool classof(const FPOProgramNode *node) {
@@ -101,8 +93,6 @@ public:
FPOProgramNodeIntegerLiteral(uint32_t value)
: FPOProgramNode(IntegerLiteral), m_value(value) {}
- void Accept(FPOProgramASTVisitor &visitor) override;
-
uint32_t GetValue() const { return m_value; }
static bool classof(const FPOProgramNode *node) {
@@ -126,8 +116,6 @@ public:
: FPOProgramNode(BinaryOp), m_op_type(op_type), m_left(&left),
m_right(&right) {}
- void Accept(FPOProgramASTVisitor &visitor) override;
-
OpType GetOpType() const { return m_op_type; }
const FPOProgramNode *Left() const { return m_left; }
@@ -155,8 +143,6 @@ public:
FPOProgramNodeUnaryOp(OpType op_type, FPOProgramNode &operand)
: FPOProgramNode(UnaryOp), m_op_type(op_type), m_operand(&operand) {}
- void Accept(FPOProgramASTVisitor &visitor) override;
-
OpType GetOpType() const { return m_op_type; }
const FPOProgramNode *Operand() const { return m_operand; }
@@ -171,84 +157,108 @@ private:
FPOProgramNode *m_operand;
};
+template <typename ResultT = void>
class FPOProgramASTVisitor {
-public:
- virtual ~FPOProgramASTVisitor() = default;
+protected:
+ virtual ResultT Visit(FPOProgramNodeBinaryOp &binary,
+ FPOProgramNode *&ref) = 0;
+ virtual ResultT Visit(FPOProgramNodeUnaryOp &unary, FPOProgramNode *&ref) = 0;
+ virtual ResultT Visit(FPOProgramNodeRegisterRef ®, FPOProgramNode *&) = 0;
+ virtual ResultT Visit(FPOProgramNodeIntegerLiteral &integer,
+ FPOProgramNode *&) = 0;
+ virtual ResultT Visit(FPOProgramNodeSymbol &symbol, FPOProgramNode *&ref) = 0;
+
+ ResultT Dispatch(FPOProgramNode *&node) {
+ switch (node->GetKind()) {
+ case FPOProgramNode::Register:
+ return Visit(llvm::cast<FPOProgramNodeRegisterRef>(*node), node);
+ case FPOProgramNode::Symbol:
+ return Visit(llvm::cast<FPOProgramNodeSymbol>(*node), node);
+
+ case FPOProgramNode::IntegerLiteral:
+ return Visit(llvm::cast<FPOProgramNodeIntegerLiteral>(*node), node);
+ case FPOProgramNode::UnaryOp:
+ return Visit(llvm::cast<FPOProgramNodeUnaryOp>(*node), node);
+ case FPOProgramNode::BinaryOp:
+ return Visit(llvm::cast<FPOProgramNodeBinaryOp>(*node), node);
+ }
+ llvm_unreachable("Fully covered switch!");
+ }
- 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);
-}
+class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
+public:
+ void Visit(FPOProgramNodeBinaryOp &binary, FPOProgramNode *&) override {
+ Dispatch(binary.Left());
+ Dispatch(binary.Right());
+ }
-void FPOProgramNodeIntegerLiteral::Accept(FPOProgramASTVisitor &visitor) {
- visitor.Visit(*this);
-}
+ void Visit(FPOProgramNodeUnaryOp &unary, FPOProgramNode *&) override {
+ Dispatch(unary.Operand());
+ }
-void FPOProgramNodeBinaryOp::Accept(FPOProgramASTVisitor &visitor) {
- visitor.Visit(*this);
-}
+ void Visit(FPOProgramNodeRegisterRef &, FPOProgramNode *&) override {}
+ void Visit(FPOProgramNodeIntegerLiteral &, FPOProgramNode *&) override {}
+ void Visit(FPOProgramNodeSymbol &symbol, FPOProgramNode *&ref) override;
-void FPOProgramNodeUnaryOp::Accept(FPOProgramASTVisitor &visitor) {
- visitor.Visit(*this);
-}
+ static void Merge(const llvm::DenseMap<llvm::StringRef, FPOProgramNode *>
+ &dependent_programs,
+ FPOProgramNode *&ast) {
+ FPOProgramASTVisitorMergeDependent(dependent_programs).Dispatch(ast);
+ }
-class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor {
-public:
+private:
FPOProgramASTVisitorMergeDependent(
const llvm::DenseMap<llvm::StringRef, FPOProgramNode *>
&dependent_programs)
: m_dependent_programs(dependent_programs) {}
- void Merge(FPOProgramNode *&node_ref);
-
-private:
- void Visit(FPOProgramNodeBinaryOp &node) override;
- void Visit(FPOProgramNodeUnaryOp &node) override;
-
- void TryReplace(FPOProgramNode *&node_ref) const;
-
-private:
const llvm::DenseMap<llvm::StringRef, FPOProgramNode *> &m_dependent_programs;
};
-void FPOProgramASTVisitorMergeDependent::Merge(FPOProgramNode *&node_ref) {
- TryReplace(node_ref);
- node_ref->Accept(*this);
-}
+void FPOProgramASTVisitorMergeDependent::Visit(FPOProgramNodeSymbol &symbol,
+ FPOProgramNode *&ref) {
-void FPOProgramASTVisitorMergeDependent::Visit(FPOProgramNodeBinaryOp &node) {
- Merge(node.Left());
- Merge(node.Right());
-}
-void FPOProgramASTVisitorMergeDependent::Visit(FPOProgramNodeUnaryOp &node) {
- Merge(node.Operand());
+ auto it = m_dependent_programs.find(symbol.GetName());
+ if (it == m_dependent_programs.end())
+ return;
+
+ ref = it->second;
+ Dispatch(ref);
}
-void FPOProgramASTVisitorMergeDependent::TryReplace(
- FPOProgramNode *&node_ref) const {
+class FPOProgramASTVisitorResolveRegisterRefs
+ : public FPOProgramASTVisitor<bool> {
+public:
+ static bool Resolve(const llvm::DenseMap<llvm::StringRef, FPOProgramNode *>
+ &dependent_programs,
+ llvm::Triple::ArchType arch_type, NodeAllocator &alloc,
+ FPOProgramNode *&ast) {
+ return FPOProgramASTVisitorResolveRegisterRefs(dependent_programs,
+ arch_type, alloc)
+ .Dispatch(ast);
+ }
- while (auto *symbol = llvm::dyn_cast<FPOProgramNodeSymbol>(node_ref)) {
- auto it = m_dependent_programs.find(symbol->GetName());
- if (it == m_dependent_programs.end()) {
- break;
- }
+ bool Visit(FPOProgramNodeBinaryOp &binary, FPOProgramNode *&) override {
+ return Dispatch(binary.Left()) && Dispatch(binary.Right());
+ }
- node_ref = it->second;
+ bool Visit(FPOProgramNodeUnaryOp &unary, FPOProgramNode *&) override {
+ return Dispatch(unary.Operand());
}
-}
-class FPOProgramASTVisitorResolveRegisterRefs : public FPOProgramASTVisitor {
-public:
+ bool Visit(FPOProgramNodeRegisterRef &, FPOProgramNode *&) override {
+ return true;
+ }
+
+ bool Visit(FPOProgramNodeIntegerLiteral &, FPOProgramNode *&) override {
+ return true;
+ }
+
+ bool Visit(FPOProgramNodeSymbol &symbol, FPOProgramNode *&ref) override;
+
+private:
FPOProgramASTVisitorResolveRegisterRefs(
const llvm::DenseMap<llvm::StringRef, FPOProgramNode *>
&dependent_programs,
@@ -256,27 +266,11 @@ public:
: m_dependent_programs(dependent_programs), m_arch_type(arch_type),
m_alloc(alloc) {}
- bool Resolve(FPOProgramNode *&program);
-
-private:
- void Visit(FPOProgramNodeBinaryOp &node) override;
- void Visit(FPOProgramNodeUnaryOp &node) override;
-
- 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) {
- if (!TryReplace(program))
- return false;
- program->Accept(*this);
- return m_no_error_flag;
-}
-
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 =
@@ -294,62 +288,49 @@ static uint32_t ResolveLLDBRegisterNum(l
return npdb::GetLLDBRegisterNumber(arch_type, reg_id);
}
-bool FPOProgramASTVisitorResolveRegisterRefs::TryReplace(
- FPOProgramNode *&node_ref) {
- auto *symbol = llvm::dyn_cast<FPOProgramNodeSymbol>(node_ref);
- if (!symbol)
- return true;
-
+bool FPOProgramASTVisitorResolveRegisterRefs::Visit(
+ FPOProgramNodeSymbol &symbol, FPOProgramNode *&ref) {
// Look up register reference as lvalue in preceding assignments.
- auto it = m_dependent_programs.find(symbol->GetName());
+ auto it = m_dependent_programs.find(symbol.GetName());
if (it != m_dependent_programs.end()) {
// Dependent programs are handled elsewhere.
return true;
}
uint32_t reg_num =
- ResolveLLDBRegisterNum(symbol->GetName().drop_front(1), m_arch_type);
+ 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);
+ 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(
- FPOProgramNodeUnaryOp &node) {
- m_no_error_flag = Resolve(node.Operand());
-}
-
-class FPOProgramASTVisitorDWARFCodegen : public FPOProgramASTVisitor {
+class FPOProgramASTVisitorDWARFCodegen : public FPOProgramASTVisitor<> {
public:
- FPOProgramASTVisitorDWARFCodegen(Stream &stream) : m_out_stream(stream) {}
+ static void Emit(Stream &stream, FPOProgramNode *&ast) {
+ FPOProgramASTVisitorDWARFCodegen(stream).Dispatch(ast);
+ }
- void Emit(FPOProgramNode &program);
+ void Visit(FPOProgramNodeRegisterRef ®, FPOProgramNode *&);
+ void Visit(FPOProgramNodeBinaryOp &binary, FPOProgramNode *&);
+ void Visit(FPOProgramNodeUnaryOp &unary, FPOProgramNode *&);
+ void Visit(FPOProgramNodeSymbol &symbol, FPOProgramNode *&) {
+ llvm_unreachable("Symbols should have been resolved by now!");
+ }
+ void Visit(FPOProgramNodeIntegerLiteral &integer, FPOProgramNode *&);
private:
- void Visit(FPOProgramNodeRegisterRef &node) override;
- void Visit(FPOProgramNodeIntegerLiteral &node) override;
- void Visit(FPOProgramNodeBinaryOp &node) override;
- void Visit(FPOProgramNodeUnaryOp &node) override;
+ FPOProgramASTVisitorDWARFCodegen(Stream &stream) : m_out_stream(stream) {}
-private:
Stream &m_out_stream;
};
-void FPOProgramASTVisitorDWARFCodegen::Emit(FPOProgramNode &program) {
- program.Accept(*this);
-}
-
-void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeRegisterRef &node) {
+void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeRegisterRef ®,
+ FPOProgramNode *&) {
- uint32_t reg_num = node.GetLLDBRegNum();
+ uint32_t reg_num = reg.GetLLDBRegNum();
lldbassert(reg_num != LLDB_INVALID_REGNUM);
if (reg_num > 31) {
@@ -362,18 +343,18 @@ void FPOProgramASTVisitorDWARFCodegen::V
}
void FPOProgramASTVisitorDWARFCodegen::Visit(
- FPOProgramNodeIntegerLiteral &node) {
- uint32_t value = node.GetValue();
+ FPOProgramNodeIntegerLiteral &integer, FPOProgramNode *&) {
+ uint32_t value = integer.GetValue();
m_out_stream.PutHex8(DW_OP_constu);
m_out_stream.PutULEB128(value);
}
-void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeBinaryOp &node) {
+void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeBinaryOp &binary,
+ FPOProgramNode *&) {
+ Dispatch(binary.Left());
+ Dispatch(binary.Right());
- Emit(*node.Left());
- Emit(*node.Right());
-
- switch (node.GetOpType()) {
+ switch (binary.GetOpType()) {
case FPOProgramNodeBinaryOp::Plus:
m_out_stream.PutHex8(DW_OP_plus);
// NOTE: can be optimized by using DW_OP_plus_uconst opcpode
@@ -395,10 +376,11 @@ void FPOProgramASTVisitorDWARFCodegen::V
}
}
-void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeUnaryOp &node) {
- Emit(*node.Operand());
+void FPOProgramASTVisitorDWARFCodegen::Visit(FPOProgramNodeUnaryOp &unary,
+ FPOProgramNode *&) {
+ Dispatch(unary.Operand());
- switch (node.GetOpType()) {
+ switch (unary.GetOpType()) {
case FPOProgramNodeUnaryOp::Deref:
m_out_stream.PutHex8(DW_OP_deref);
break;
@@ -523,19 +505,16 @@ static FPOProgramNode *ParseFPOProgram(l
lldbassert(rvalue_ast);
// check & resolve assignment program
- FPOProgramASTVisitorResolveRegisterRefs resolver(dependent_programs,
- arch_type, alloc);
- if (!resolver.Resolve(rvalue_ast)) {
+ if (!FPOProgramASTVisitorResolveRegisterRefs::Resolve(
+ dependent_programs, arch_type, alloc, 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
// from predecessors
- FPOProgramASTVisitorMergeDependent merger(dependent_programs);
- merger.Merge(rvalue_ast);
+ FPOProgramASTVisitorMergeDependent::Merge(dependent_programs, rvalue_ast);
return rvalue_ast;
}
@@ -557,7 +536,6 @@ bool lldb_private::npdb::TranslateFPOPro
return false;
}
- FPOProgramASTVisitorDWARFCodegen codegen(stream);
- codegen.Emit(*target_program);
+ FPOProgramASTVisitorDWARFCodegen::Emit(stream, target_program);
return true;
}
More information about the lldb-commits
mailing list