[Lldb-commits] [PATCH] D101361: [LLDB] Support AArch64/Linux watchpoint on tagged addresses

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:888
+  // consolidated data address mask after ignoring the top byte.
+  if (!ReadPAuthMask().Fail())
+    mask |= m_pac_mask.data_mask;
----------------
DavidSpickett wrote:
> I'm guessing this returns a Status, I think that has a .Success().
Fixed in current rev.


================
Comment at: lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h:77
+    return hit_addr;
+  }
 };
----------------
DavidSpickett wrote:
> This is going to be used by FreeBSD, have you checked if it enables top byte ignore at all?
In current rev I have moved a default implementation of this function which only ignores top byte. Should work for all other platforms.


================
Comment at: lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py:34
+    def test_watch_hit_tagged_ptr_access(self):
+        """Test LLDB hits watchpoints when tagged pointer is used for memory access"""
+
----------------
DavidSpickett wrote:
> These docstrings should be more specific. E.g. this test puts a watchpoint on an untagged address, where the one below is tagged.
Fixed in current rev.


================
Comment at: lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py:103
+
+        self.runCmd("process continue")
+
----------------
DavidSpickett wrote:
> You could remove some of the continues by adding a command to the watchpoint:
> ```
> (lldb) watchpoint command add 1 -o continue
> ```
> Then just check the stats at the end after a single continue.
> 
> I thought at first this would be bad because you wouldn't know which hit was missed, but we don't know that with the current setup either.
I would like to avoid this and keep the current version as it makes testcase more readable for newbies.


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

https://reviews.llvm.org/D101361