aprantl added a comment.
This is nice, I have a few mostly stylistic comments inline.
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:947
+llvm::Optional<lldb::addr_t>
+ResolveAndLoadFileAddress(ExecutionContext *exe_ctx, lldb::ModuleSP module_sp,
----------------
Maybe add a Doxygen comment explaining what this function does?
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:948
+llvm::Optional<lldb::addr_t>
+ResolveAndLoadFileAddress(ExecutionContext *exe_ctx, lldb::ModuleSP module_sp,
+ Status *error_ptr, const char *dw_op_type,
----------------
Should this also be in the anonymous namespace?
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:964
+
+ addr_t load_Addr = so_addr.GetLoadAddress(exe_ctx->GetTargetPtr());
+
----------------
`return so_addr.GetLoadAddress(exe_ctx->GetTargetPtr();`
or `load_addr`
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:977
+ return addr_data.GetU8(&addr_data_offset);
+ break;
+ case 2:
----------------
the `break` is redundant after the `return`.
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:990
+ }
+ llvm_unreachable("Fell out of a switch with a default, shouldn't happen!");
+}
----------------
This shouldn't be necessary, or does clang warn about the function not having a
return on this otherwise?
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1141
Address so_addr;
- if (!module_sp->ResolveFileAddress(file_addr, so_addr)) {
- if (error_ptr)
- error_ptr->SetErrorString(
- "failed to resolve file address in module");
+ auto load_Addr_optional = ResolveAndLoadFileAddress(
+ exe_ctx, module_sp, error_ptr, "DW_OP_deref", file_addr, so_addr);
----------------
why is `Addr` captialized?
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1141
Address so_addr;
- if (!module_sp->ResolveFileAddress(file_addr, so_addr)) {
- if (error_ptr)
- error_ptr->SetErrorString(
- "failed to resolve file address in module");
+ auto load_Addr_optional = ResolveAndLoadFileAddress(
+ exe_ctx, module_sp, error_ptr, "DW_OP_deref", file_addr, so_addr);
----------------
aprantl wrote:
> why is `Addr` captialized?
we sometimes call these `maybe_load_addr` or `load_addr_or_err` not sure if
that's better :-)
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1149
+
if (load_Addr == LLDB_INVALID_ADDRESS) {
if (error_ptr)
----------------
should we sink this check into `ResolveAndLoadFileAddress`, too?
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1278
+
+ if (load_Addr == LLDB_INVALID_ADDRESS && so_addr.IsSectionOffset()) {
+ uint8_t addr_bytes[size];
----------------
Comment what this case handles? It's unintuitive that we go into a case where
`load_Addr == LLDB_INVALID_ADDRESS`
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1282
+
+ const bool force_live_memory = true;
+ if (exe_ctx->GetTargetRef().ReadMemory(so_addr, &addr_bytes, size,
----------------
Why do we need to force live memory?
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1293
+ } else {
+ if (error_ptr)
+ error_ptr->SetErrorStringWithFormat(
----------------
It would be nice to invert the if and have an early exit instead.
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1299
+ }
+ } else if (load_Addr == LLDB_INVALID_ADDRESS) {
+ if (error_ptr)
----------------
same here, but maybe we can just sink this into the helper.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121408/new/
https://reviews.llvm.org/D121408
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
