[Lldb-commits] [PATCH] D67376: [DWARF] Evaluate DW_OP_entry_value

[Vedant Kumar via Phabricator via lldb-commits]

vsk planned changes to this revision.
vsk added a comment.

TODO:

- Split out llvm change.
- Add a test to validate that lldb skips inline frames when evaluating DW_OP_entry_value.



================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values/Makefile:4
+include $(LEVEL)/Makefile.rules
+CXXFLAGS += -g -O1 -glldb -Xclang -femit-debug-entry-values
----------------
aprantl wrote:
> The -g should be redundant, and if we leave out -glldb (which should be the default for clang anyway) then this test in theory could also work with other compilers such as GCC (which I think implements the same option under the same -f option). I have no idea whether anyone runs such a bot, but it's still nice.
llvm entry value support requires one of -glldb or -ggdb at the moment, but -g can be dropped.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1117
+  if ((!exe_ctx || !exe_ctx->HasTargetScope()) || !reg_ctx) {
+    LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no exe/reg context");
+    return false;
----------------
aprantl wrote:
> The Evaluate function allows setting an Error with a message. Should we do the same thing here so we can return the error to the caller?
I worry that these log messages are too specific to be meaningful to the caller. The immediate caller should be able to issue an error, however.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.cpp:62
 DRC_class DW_OP_value_to_class(uint32_t val) {
+  // FIXME: This switch should be simplified by using DW_OP_* names.
   switch (val) {
----------------
aprantl wrote:
> Don't spend too much time on that, we should just use the llvm DWARFExpression iterator instead.
Updated the fixme.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3752
+        LocationInCaller = parse_simple_location(i);
+        break;
+      }
----------------
aprantl wrote:
> default?
I don't believe any action is needed in the default case: we do not want to log or report an error.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67376/new/

https://reviews.llvm.org/D67376