[Lldb-commits] [PATCH] Patch for fixing the handling of hardware breakpoints

[Deepak Panickal]

Hi Jim,

Thanks for looking through.

I've fixed the typos and uploaded a new patch.
Will commit once Greg takes a look.

Thanks,
Deepak

On 21/02/2014 17:22, jingham at apple.com wrote:
> Greg should probably take a look as well, but this seems fine to me.  There are a couple of typos in the comments, e.g.
>
>          // check if the error was something other then an unsupported breakpoint type
>
> the "then" should be "than".
>
>          // we will reach here when the stub gives an unsported response to a hardware breakpoint
>
> That should be "unsupported".
>
> And in general we try to write comments as whole sentences with initial capitals and punctuation, but on the other hand you've done a great job explaining the method so I can't get too sticky about that.
>
> The "unavailable" in this error:
>
>              error.SetErrorString("failed to set hardware breakpoint (hardware breakpoint resources might be exhausted or unavailable)");
>
> is a little confusing to me.  You already know the target says it supports hardware breakpoints so I'm not sure what unavailable means.
>
> BTW, I think the SupportsGDBStoppointPacket interface would be easier to understand if it was returning LazyBool's rather than returning a bool where "true" might mean "dunno" or it might mean "true".  This code was probably written before we added the LazyBool's so it goes this route instead.  I don't think it is necessary to rewrite your patch to use them, it's not hard to follow and you've explained it well.  But for future reference, that's a better way to handle this tri-state of "yes", "no" and "dunno".
>
> Thanks for working on this.
>
> Jim
>
>
> On Feb 21, 2014, at 5:26 AM, Deepak Panickal <deepak2427 at gmail.com> wrote:
>
>>   This addresses the concerns raised by Greg and Jim. Fixed the patch so that if software breakpoints are supported, we do not fall through to other breakpoint types. Only if it's not supported, we try the next option.
>>
>> http://llvm-reviews.chandlerc.com/D2826
>>
>> CHANGE SINCE LAST DIFF
>>   http://llvm-reviews.chandlerc.com/D2826?vs=7207&id=7278#toc
>>
>> Files:
>>   source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
>>   source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
>> <D2826.2.patch>_______________________________________________
>> lldb-commits mailing list
>> lldb-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits