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

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

Orlando added a comment.

In the test you've added cross-project-tests/debuginfo-tests/dexter-tests/address.cpp we have:

  // DexDeclareAddress('addr')
  // DexExpectWatchValue('a', 0, address('addr'), from_line=8, to_line=10)
  // DexExpectWatchValue('b', address('addr'), on_line=10)

Comparing this to:

  // DexExpectWatchValue('b == a', True, on_line=10)

Am I right in thinking the main benefit is that `addr` is captured (on first use in a `DexExpectWatchValue`) which means we can use the value later, even if `a` has been `optimized out` when we want to check the value of `b`?

I don't particularly like the way of assigning a value to the address variable: "In its first appearance it will match against any valid pointer". I'd prefer a dedicated command (or even rolling it into `DexDeclareAddress`) because this implicit behaviour is not very obvious when skimming a test. What do you think?

Please can you add some regression tests for the 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> using it.



================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectWatchBase.py:156
 
-        if step_info.expected_value not in self.values:
-            self.unexpected_watches.append(step_info)
-            return
+        # Check to see if this value matches with an already-resolved address
+        matching_address = None
----------------
nit: `.`


================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectWatchBase.py:165
+
+        # If we don't have a address that is already resolved to a matching value, then try and resolve a new one now
+        if step_info.expected_value not in self.values and matching_address is None:
----------------
nit: `a address` -> `an address`, and the comment needs a full stop.


================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/heuristic/Heuristic.py:131
             for command in steps.commands['DexExpectWatchValue']:
+                command.resolve_addresses(address_resolutions)
                 command.eval(steps)
----------------
Sorry if this is a silly question, why do we need to do this here in the heuristic code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111447