Improve watchpoint error reporting specially for arm/aarch64 targets

[omjavaid]

Summary

This patch takes a few corrective measures to make sure we display meaningful reason against a watchpoint creation failure.

This is particularly important for the targets where we get to know dynamically about the availability of hardware watchpoint resources.

We are trying to check whether we have sufficient hardware watchpoint slots available before we go ahead and create a new watchpoint.

[labath]

Native register context changes seem legit. Adding Greg for the Target.cpp changes.

[omjavaid]

@clayborg
Any comments about this change? Thanks!

[clayborg]

Seems like CheckIfWatchpointsExhausted() inside Target.cpp works for some targets, but not all of them. Seems like this functionality should be pushed down into RegisterContext to do this right. For now this is OK, but we should file a bug to track fixing this correctly so that multiple watchpoints can share a single hardware watchpoint when possible.

source/Target/Target.cpp

714 ↗

(On Diff #60127)

This logic isn't necessarily correct. If we have the ability to watch N bytes at a time with a single hardware watchpoint, we might have 1 hardware watchpoint that is able to watch multiple things. So for code like:

char buffer[8] = ...;

then we watch "buffer[1]" and "buffer[7]", we could actually have 2 watchpoints but only use 1 hardware watchpoint. We really should be allowing each process plug-in to try and set the watchpoint and return the correct error instead of generically trying to catch _anything_ at the target level. So it seems like this code should be removed and moved into RegisterContext and allow each register context plug-in to respond correctly as only the it will know what can be done.

[omjavaid]

source/Target/Target.cpp

714 ↗

(On Diff #60127)

I am seeing this a little different. GetWatchpointSupportInfo has to provide information on number of watchpoint resources a target supports.
We should only fail a watchpoint on the basis of GetWatchpointSupportInfo if it returns zero that is for a particular hardware we have no watchpoint support.

For the case where we want to utilize same hardware resource to watch multiple watchpoints, we should try to manage that in register context where we can keep track of watchpoints previously installed without getting the target backend involved. First we can attempt to intall a watchpoint using an existing resource and if that fails we can use a new hardware resource. Return failure if watchpoint resources are used up. This way you can put multiple watchpoint on same address without having to use up hardware watchpoint resources. I will post this solution of arm and aarch64 soon.

721 ↗

(On Diff #60127)

I am going to remove this case from final commit as we may have more watchpoints utilizing one or more hardware watchpoint resources.

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

if (log)
         log->Printf ("NativeRegisterContextLinux_arm::%s()", __FUNCTION__);
 
     Error error;
 
     // Read hardware breakpoint and watchpoint information.
     error = ReadHardwareDebugInfo ();
 
     if (error.Fail())
-        return LLDB_INVALID_INDEX32;
+        return 0;
 
     return m_max_hwp_supported;
 }
 
 uint32_t
 NativeRegisterContextLinux_arm::SetHardwareWatchpoint (lldb::addr_t addr, size_t size, uint32_t watch_flags)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));

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

if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
     Error error;
 
     // Read hardware breakpoint and watchpoint information.
     error = ReadHardwareDebugInfo ();
 
     if (error.Fail())
-        return LLDB_INVALID_INDEX32;
+        return 0;
 
     return m_max_hwp_supported;
 }
 
 uint32_t
 NativeRegisterContextLinux_arm64::SetHardwareWatchpoint (lldb::addr_t addr, size_t size, uint32_t watch_flags)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));

lldb/trunk/source/Target/Target.cpp

Target::ProcessIsValid()
     return (m_process_sp && m_process_sp->IsAlive());
 }
 
 static bool
 CheckIfWatchpointsExhausted(Target *target, Error &error)
 {
     uint32_t num_supported_hardware_watchpoints;
     Error rc = target->GetProcessSP()->GetWatchpointSupportInfo(num_supported_hardware_watchpoints);
-    if (rc.Success())
+    if (num_supported_hardware_watchpoints == 0)
     {
-        uint32_t num_current_watchpoints = target->GetWatchpointList().GetSize();
-        if (num_current_watchpoints >= num_supported_hardware_watchpoints)
-            error.SetErrorStringWithFormat("number of supported hardware watchpoints (%u) has been reached",
-                                           num_supported_hardware_watchpoints);
-    }
+        error.SetErrorStringWithFormat ("Target supports (%u) hardware watchpoint slots.\n",
         return false;
     }
+    return true;
+}
 
 // See also Watchpoint::SetWatchpointType(uint32_t type) and
 // the OptionGroupWatchpoint::WatchType enum type.
 WatchpointSP
 Target::CreateWatchpoint(lldb::addr_t addr, size_t size, const CompilerType *type, uint32_t kind, Error &error)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));
     if (log)
if (addr == LLDB_INVALID_ADDRESS || size == 0)
         return wp_sp;
     }
     
     if (!LLDB_WATCH_TYPE_IS_VALID(kind))
     {
         error.SetErrorStringWithFormat ("invalid watchpoint type: %d", kind);
     }
 
+    if (!CheckIfWatchpointsExhausted (this, error))
+        return wp_sp;
+
     // Currently we only support one watchpoint per address, with total number
     // of watchpoints limited by the hardware which the inferior is running on.
 
     // Grab the list mutex while doing operations.
     const bool notify = false;   // Don't notify about all the state changes we do on creating the watchpoint.
     std::unique_lock<std::recursive_mutex> lock;
     this->GetWatchpointList().GetListMutex(lock);
     WatchpointSP matched_sp = m_watchpoint_list.FindByAddress(addr);
if (log)
                     wp_sp->GetID());
 
     if (error.Fail()) 
     {
         // Enabling the watchpoint on the device side failed.
         // Remove the said watchpoint from the list maintained by the target instance.
         m_watchpoint_list.Remove (wp_sp->GetID(), true);
         // See if we could provide more helpful error message.
-        if (!CheckIfWatchpointsExhausted(this, error))
-        {
-            if (!OptionGroupWatchpoint::IsWatchSizeSupported(size))
-                error.SetErrorStringWithFormat("watch size of %" PRIu64 " is not supported", (uint64_t)size);
-        }
+
         wp_sp.reset();
     }
     else
         m_last_created_watchpoint = wp_sp;
     return wp_sp;
 }
 
 void