-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[LLDB] Add unary operators Dereference and AddressOf to DIL #134428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
58c4f08
36d4f95
d7fdf3f
336d59f
8b63dfd
b183a3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,13 @@ namespace lldb_private::dil { | |
enum class NodeKind { | ||
eErrorNode, | ||
eIdentifierNode, | ||
eUnaryOpNode, | ||
}; | ||
|
||
/// The Unary operators recognized by DIL. | ||
enum class UnaryOpKind { | ||
AddrOf, // "&" | ||
Deref, // "*" | ||
}; | ||
|
||
/// Forward declaration, for use in DIL AST nodes. Definition is at the very | ||
|
@@ -44,6 +51,8 @@ class ASTNode { | |
|
||
virtual llvm::Expected<lldb::ValueObjectSP> Accept(Visitor *v) const = 0; | ||
|
||
virtual bool is_rvalue() const { return false; } | ||
|
||
uint32_t GetLocation() const { return m_location; } | ||
NodeKind GetKind() const { return m_kind; } | ||
|
||
|
@@ -81,6 +90,27 @@ class IdentifierNode : public ASTNode { | |
std::string m_name; | ||
}; | ||
|
||
class UnaryOpNode : public ASTNode { | ||
public: | ||
UnaryOpNode(uint32_t location, UnaryOpKind kind, ASTNodeUP rhs) | ||
: ASTNode(location, NodeKind::eUnaryOpNode), m_kind(kind), | ||
m_rhs(std::move(rhs)) {} | ||
|
||
llvm::Expected<lldb::ValueObjectSP> Accept(Visitor *v) const override; | ||
bool is_rvalue() const override { return m_kind != UnaryOpKind::Deref; } | ||
|
||
UnaryOpKind kind() const { return m_kind; } | ||
ASTNode *rhs() const { return m_rhs.get(); } | ||
|
||
static bool classof(const ASTNode *node) { | ||
return node->GetKind() == NodeKind::eUnaryOpNode; | ||
} | ||
|
||
private: | ||
UnaryOpKind m_kind; | ||
ASTNodeUP m_rhs; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's kinda weird to have the right side of a unary expression (and it would get particularly weird if we end up having a postfix |
||
}; | ||
|
||
/// This class contains one Visit method for each specialized type of | ||
/// DIL AST node. The Visit methods are used to dispatch a DIL AST node to | ||
/// the correct function in the DIL expression evaluator for evaluating that | ||
|
@@ -90,6 +120,8 @@ class Visitor { | |
virtual ~Visitor() = default; | ||
virtual llvm::Expected<lldb::ValueObjectSP> | ||
Visit(const IdentifierNode *node) = 0; | ||
virtual llvm::Expected<lldb::ValueObjectSP> | ||
Visit(const UnaryOpNode *node) = 0; | ||
}; | ||
|
||
} // namespace lldb_private::dil | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -18,6 +18,22 @@ | |||||
|
||||||
namespace lldb_private::dil { | ||||||
|
||||||
static lldb::ValueObjectSP | ||||||
ArrayToPointerConversion(lldb::ValueObjectSP valobj, | ||||||
std::shared_ptr<ExecutionContextScope> ctx) { | ||||||
assert(valobj->IsArrayType() && | ||||||
"an argument to array-to-pointer conversion must be an array"); | ||||||
|
||||||
uint64_t addr = valobj->GetLoadAddress(); | ||||||
llvm::StringRef name = "result"; | ||||||
ExecutionContext exe_ctx; | ||||||
ctx->CalculateExecutionContext(exe_ctx); | ||||||
return ValueObject::CreateValueObjectFromAddress( | ||||||
name, addr, exe_ctx, | ||||||
valobj->GetCompilerType().GetArrayElementType(ctx.get()).GetPointerType(), | ||||||
/* do_deref */ false); | ||||||
} | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is an interesting question (and one that may deserve a PR of its own). For C, this makes sense, but I'm not sure if we want to do this for all languages. Also, the implementations assumes that an array is just a list of elements (i.e., that the address of the first array element is the same as the address of the array itself). This is true for C, but it may not be true for all languages that may want to support. I'm not sure what's the situation with Swift, but Go (which we don't support, but it makes a good point) has a first class "array slice" type, which is basically equivalent to Another problem here is that this ignores any data formatters the type may have. A user may have defined a synthetic child provider for the array type, and that provider may define (by providing a special For these reasons, I think this logic should go into
@jimingham, @adrian-prantl, do you concur, or have anything to add here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I concur. As a general rule, we want the evaluator to know as little as possible about how any operation is performed and delegate as much as possible to ValueObject methods. The evaluator can't know anything about the language of the values it is operating on or it will either only work for C-backed ValueObjects or end up cluttered with "if C/C++/swift/go/future_language" code, which we really want to avoid. However, ValueObjects always know what type system their type comes from, and so they can naturally do the right thing for their language. So we should defer to them whenever we're doing any non-trivial operations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite sure here, how would we go about implementing something like array to pointer conversion for C only (or any other language specific thing)? Do we just add an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well.. that's why I said it's an interesting question :) I don't think it should be a language check. It should be plumbed into the type system code.. somehow. Currently So, one option that might be acceptable (to me, others may disagree) would be to call this function on array types as well and then to check whether the result has the The alternative would be to create a new function that explicitly tells the type system that you're trying to dereference the value, and then it can do pretty much anything it wants. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any downside to the second option? It seems to me the general philosophy here is to have DIL do as little as possible and the type system as much as possible, since then we're asking the entity that actually knows what it is doing... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I make a separate PR and merge this one as is? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's definitely a job for a separate patch. To answer Jim's question, I don't see a specific downside to the second option. Just some open questions. I don't exactly know what the interface of the new methods should be for instance (maybe it should not return a ValueObject since GetChildCompilerTypeAtIndex does not either). Ideally, I also wouldn't want too much code duplication between this and GetChildCompilerTypeAtIndex. But generally, yes, I think this would be better. |
||||||
static lldb::ValueObjectSP LookupStaticIdentifier( | ||||||
VariableList &variable_list, std::shared_ptr<StackFrame> exe_scope, | ||||||
llvm::StringRef name_ref, llvm::StringRef unqualified_name) { | ||||||
|
@@ -206,10 +222,25 @@ Interpreter::Interpreter(lldb::TargetSP target, llvm::StringRef expr, | |||||
: m_target(std::move(target)), m_expr(expr), m_default_dynamic(use_dynamic), | ||||||
m_exe_ctx_scope(frame_sp) {} | ||||||
|
||||||
llvm::Expected<lldb::ValueObjectSP> Interpreter::Evaluate(const ASTNode *node) { | ||||||
llvm::Expected<lldb::ValueObjectSP> Interpreter::Evaluate(const ASTNode *tree) { | ||||||
// Evaluate an AST. | ||||||
auto value_or_error = EvaluateNode(tree); | ||||||
|
||||||
// Return the computed result-or-error. | ||||||
return value_or_error; | ||||||
} | ||||||
|
||||||
llvm::Expected<lldb::ValueObjectSP> | ||||||
Interpreter::EvaluateNode(const ASTNode *node, FlowAnalysis *flow) { | ||||||
// Set up the evaluation context for the current node. | ||||||
m_flow_analysis_chain.push_back(flow); | ||||||
// Traverse an AST pointed by the `node`. | ||||||
return node->Accept(this); | ||||||
auto value_or_error = node->Accept(this); | ||||||
// Cleanup the context. | ||||||
m_flow_analysis_chain.pop_back(); | ||||||
// Return the computed value-or-error. The caller is responsible for | ||||||
// checking if an error occured during the evaluation. | ||||||
return value_or_error; | ||||||
} | ||||||
|
||||||
llvm::Expected<lldb::ValueObjectSP> | ||||||
|
@@ -232,4 +263,106 @@ Interpreter::Visit(const IdentifierNode *node) { | |||||
return identifier; | ||||||
} | ||||||
|
||||||
} // namespace lldb_private::dil | ||||||
llvm::Expected<lldb::ValueObjectSP> | ||||||
Interpreter::Visit(const UnaryOpNode *node) { | ||||||
FlowAnalysis rhs_flow( | ||||||
/* address_of_is_pending */ node->kind() == UnaryOpKind::AddrOf); | ||||||
|
||||||
Status error; | ||||||
auto rhs_or_err = EvaluateNode(node->rhs(), &rhs_flow); | ||||||
if (!rhs_or_err) { | ||||||
return rhs_or_err; | ||||||
} | ||||||
lldb::ValueObjectSP rhs = *rhs_or_err; | ||||||
|
||||||
if (rhs->GetCompilerType().IsReferenceType()) { | ||||||
rhs = rhs->Dereference(error); | ||||||
if (error.Fail()) { | ||||||
return llvm::make_error<DILDiagnosticError>(m_expr, error.AsCString(), | ||||||
node->GetLocation()); | ||||||
} | ||||||
} | ||||||
CompilerType rhs_type = rhs->GetCompilerType(); | ||||||
switch (node->kind()) { | ||||||
case UnaryOpKind::Deref: { | ||||||
if (rhs_type.IsArrayType()) | ||||||
rhs = ArrayToPointerConversion(rhs, m_exe_ctx_scope); | ||||||
|
||||||
lldb::ValueObjectSP dynamic_rhs = rhs->GetDynamicValue(m_default_dynamic); | ||||||
if (dynamic_rhs) | ||||||
rhs = dynamic_rhs; | ||||||
|
||||||
if (rhs->GetCompilerType().IsPointerType()) | ||||||
return EvaluateDereference(rhs); | ||||||
lldb::ValueObjectSP child_sp = rhs->Dereference(error); | ||||||
if (error.Success()) | ||||||
rhs = child_sp; | ||||||
|
||||||
return rhs; | ||||||
} | ||||||
case UnaryOpKind::AddrOf: { | ||||||
if (node->rhs()->is_rvalue()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I don't feel as strongly about this as about the array dereference thing, I do think that (and I hope you start to see a theme here) we shouldn't need to track the rvalue-ness of an expression/value separately from the ValueObject itself. I think that a ValueObject should know whether it's able to take the address of itself, because people may invoke
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I could gather, the first one is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can believe that's what happens now, but I don't think that's what should happen. I think my snipped above shows pretty clearly that the behavior of double
Notice that in both cases, the DIL implementation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created #137688 to address this issue. |
||||||
std::string errMsg = | ||||||
llvm::formatv("cannot take the address of an rvalue of type {0}", | ||||||
rhs_type.TypeDescription()); | ||||||
return llvm::make_error<DILDiagnosticError>(m_expr, errMsg, | ||||||
node->GetLocation()); | ||||||
} | ||||||
if (rhs->IsBitfield()) { | ||||||
return llvm::make_error<DILDiagnosticError>( | ||||||
m_expr, "address of bit-field requested", node->GetLocation()); | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if you call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error object (in the by-ref return value) will contain the string
If you want to make the error more specific, I think you should do it inside |
||||||
// If the address-of operation wasn't cancelled during the evaluation of | ||||||
// RHS (e.g. because of the address-of-a-dereference elision), apply it | ||||||
// here. | ||||||
if (rhs_flow.AddressOfIsPending()) { | ||||||
Status error; | ||||||
lldb::ValueObjectSP value = rhs->AddressOf(error); | ||||||
if (error.Fail()) { | ||||||
return llvm::make_error<DILDiagnosticError>(m_expr, error.AsCString(), | ||||||
node->GetLocation()); | ||||||
} | ||||||
return value; | ||||||
} | ||||||
return rhs; | ||||||
} | ||||||
} | ||||||
|
||||||
// Unsupported/invalid operation. | ||||||
return llvm::make_error<DILDiagnosticError>( | ||||||
m_expr, "invalid ast: unexpected binary operator", node->GetLocation(), | ||||||
1); | ||||||
} | ||||||
|
||||||
lldb::ValueObjectSP Interpreter::EvaluateDereference(lldb::ValueObjectSP rhs) { | ||||||
// If rhs is a reference, dereference it first. | ||||||
Status error; | ||||||
if (rhs->GetCompilerType().IsReferenceType()) | ||||||
rhs = rhs->Dereference(error); | ||||||
|
||||||
assert(rhs->GetCompilerType().IsPointerType() && | ||||||
"invalid ast: must be a pointer type"); | ||||||
|
||||||
if (rhs->GetDerefValobj()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why doesn't this call
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This checks if the value has already been dereferenced. The same check is also done in the beginning of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but that's exactly my problem with it. I don't see why the behavior of the DIL should depened on whether somebody has called |
||||||
return rhs->GetDerefValobj()->GetSP(); | ||||||
|
||||||
CompilerType pointer_type = rhs->GetCompilerType(); | ||||||
lldb::addr_t base_addr = rhs->GetValueAsUnsigned(0); | ||||||
|
||||||
llvm::StringRef name = "result"; | ||||||
ExecutionContext exe_ctx(m_target.get(), false); | ||||||
lldb::ValueObjectSP value = ValueObject::CreateValueObjectFromAddress( | ||||||
name, base_addr, exe_ctx, pointer_type, | ||||||
/* do_deref */ false); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the comment style used in llvm
Suggested change
|
||||||
|
||||||
// If we're in the address-of context, skip the dereference and cancel the | ||||||
// pending address-of operation as well. | ||||||
if (flow_analysis() && flow_analysis()->AddressOfIsPending()) { | ||||||
flow_analysis()->DiscardAddressOf(); | ||||||
return value; | ||||||
} | ||||||
|
||||||
return value->Dereference(error); | ||||||
} | ||||||
|
||||||
} // namespace lldb_private::dil | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing newline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add just plain primary_expression -- we're not forcing all expressions to have unary ops -- could still be plain variable names.