Timmmm wrote:
Thanks for the test case! I fixed it. The issue was that `Read()` could read
more than one packet into the internal buffer, and then `CheckForPacket()`
would only look at the first one, then when you `Read()` again there's nothing
to read. I changed it to use a loop.
I'm not really sure that the original code is correct anyway, with respect to
invalid packets. If you read an invalid packet and a valid packet in the same
`Read()` then you will have exactly the same problem - `CheckForPacket` ignores
the invalid one, but then you loop back to `Read()` and it will never return
because the following valid packet was already ready into the buffer. Difficult
to fix without a bigger refactor because `CheckForPacket` doesn't distinguish
"an invalid packet was deleted" and "incomplete packet read" so I left it and
added a comment.
There were a few options for the code that calls
`CheckForPacketIngoreNotifications`:
1. Minimal change:
```
if (CheckForPacketIgnoreNotifications(nullptr, 0, packet) !=
PacketType::Invalid)
return PacketResult::Success;
```
2. The code I've pushed with a `switch` (yeay) and `llvm_unreachable()` (boo).
```
switch (CheckForPacketIgnoreNotifications(nullptr, 0, packet)) {
case PacketType::Standard:
return PacketResult::Success;
case PacketType::Invalid:
break;
case PacketType::Notify:
// These are ignored by CheckForPacketIgnoreNotifications.
llvm_unreachable("unreachable");
}
```
3. Add another enum type:
```
enum class PacketType { Invalid = 0, Standard, Notify };
enum class PacketTypeNoNotify { Invalid = 0, Standard };
...
switch (CheckForPacketIgnoreNotifications(nullptr, 0, packet)) {
case PacketTypeNoNotify::Standard:
return PacketResult::Success;
case PacketTypeNoNotify::Invalid:
break;
}
```
Let me know which you prefer.
Finally, I had a skim of that other PR.... It doesn't look like it resolved
this issue.
> thank you for looking so closely at this — you're right that the receive path
> is subtle
I'll let you decide whether you want to talk to AI or a human!
https://github.com/llvm/llvm-project/pull/204788
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
