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; } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
