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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

This is very exciting!



================
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
----------------
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.


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values/main.cpp:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
I would just drop the header for testcases.


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values/main.cpp:35
+      /* Clobbers */ : "rsi"
+  );
+
----------------
Cool.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:497
+
+  finish_subexpressions_to(end_offset);
 }
----------------
We should probably just let llvm's libDebugInfo do the printing here, but this works fine for now.


================
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;
----------------
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?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2856
+                                      error_ptr, log)) {
+        LLDB_LOGF(log, "Could not evaluate %s.", DW_OP_value_to_name(op));
+        return false;
----------------
See above, we can set `error_ptr` here.


================
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) {
----------------
Don't spend too much time on that, we should just use the llvm DWARFExpression iterator instead.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3712
 
+/// Collect call site parameters in a TAG_call_site DIE.
+static CallSiteParameterArray
----------------
DW_TAG_call_site


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3752
+        LocationInCaller = parse_simple_location(i);
+        break;
+      }
----------------
default?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:795
       if (Asm->TM.Options.EnableDebugEntryValues &&
-          tuneForGDB()) {
+          (tuneForGDB() || tuneForLLDB())) {
         ParamSet Params;
----------------
This should also be tested inside of LLVM itself and probably should be a separate commit.


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

https://reviews.llvm.org/D67376