[Lldb-commits] [PATCH] D89842: [lldb/DWARF] Add support for DW_OP_implicit_value

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The code looks pretty straight-forward, but I believe the test should be asm-based with hardcoded dwarf (if you're feeling adventurous, you can also try yaml). My reasons for that are:

- it makes it guarantees (and makes it explicit) the input that is being tested
- the test can run on any platform (if done right -- no launching of processes)
- it enables us to test the error scenario where we fail to read the DW_OP_implicit_value data.

I think the simplest way to produce such a test would be to take `DW_TAG_variable-DW_AT_const_value.s` and exchange DW_AT_const_value for DW_AT_location/DW_FORM_exprloc



================
Comment at: lldb/source/Expression/DWARFExpression.cpp:856-867
+  const uint32_t len = opcodes.GetULEB128(&opcode_offset);
+  const void *data = opcodes.GetData(&opcode_offset, len);
+
+  if (!data) {
+    LLDB_LOG(log, "Evaluate_DW_OP_implicit_value: could not be read data");
+    return false;
+  }
----------------
I'm not sure this function is really complex enough to justify its existence. The actual code is pretty short and most of it is just argument lists and logging. I don't think the logging is very useful as the caller logs already, and the argument lists would go away if this were inlined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89842