Allow installing watchpoints at less than 8-byte alligned addresses for AArch64 targets

[omjavaid]

Summary

This patch adds logic to make sure we can install watchpoints at 1,2 and 4 byte alligned addresses.

ptrace interface allows watchpoints at 8-byte alligned addresses. Therefor for all lower allignment levels we have to watch full 8-bytes.
We will ignore all irrelevant watchpoint trigger exceptions and will continue the target after stepping over the watchpoint instruction.

In worst case while watching a 8-byte array like byteArr[8] we may have to ignore 7 false watchpoint hits if we install watchpoint at the last byte in the array.

However overall advantage of this solution overwhelms this disadvantage. We now have all watchpoint tests passing on AArch64 Linux (HiKey board) and Arm64 Android (Nexus9).

[clayborg]

Does this patch handle being able to share an 8 byte watchpoint between two watchpoints? Lets say you have an 8 byte array named "a" and watch to watch a[0] and a[3] and a[7]. You should be able to allow the watchpoints to share the 1 watchpoint. This patch is fine as is, just something to think about for a future patch.

[labath]

The overall change looks good, but please also add a test which specifically tests for watchpoints at unaligned addresses. Last time I checked, we all watchpoint tests were passing (at least on android) even without this patch, so it looks like our tests coverage is not sufficient here.

[omjavaid]

In D21280#457196, @labath wrote:

The overall change looks good, but please also add a test which specifically tests for watchpoints at unaligned addresses. Last time I checked, we all watchpoint tests were passing (at least on android) even without this patch, so it looks like our tests coverage is not sufficient here.

Patch causes no regressions and following tests fail without this patch on Nexus 9 and arm-linux hikey board:

No test currently fully tests the functionality supported by this patch. So I ll add a new test that tests different watchpoint sizes.

UNEXPECTED SUCCESS: test_watchpoint_command_can_disable_a_watchpoint_dwarf (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py)
UNEXPECTED SUCCESS: test_watchpoint_command_can_disable_a_watchpoint_dwo (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py)
UNEXPECTED SUCCESS: test_watchpoint_command_dwarf (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py)
UNEXPECTED SUCCESS: test_watchpoint_command_dwarf (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py)
UNEXPECTED SUCCESS: test_watchpoint_command_dwo (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py)
UNEXPECTED SUCCESS: test_watchpoint_command_dwo (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py)
UNEXPECTED SUCCESS: test_watchpoint_cond_api_dwarf (python_api/watchpoint/condition/TestWatchpointConditionAPI.py)
UNEXPECTED SUCCESS: test_watchpoint_cond_api_dwo (python_api/watchpoint/condition/TestWatchpointConditionAPI.py)
UNEXPECTED SUCCESS: test_watchpoint_cond_dwarf (functionalities/watchpoint/watchpoint_commands/condition/TestWatchpointConditionCmd.py)
UNEXPECTED SUCCESS: test_watchpoint_cond_dwo (functionalities/watchpoint/watchpoint_commands/condition/TestWatchpointConditionCmd.py)
UNEXPECTED SUCCESS: test_with_python_api_dwarf (functionalities/watchpoint/watchpoint_events/TestWatchpointEvents.py)
UNEXPECTED SUCCESS: test_with_python_api_dwo (functionalities/watchpoint/watchpoint_events/TestWatchpointEvents.py)

In D21280#456670, @clayborg wrote:

Does this patch handle being able to share an 8 byte watchpoint between two watchpoints? Lets say you have an 8 byte array named "a" and watch to watch a[0] and a[3] and a[7]. You should be able to allow the watchpoints to share the 1 watchpoint. This patch is fine as is, just something to think about for a future patch.

This functionality may not work with current patch and may require more work. I will follow up with another patch after adding support for this kind of scenario.

[omjavaid]

I have added a test cases that tests all possibilities supported by current configuration.

Tests pass on Nexus 9 and aarch64-linux-gnu (hikey board).

LGTM?

[labath]

Thanks for adding the test. I have some nits about some details of the test. If you agree with them, you can commit the updated version without additional review.

packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py

2 ↗

(On Diff #60942)

Add: "Also make sure that the watchpoints work when the variables are not placed at 8-byte aligned addresses." or something to that effect. Otherwise, someone in the future will wonder why the test is so complicated.

Also, you don't seem to test 8-byte watchpoints (which is fine, but the comment is not correct then).

15 ↗

(On Diff #60942)

How about adding NO_DEBUG_INFO_TESTCASE = True here? We are not testing debug info, so it should be enough to run the test once.

49 ↗

(On Diff #60942)

this would a lot more natural if you renamed this into array_size or something and used 0-based indexes.

66 ↗

(On Diff #60942)

range(array_size)

73 ↗

(On Diff #60942)

str(i)

104 ↗

(On Diff #60942)

I guess the intention here is to test both read and write watchpoints. If so, could you state that somewhere (in a comment at least, if it's possible to somehow verify the watchpoint type in code, even better).

packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/main.c

46 ↗

(On Diff #60942)

shouldn't (typo)

[omjavaid]

packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py

104 ↗

(On Diff #60942)

We cant possibly figure out the watchpoint type read or write based on output string emitted when watchpoint is hit.

I have added a comment here which tells the user about why hit count should be updated here.

lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/Makefile

+LEVEL = ../../../make
+
+C_SOURCES := main.c
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py

+"""
+Test watchpoint size cases (1-byte, 2-byte, 4-byte).
+Make sure we can watch all bytes, words or double words individually
+when they are packed in a 8-byte region.
+
+"""
+
+from __future__ import print_function
+
+import os, time
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class WatchpointSizeTestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+
+        # Source filename.
+        self.source = 'main.c'
+
+        # Output filename.
+        self.exe_name = 'a.out'
+        self.d = {'C_SOURCES': self.source, 'EXE': self.exe_name}
+
+    @expectedFailureAndroid(archs=['arm', 'aarch64']) # Watchpoints not supported
+    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not supported on Windows")
+    @expectedFailureAll(archs=['s390x']) # Read-write watchpoints not supported on SystemZ
+    def test_byte_size_watchpoints_with_byte_selection(self):
+        """Test to selectively watch different bytes in a 8-byte array."""
+        self.run_watchpoint_size_test('byteArray', 8, '1')
+
+    @expectedFailureAndroid(archs=['arm', 'aarch64']) # Watchpoints not supported
+    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not supported on Windows")
+    @expectedFailureAll(archs=['s390x']) # Read-write watchpoints not supported on SystemZ
+    def test_two_byte_watchpoints_with_word_selection(self):
+        """Test to selectively watch different words in an 8-byte word array."""
+        self.run_watchpoint_size_test('wordArray', 4, '2')
+
+    @expectedFailureAndroid(archs=['arm', 'aarch64']) # Watchpoints not supported
+    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not supported on Windows")
+    @expectedFailureAll(archs=['s390x']) # Read-write watchpoints not supported on SystemZ
+    def test_four_byte_watchpoints_with_dword_selection(self):
+        """Test to selectively watch two double words in an 8-byte dword array."""
+        self.run_watchpoint_size_test('dwordArray', 2, '4')
+
+    def run_watchpoint_size_test(self, arrayName, array_size, watchsize):
+        self.build(dictionary=self.d)
+        self.setTearDownCleanup(dictionary=self.d)
+
+        exe = os.path.join(os.getcwd(), self.exe_name)
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+        # Detect line number after which we are going to increment arrayName.
+        loc_line = line_number('main.c', '// About to write ' + arrayName)
+
+        # Set a breakpoint on the line detected above.
+        lldbutil.run_break_set_by_file_and_line (self, "main.c",loc_line,
+            num_expected_locations=1, loc_exact=True)
+
+        # Run the program.
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        for i in range(array_size):
+            # We should be stopped again due to the breakpoint.
+            # The stop reason of the thread should be breakpoint.
+            self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                substrs = ['stopped', 'stop reason = breakpoint'])
+
+            # Set a read_write type watchpoint arrayName
+            watch_loc=arrayName+"[" + str(i) + "]"
+            self.expect("watchpoint set variable -w read_write " + watch_loc,
+                WATCHPOINT_CREATED,
+            substrs = ['Watchpoint created', 'size = ' + watchsize, 'type = rw'])
+
+            # Use the '-v' option to do verbose listing of the watchpoint.
+            # The hit count should be 0 initially.
+            self.expect("watchpoint list -v", substrs = ['hit_count = 0'])
+
+            self.runCmd("process continue")
+
+            # We should be stopped due to the watchpoint.
+            # The stop reason of the thread should be watchpoint.
+            self.expect("thread list", STOPPED_DUE_TO_WATCHPOINT,
+            substrs = ['stopped', 'stop reason = watchpoint'])
+
+            # Use the '-v' option to do verbose listing of the watchpoint.
+            # The hit count should now be 1.
+            self.expect("watchpoint list -v",
+                substrs = ['hit_count = 1'])
+
+            self.runCmd("process continue")
+
+            # We should be stopped due to the watchpoint.
+            # The stop reason of the thread should be watchpoint.
+            self.expect("thread list", STOPPED_DUE_TO_WATCHPOINT,
+            substrs = ['stopped', 'stop reason = watchpoint'])
+
+            # Use the '-v' option to do verbose listing of the watchpoint.
+            # The hit count should now be 1.
+            # Verify hit_count has been updated after value has been read.
+            self.expect("watchpoint list -v",
+                substrs = ['hit_count = 2'])
+
+            # Delete the watchpoint immediately, but set auto-confirm to true first.
+            self.runCmd("settings set auto-confirm true")
+            self.expect("watchpoint delete", substrs = ['All watchpoints removed.'])
+            # Restore the original setting of auto-confirm.
+            self.runCmd("settings clear auto-confirm")
+
+            self.runCmd("process continue")

lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/main.c

+//===-- main.c --------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#include <stdio.h>
+#include <stdint.h>
+
+uint64_t pad0 = 0;
+uint8_t byteArray[8] = {0};
+uint64_t pad1 = 0;
+uint16_t wordArray[4] = {0};
+uint64_t pad2 = 0;
+uint32_t dwordArray[2] = {0};
+
+int main(int argc, char** argv) {
+
+    int i;
+    uint8_t localByte;
+    uint16_t localWord;
+    uint32_t localDword;
+
+    for (i = 0; i < 8; i++)
+    {
+        printf("About to write byteArray[%d] ...\n", i); // About to write byteArray
+        pad0++;
+        byteArray[i]++;
+        pad1++;
+        localByte = byteArray[i]; // Here onwards we should'nt be stopped in loop
+        byteArray[i]++;
+        localByte = byteArray[i];
+    }
+
+    pad0 = 0;
+    pad1 = 0;
+
+    for (i = 0; i < 4; i++)
+    {
+        printf("About to write wordArray[%d] ...\n", i); // About to write wordArray
+        pad0++;
+        wordArray[i]++;
+        pad1++;
+        localWord = wordArray[i]; // Here onwards we should'nt be stopped in loop
+        wordArray[i]++;
+        localWord = wordArray[i];
+    }
+
+    pad0 = 0;
+    pad1 = 0;
+
+    for (i = 0; i < 2; i++)
+    {
+        printf("About to write dwordArray[%d] ...\n", i); // About to write dwordArray
+        pad0++;
+        dwordArray[i]++;
+        pad1++;
+        localDword = dwordArray[i]; // Here onwards we shouldn't be stopped in loop
+        dwordArray[i]++;
+        localDword = dwordArray[i];
+    }
+
+    return 0;
+}

lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h

public:
 
         Error
         ClearAllHardwareWatchpoints () override;
 
         Error
         GetWatchpointHitIndex(uint32_t &wp_index, lldb::addr_t trap_addr) override;
 
         lldb::addr_t
+        GetWatchpointHitAddress (uint32_t wp_index) override;
+
+        lldb::addr_t
         GetWatchpointAddress (uint32_t wp_index) override;
 
         uint32_t
         GetWatchpointSize(uint32_t wp_index);
 
         bool
         WatchpointIsEnabled(uint32_t wp_index);
 
private:
         uint64_t m_gpr_arm64[k_num_gpr_registers_arm64]; // 64-bit general purpose registers.
         RegInfo  m_reg_info;
         FPU m_fpr; // floating-point registers including extended register sets.
 
         // Debug register info for hardware breakpoints and watchpoints management.
         struct DREG
         {
             lldb::addr_t address;  // Breakpoint/watchpoint address value.
+            lldb::addr_t hit_addr; // Address at which last watchpoint trigger exception occurred.
+            lldb::addr_t real_addr;  // Address value that should cause target to stop.
             uint32_t control;  // Breakpoint/watchpoint control value.
             uint32_t refcount;  // Serves as enable/disable and refernce counter.
         };
 
         struct DREG m_hbr_regs[16];  // Arm native linux hardware breakpoints
         struct DREG m_hwp_regs[16];  // Arm native linux hardware watchpoints
 
         uint32_t m_max_hwp_supported;

lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

NativeRegisterContextLinux_arm64::SetHardwareWatchpoint (lldb::addr_t addr, size_t size, uint32_t watch_flags)
 
     // Read hardware breakpoint and watchpoint information.
     error = ReadHardwareDebugInfo ();
 
     if (error.Fail())
         return LLDB_INVALID_INDEX32;
     
     uint32_t control_value = 0, wp_index = 0;
+    lldb::addr_t real_addr = addr;
 
     // Check if we are setting watchpoint other than read/write/access
     // Also update watchpoint flag to match AArch64 write-read bit configuration.
     switch (watch_flags)
     {
         case 1:
             watch_flags = 2;
             break;
         case 2:
             watch_flags = 1;
             break;
         case 3:
             break;
         default:
             return LLDB_INVALID_INDEX32;
     }
 
     // Check if size has a valid hardware watchpoint length.
     if (size != 1 && size != 2 && size != 4 && size != 8)
         return LLDB_INVALID_INDEX32;
 
     // Check 8-byte alignment for hardware watchpoint target address.
-    // TODO: Add support for watching un-aligned addresses
+    // Below is a hack to recalculate address and size in order to
+    // make sure we can watch non 8-byte alligned addresses as well.
     if (addr & 0x07)
+    {
+        uint8_t watch_mask = (addr & 0x07) + size;
+
+        if (watch_mask > 0x08)
             return LLDB_INVALID_INDEX32;
+        else if (watch_mask <= 0x02)
+            size = 2;
+        else if (watch_mask <= 0x04)
+            size = 4;
+        else
+            size = 8;
+
+        addr = addr & (~0x07);
+    }
 
     // Setup control value
     control_value = watch_flags << 3;
     control_value |= ((1 << size) - 1) << 5;
     control_value |= (2 << 1) | 1;
 
     // Iterate over stored watchpoints
     // Find a free wp_index or update reference count if duplicate.
NativeRegisterContextLinux_arm64::SetHardwareWatchpoint (lldb::addr_t addr, size_t size, uint32_t watch_flags)
 
      if (wp_index == LLDB_INVALID_INDEX32)
         return LLDB_INVALID_INDEX32;
 
     // Add new or update existing watchpoint
     if ((m_hwp_regs[wp_index].control & 1) == 0)
     {
         // Update watchpoint in local cache
+        m_hwp_regs[wp_index].real_addr = real_addr;
         m_hwp_regs[wp_index].address = addr;
         m_hwp_regs[wp_index].control = control_value;
         m_hwp_regs[wp_index].refcount = 1;
 
         // PTRACE call to set corresponding watchpoint register.
         error = WriteHardwareDebugRegs(eDREGTypeWATCH);
 
         if (error.Fail())
NativeRegisterContextLinux_arm64::GetWatchpointHitIndex(uint32_t &wp_index, lldb::addr_t trap_addr)
     for (wp_index = 0; wp_index < m_max_hwp_supported; ++wp_index)
     {
         watch_size = GetWatchpointSize (wp_index);
         watch_addr = m_hwp_regs[wp_index].address;
 
         if (m_hwp_regs[wp_index].refcount >= 1 && WatchpointIsEnabled(wp_index)
             && trap_addr >= watch_addr && trap_addr < watch_addr + watch_size)
         {
+            m_hwp_regs[wp_index].hit_addr = trap_addr;
             return Error();
         }
     }
 
     wp_index = LLDB_INVALID_INDEX32;
     return Error();
 }
 
 lldb::addr_t
 NativeRegisterContextLinux_arm64::GetWatchpointAddress (uint32_t wp_index)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));
 
     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
     if (wp_index >= m_max_hwp_supported)
         return LLDB_INVALID_ADDRESS;
 
     if (WatchpointIsEnabled(wp_index))
-        return m_hwp_regs[wp_index].address;
+        return m_hwp_regs[wp_index].real_addr;
+    else
+        return LLDB_INVALID_ADDRESS;
+}
+
+lldb::addr_t
+NativeRegisterContextLinux_arm64::GetWatchpointHitAddress (uint32_t wp_index)
+{
+    Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));
+
+    if (log)
+        log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
+
+    if (wp_index >= m_max_hwp_supported)
+        return LLDB_INVALID_ADDRESS;
+
+    if (WatchpointIsEnabled(wp_index))
+        return m_hwp_regs[wp_index].hit_addr;
     else
         return LLDB_INVALID_ADDRESS;
 }
 
 Error
 NativeRegisterContextLinux_arm64::ReadHardwareDebugInfo()
 {
     if (!m_refresh_hwdebug_info)

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (tid != LLDB_INVALID_THREAD_ID)
                             addr_t wp_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
                             uint32_t wp_index = desc_extractor.GetU32(LLDB_INVALID_INDEX32);
                             addr_t wp_hit_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
                             watch_id_t watch_id = LLDB_INVALID_WATCH_ID;
                             if (wp_addr != LLDB_INVALID_ADDRESS)
                             {
                                 WatchpointSP wp_sp;
                                 ArchSpec::Core core = GetTarget().GetArchitecture().GetCore();
-                                if (core >= ArchSpec::kCore_mips_first && core <= ArchSpec::kCore_mips_last)
+                                if ((core >= ArchSpec::kCore_mips_first && core <= ArchSpec::kCore_mips_last) ||
+                                    (core >= ArchSpec::eCore_arm_arm64 && core <= ArchSpec::eCore_arm_aarch64))
                                     wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_hit_addr);
                                 if (!wp_sp)
                                     wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_addr);
                                 if (wp_sp)
                                 {
                                     wp_sp->SetHardwareIndex(wp_index);
                                     watch_id = wp_sp->GetID();
                                 }