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.

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;

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)

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();
                                 }