[PATCH] D111447: [Dexter] Add DexDeclareAddress command and address function

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

The new tactic, code, and doc changes all SGTM (one nit inline).

I'll hold off my LGTM just for now though as I think it'd be good to have regression tests for the new command. You can take inspiration from the duplicate label test <https://github.com/llvm/llvm-project/blob/main/cross-project-tests/debuginfo-tests/dexter/feature_tests/subtools/test/err_duplicate_label.cpp> and referencing an undefined label test <https://github.com/llvm/llvm-project/blob/main/cross-project-tests/debuginfo-tests/dexter/feature_tests/subtools/test/err_bad_label_ref.cpp>, plus it would be good to have at least one test in the command tests <https://github.com/llvm/llvm-project/tree/main/cross-project-tests/debuginfo-tests/dexter/feature_tests/commands> directory using it. To that end, the test you have at the moment (cross-project-tests/debuginfo-tests/dexter-tests/address.cpp) makes more sense as a command regression test <https://github.com/llvm/llvm-project/tree/main/cross-project-tests/debuginfo-tests/dexter/feature_tests/commands> IMO (see inline comment).



================
Comment at: cross-project-tests/debuginfo-tests/dexter-tests/address.cpp:3-4
+//
+// RUN: %dexter --fail-lt 1.0 -w --builder 'clang-cl_vs2015' \
+// RUN:      --debugger 'dbgeng' --cflags '/Z7 /Zi' --ldflags '/Z7 /Zi' -- %s
+
----------------
Imo this test should be a regression test in the `dexter/feature_tests` directory, and this `RUN` line should become:

```// RUN: %dexter_regression_test -- %s```

IIRC this substitution should allow the test to run on all systems, so you can also remove `REQUIRES: system-windows`.

wdyt?


================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/command/ParseCommand.py:325
+                elif isinstance(command, DexExpectWatchBase):
+                    command.address_resolutions = address_resolutions
                 assert (path, cmd_point) not in commands[command_name], (
----------------
I wonder if there's a better way of sharing access to the `address_resolutions` dict? This doesn't feel quite right but (unhelpfully) no suggestion comes to mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111447