[LLDB][NFC] Fix potential div by 0 "count" can be zero potentially causing div by 0

[fixathon]

Summary

Add a check for div by zero

[DavidSpickett]

Given that testing packet speed by sending no packets seems pointless, I wonder if this should be asserts on the packet counts instead.

If this speed test is something we do as part of connection setup, you'd probably want to know that something went wrong rather than lldb claiming that any speed/packet size/whatever is fine. Then failing to send any real packets later.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

2500–2501

This could have the same issue if num_packets parameter is 0.

[fixathon]

The function has a void return type, with no way to communicate its success/failure status. Perhaps the right thing to do is to mod the function to return error Status, in addition to adding the assert (or some runtime check that works for all build flavors)?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

2500–2501

This instance of average_per_packet variable is not even being used anywhere in the code, in its local scope. Same for packets_per_second just above it.
These 2 could possibly just be deleted?

[DavidSpickett]

I stopped being lazy and went and found where this is used. It's actually a command you can run from the interpreter that prints out the results of the testing.

(lldb) process plugin packet speed-test -c 0
Testing sending 0 packets of various sizes:
<...>
qSpeedTest(send=   1024, recv=   1024) in 0.000000120 s for      0.00 packets/s (0.000000 ms per packet) with standard deviation of 0.000000 ms
Testing receiving 4.0MB of data using varying receive packet sizes:
<...>
qSpeedTest(send=      0, recv=   1024)   4096 packets needed to receive 4.0MB in 0.222097710 s for 18.01 MB/sec for  18442.33 packets/sec (0.054223 ms per packet)

So I think it is legitimate to specify 0 send packets here, if you just want to test receiving. So keep the check you've got and add the same sort of check for the instance above it.

(whether this command should even print anything if you're sending 0 packets, that's a matter for another time)

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

2500–2501

It's used to print to the stream below.

[fixathon]

Address the review comments

[DavidSpickett]

Thanks for these fixes! I'll fix up the 0 packets sent behaviour when I find some spare time.

[fixathon]

In D130939#3698800, @DavidSpickett wrote:

Thanks for these fixes! I'll fix up the 0 packets sent behaviour when I find some spare time.

Sounds great. Thanks for the review and suggestions!

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

if (bytes_left >= 26) {
                     "abcdefghijklmnopqrstuvwxyz");
       bytes_left = 0;
     }
   }
 }
 
 duration<float>
 calculate_standard_deviation(const std::vector<duration<float>> &v) {
+  if (v.size() == 0)
+    return duration<float>::zero();
   using Dur = duration<float>;
   Dur sum = std::accumulate(std::begin(v), std::end(v), Dur());
   Dur mean = sum / v.size();
   float accum = 0;
   for (auto d : v) {
     float delta = (d - mean).count();
     accum += delta * delta;
   };
 
   return Dur(sqrtf(accum / (v.size() - 1)));
 }
 
 void GDBRemoteCommunicationClient::TestPacketSpeed(const uint32_t num_packets,
                                                    uint32_t max_send,
                                                    uint32_t max_recv,
                                                    uint64_t recv_amount,
                                                    bool json, Stream &strm) {
-  uint32_t i;
+
   if (SendSpeedTestPacket(0, 0)) {
     StreamString packet;
     if (json)
       strm.Printf("{ \"packet_speeds\" : {\n    \"num_packets\" : %u,\n    "
                   "\"results\" : [",
                   num_packets);
     else
       strm.Printf("Testing sending %u packets of various sizes:\n",
                   num_packets);
     strm.Flush();
 
     uint32_t result_idx = 0;
     uint32_t send_size;
     std::vector<duration<float>> packet_times;
 
     for (send_size = 0; send_size <= max_send;
          send_size ? send_size *= 2 : send_size = 4) {
       for (uint32_t recv_size = 0; recv_size <= max_recv;
            recv_size ? recv_size *= 2 : recv_size = 4) {
         MakeSpeedTestPacket(packet, send_size, recv_size);
 
         packet_times.clear();
         // Test how long it takes to send 'num_packets' packets
         const auto start_time = steady_clock::now();
-        for (i = 0; i < num_packets; ++i) {
+        for (uint32_t i = 0; i < num_packets; ++i) {
           const auto packet_start_time = steady_clock::now();
           StringExtractorGDBRemote response;
           SendPacketAndWaitForResponse(packet.GetString(), response);
           const auto packet_end_time = steady_clock::now();
           packet_times.push_back(packet_end_time - packet_start_time);
         }
         const auto end_time = steady_clock::now();
         const auto total_time = end_time - start_time;
 
         float packets_per_second =
             ((float)num_packets) / duration<float>(total_time).count();
-        auto average_per_packet = total_time / num_packets;
+        auto average_per_packet = num_packets > 0 ? total_time / num_packets
+                                                  : duration<float>::zero();
         const duration<float> standard_deviation =
             calculate_standard_deviation(packet_times);
         if (json) {
           strm.Format("{0}\n     {{\"send_size\" : {1,6}, \"recv_size\" : "
                       "{2,6}, \"total_time_nsec\" : {3,12:ns-}, "
                       "\"standard_deviation_nsec\" : {4,9:ns-f0}}",
                       result_idx > 0 ? "," : "", send_size, recv_size,
                       total_time, standard_deviation);
for (uint32_t recv_size = 32; recv_size <= max_recv; recv_size *= 2) {
         }
         const auto end_time = steady_clock::now();
         const auto total_time = end_time - start_time;
         float mb_second = ((float)recv_amount) /
                           duration<float>(total_time).count() /
                           (1024.0 * 1024.0);
         float packets_per_second =
             ((float)packet_count) / duration<float>(total_time).count();
-        const auto average_per_packet = total_time / packet_count;
+        const auto average_per_packet = packet_count > 0
+                                            ? total_time / packet_count
+                                            : duration<float>::zero();
 
         if (json) {
           strm.Format("{0}\n     {{\"send_size\" : {1,6}, \"recv_size\" : "
                       "{2,6}, \"total_time_nsec\" : {3,12:ns-}}",
                       result_idx > 0 ? "," : "", send_size, recv_size,
                       total_time);
           ++result_idx;
         } else {