[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

Tue Sep 27 08:36:22 PDT 2022

labath added a comment.

In D133858#3816105 <https://reviews.llvm.org/D133858#3816105>, @fdeazeve wrote:

> In D133858#3805630 <https://reviews.llvm.org/D133858#3805630>, @labath wrote:
>
>> I am afraid that this patch misses one method of initiating a debug session -- connecting to a running debug server (`process connect`, `SBTarget::ConnectRemote`). The breakpoint hit counts don't get reset in case of a reconnect. This isn't a particularly common use case (and the only reason I've noticed it is that for `PlatformQemuUser`, all "launches" are actually "connects" under the hood <https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp#L227>), but I've verified that this problem can be reproduced by issuing connect commands manually (on the regular host platform). I'm pretty sure that was not intentional.
>>
>> Fixing this by adding another callout to `ResetBreakpointHitCounts` would be easy enough, but I'm also thinking if there isn't a better place from which to call this function, one that would capture all three scenarios in a single statement. I think that one such place could be `Target::CreateProcess`. This function is called by all three code paths, and it's a very good indicator that we will be starting a new debug session.
>>
>> What do you think?
>
> My understanding is that there is an obligation of calling the WillXX methods before calling XX, so by placing the reset code in the WillXX functions we can rely on that guarantee. Right now, the same is implicit for "one must call Target::CreateProcess before launching or attaching". We could instead define a WillConnect and have the usual contract for that.

I wouldn't really call it a "usual" contract, but yes, I'm sure this could be fixed by adding a WillConnect method. It might be also sufficient to just call WillAttach from at some appropriate place, since a "connect" operation looks very similar to an "attach", and a lot of the initialization operations are the same. I think we're already something like this somewhere (maybe for DidAttach?). However, that still leaves us with three (or two) places that need to be kept in sync.

> The code is fairly new to me, so I'm not confident enough to make a judgement call here. Thoughts?

I don't see any advantage in doing this particular action "just before" a launch, as opposed to doing in on process creation, so I would definitely do it that way.

I also find it weird to be going through the Process class just to call another Target method, when all of the launch/attach/connect operations already go through the Target class, and so it should be perfectly capable of calling that method on its own.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133858