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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
