[LLDB][NFC] Suppress spurious static inspection warnings

[fixathon]

Summary

Suppress coverity false positives.
This diff contains comments only, including the hints for Coverity static code inspection
to suppress the warning originating at the next line after the comment.

[clayborg]

Can we use C++ comments instead of C comments?

[fixathon]

In D131998#3727664, @clayborg wrote:

Can we use C++ comments instead of C comments?

We can, and I'll make the change. Do you think it would be a good idea to keep the coverity annotations only as C-style comments (comments for the machine) to disambiguate it from the regular comments (comments for the humans) ?

[clayborg]

In D131998#3728016, @fixathon wrote:

In D131998#3727664, @clayborg wrote:

Can we use C++ comments instead of C comments?

We can, and I'll make the change. Do you think it would be a good idea to keep the coverity annotations only as C-style comments (comments for the machine) to disambiguate it from the regular comments (comments for the humans) ?

I would prefer c++ comments for coverity just so no one else sees this and says "hey, I can use C comments now in LLDB!".

[labath]

In D131998#3728016, @fixathon wrote:

In D131998#3727664, @clayborg wrote:

Can we use C++ comments instead of C comments?

We can, and I'll make the change. Do you think it would be a good idea to keep the coverity annotations only as C-style comments (comments for the machine) to disambiguate it from the regular comments (comments for the humans) ?

The llvm comment style is documented here. I don't think we should be making exceptions for coverity comments. (also, I wouldn't want to be having too many of these machine annotations around the codebase).

[fixathon]

Change to C++ style comments

[JDevlieghere]

lldb/tools/lldb-vscode/FifoFiles.cpp

65–69

89–95

lldb/include/lldb/Core/ThreadSafeValue.h

public:
 
   void SetValue(const T &value) {
     std::lock_guard<std::recursive_mutex> guard(m_mutex);
     m_value = value;
   }
 
   // Call this if you have already manually locked the mutex using the
   // GetMutex() accessor
+  // coverity[missing_lock]
   void SetValueNoLock(const T &value) { m_value = value; }
 
   std::recursive_mutex &GetMutex() { return m_mutex; }
 
 private:
   T m_value;
   mutable std::recursive_mutex m_mutex;
 
   // For ThreadSafeValue only
   ThreadSafeValue(const ThreadSafeValue &) = delete;
   const ThreadSafeValue &operator=(const ThreadSafeValue &) = delete;
 };
 
 } // namespace lldb_private
 #endif // LLDB_CORE_THREADSAFEVALUE_H

lldb/source/Host/common/ProcessRunLock.cpp

 ProcessRunLock::~ProcessRunLock() {
   int err = ::pthread_rwlock_destroy(&m_rwlock);
   (void)err;
 }
 
 bool ProcessRunLock::ReadTryLock() {
   ::pthread_rwlock_rdlock(&m_rwlock);
   if (!m_running) {
+    // coverity[missing_unlock]
     return true;
   }
   ::pthread_rwlock_unlock(&m_rwlock);
   return false;
 }
 
 bool ProcessRunLock::ReadUnlock() {
   return ::pthread_rwlock_unlock(&m_rwlock) == 0;

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

for (size_t j = 0; j < sections_count; j++) {
           continue;
         ConstString name = section->GetName();
         segment_vmaddr seg_vmaddr;
         // This is the uncommon case where strncpy is exactly
         // the right one, doesn't need to be nul terminated.
         // The segment name in a Mach-O LC_SEGMENT/LC_SEGMENT_64 is char[16] and
         // is not guaranteed to be nul-terminated if all 16 characters are
         // used.
+        // coverity[buffer_size_warning]
         strncpy(seg_vmaddr.segname, name.AsCString(),
                 sizeof(seg_vmaddr.segname));
         seg_vmaddr.vmaddr = vmaddr;
         seg_vmaddr.unused = 0;
         segment_vmaddrs.push_back(seg_vmaddr);
       }
     }
     modules_segment_vmaddrs.push_back(segment_vmaddrs);
if (make_core) {
           buffer.PutHex32(LC_NOTE);
           buffer.PutHex32(sizeof(llvm::MachO::note_command));
           char namebuf[16];
           memset(namebuf, 0, sizeof(namebuf));
           // This is the uncommon case where strncpy is exactly
           // the right one, doesn't need to be nul terminated.
           // LC_NOTE name field is char[16] and is not guaranteed to be
           // nul-terminated.
+          // coverity[buffer_size_warning]
           strncpy(namebuf, lcnote->name.c_str(), sizeof(namebuf));
           buffer.PutRawBytes(namebuf, sizeof(namebuf));
           buffer.PutHex64(lcnote->payload_file_offset);
           buffer.PutHex64(lcnote->payload.GetSize());
         }
 
         // Align to 4096-byte page boundary for the LC_SEGMENTs.
         file_offset = llvm::alignTo(file_offset, 4096);

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h

public:
 
   void AddRegSetPAuth();
 
   void AddRegSetMTE();
 
   uint32_t ConfigureVectorLength(uint32_t sve_vq);
 
   bool VectorSizeIsValid(uint32_t vq) {
+    // coverity[unsigned_compare]
     if (vq >= eVectorQuadwordAArch64 && vq <= eVectorQuadwordAArch64SVEMax)
       return true;
     return false;
   }
 
   bool IsSVEEnabled() const { return m_opt_regsets.AnySet(eRegsetMaskSVE); }
   bool IsPAuthEnabled() const { return m_opt_regsets.AnySet(eRegsetMaskPAuth); }
   bool IsMTEEnabled() const { return m_opt_regsets.AnySet(eRegsetMaskMTE); }

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.cpp

 }
 
 size_t RegisterInfoPOSIX_riscv64::GetRegisterSetCount() const {
   return k_num_register_sets - 1;
 }
 
 size_t RegisterInfoPOSIX_riscv64::GetRegisterSetFromRegisterIndex(
     uint32_t reg_index) const {
+  // coverity[unsigned_compare]
   if (reg_index >= gpr_first_riscv && reg_index <= gpr_last_riscv)
     return GPRegSet;
   if (reg_index >= fpr_first_riscv && reg_index <= fpr_last_riscv)
     return FPRegSet;
   return LLDB_INVALID_REGNUM;
 }
 
 const lldb_private::RegisterSet *
 RegisterInfoPOSIX_riscv64::GetRegisterSet(size_t set_index) const {
   if (set_index < GetRegisterSetCount())
     return &g_reg_sets_riscv64[set_index];
   return nullptr;
 }

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

 
 #else  // #if defined(__APPLE__)
   if (HostInfo::GetHostname(s)) {
     response.PutCString("hostname:");
     response.PutStringAsRawHex8(s);
     response.PutChar(';');
   }
 #endif // #if defined(__APPLE__)
-
+  // coverity[unsigned_compare]
   if (g_default_packet_timeout_sec > 0)
     response.Printf("default_packet_timeout:%u;", g_default_packet_timeout_sec);
 
   return SendPacketNoLock(response.GetString());
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_qProcessInfoPID(

lldb/tools/lldb-vscode/FifoFiles.cpp

std::future<void> *future =
       new std::future<void>(std::async(std::launch::async, [&]() {
         std::ifstream reader(m_fifo_file, std::ifstream::in);
         std::string buffer;
         std::getline(reader, buffer);
         if (!buffer.empty())
           line = buffer;
       }));
   if (future->wait_for(timeout) == std::future_status::timeout || !line)
+    // Indeed this is a leak, but it's intentional. "future" obj destructor
+    //  will block on waiting for the worker thread to join. And the worker
+    //  thread might be stuck in blocking I/O. Intentionally leaking the  obj
+    //  as a hack to avoid blocking main thread, and adding annotation to
+    //  supress static code inspection warnings
+
+    // coverity[leaked_storage]
     return createStringError(inconvertibleErrorCode(),
                              "Timed out trying to get messages from the " +
                                  m_other_endpoint_name);
   delete future;
   return json::parse(*line);
 }
 
 Error FifoFileIO::SendJSON(const json::Value &json,
                            std::chrono::milliseconds timeout) {
   bool done = false;
   std::future<void> *future =
       new std::future<void>(std::async(std::launch::async, [&]() {
         std::ofstream writer(m_fifo_file, std::ofstream::out);
         writer << JSONToString(json) << std::endl;
         done = true;
       }));
   if (future->wait_for(timeout) == std::future_status::timeout || !done) {
+    // Indeed this is a leak, but it's intentional. "future" obj destructor will
+    // block on waiting for the worker thread to join. And the worker thread
+    // might be stuck in blocking I/O. Intentionally leaking the  obj as a hack
+    // to avoid blocking main thread, and adding annotation to supress static
+    // code inspection warnings"
+
+    // coverity[leaked_storage]
     return createStringError(inconvertibleErrorCode(),
                              "Timed out trying to send messages to the " +
                                  m_other_endpoint_name);
   }
   delete future;
   return Error::success();
 }
 
 } // namespace lldb_vscode