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

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

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

I've looked at the two changes since my SGTM comment, which I'll now happily upgrade to a LGTM.

The regression tests look great! Most of them have `REQUIRES: system-linux`. I assume this is because they rely on the conditional controller, and dbgeng wrapper doesn't support it. Is that right? If so, I wonder if there's anything stopping them working on `system-darwin`. Perhaps these should instead be `XFAIL: system-windows` instead?

I enjoy the small refactor, and the improved verbose output looks good. Thanks!



================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectWatchBase.py:31
+    def resolved_value(self, resolutions):
+        if not self.name in resolutions or resolutions[self.name] is None:
+            return None
----------------
StephenTozer wrote:
> Aforementioned minor fix is here: the addition of `resolutions[self.name] is None` is needed to catch an address which is validly declared but does not have a resolved value (which should be because the line it was declared for was never stepped on). This results in a 'missing value' result for the variable that references it.
SGTM


================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectWatchBase.py:34
+            return None
+        # Technically we should fill(8) if we're debugging on a 32bit architecture?
+        return format_address(resolutions[self.name] + self.offset)
----------------
nit: this comment needs updating after the latest change


================
Comment at: cross-project-tests/debuginfo-tests/dexter/feature_tests/subtools/test/address_printing.cpp:6-9
+//
+//      Note: Currently "misordered result" is the only penalty that does not
+//      display the address properly; if it is implemented, this test should be
+//      updated.
----------------
I don't think it should block the feature either way but I'm curious about why this is? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111447