[Lldb-commits] [lldb] [lldb] Ignore notification packets (PR #204788)

[Tim Hutt via lldb-commits]

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