[Lldb-commits] [PATCH] D130098: [LLDB][NFC][Reliability] Fix uninitialized variables from Coverity scan

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.


================
Comment at: lldb/include/lldb/Core/EmulateInstruction.h:182
     eInfoTypeNoArgs
-  } InfoType;
+  };
 
----------------
This doesn't fall into the un initialized variable case, I would revert this. You can always submit a new patch if you want to fix this.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4685
+    lldb::tid_t m_thread_id = LLDB_INVALID_THREAD_ID;
+    uint32_t m_thread_index = 0;
     std::string m_thread_name;
----------------
jingham wrote:
> Because of the way CommandObjects work, the constructed values of its ivars are never used.  You have to call OptionParsingStarting before doing any work with the object, so the values set in that function are the real ones.  There's no actual point in initializing these variables, but it mostly doesn't hurt except if you set them to different values from the ones in OptionParsingStarting, in which case they could cause confusion in people reading the code.  Here you've set m_thread_index to 0 but the correct starting value is actually UINT32_MAX as you can see from the OptionParsingStarting just above.
> 
> Can you go through all of the CommandObject subclass ivars that you've given values to and make sure they are the ones in the class's OptionParsingStarting?  Thanks.
yeah, I would like to get everything initialized if we can to limit our static analysis warnings.


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:410
   /// already.
-  const char *Written;
+  const char *Written = "";
 
----------------
set to nullptr instead of empty string? Check the code to see if they check this variable for NULL and use nullptr if they do.


================
Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:140
   DataDescriptor_64 *m_data_64 = nullptr;
-  lldb::addr_t m_data_ptr;
+  lldb::addr_t m_data_ptr = {};
   CompilerType m_pair_type;
----------------
jingham wrote:
> jingham wrote:
> > Invalid addresses in lldb are indicated by the value LLDB_INVALID_ADDRESS.  Not sure what {} does for an int value, but probably not that.
> Zero, which is in this case wrong.
yes, for lldb::addr_t the LLDB_INVALID_ADDRESS is the value we need to use


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1305-1306
 
         llvm::MachO::load_command lc;
+        lc.cmdsize = 0;
         if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
----------------
There are more members than just "cmdsize" that should be initialized to zero. Hopefully adding "= {};" will init everything to zero and avoid us having to say "= {0,0};"


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5425-5426
       const uint32_t cmd_offset = offset;
       llvm::MachO::load_command lc;
+      lc.cmdsize = 0;
       if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
----------------



================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5493-5494
       const uint32_t cmd_offset = offset;
       llvm::MachO::load_command lc;
+      lc.cmdsize = 0;
       if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
----------------
Ditto, and please apply to all other places where you only initialize "cmdsize" to zero. llvm::MachO::load_command only has two entries, but the other definitions below have more members.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h:274
   FileRangeArray m_thread_context_offsets;
-  lldb::offset_t m_linkedit_original_offset;
-  lldb::addr_t m_text_address;
+  lldb::offset_t m_linkedit_original_offset = {};
+  lldb::addr_t m_text_address = {};
----------------
jingham wrote:
> These are all typedef to int types.  Putting `{}` for the initializer seems overly mysterious.  And the address should be LLDB_INVALID_ADDRESS anyway.
I would set any "lldb::offset_t" values to zero instead of {} for clarity


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h:275
+  lldb::offset_t m_linkedit_original_offset = {};
+  lldb::addr_t m_text_address = {};
   bool m_thread_context_offsets_valid;
----------------
LLDB_INVALID_ADDRESS


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:417
     : ObjectFile(module_sp, file, file_offset, length, data_sp, data_offset),
-      m_dos_header(), m_coff_header(), m_sect_headers(),
+      m_dos_header(), m_coff_header(), m_sect_headers(), m_image_base(),
       m_entry_point_address(), m_deps_filespec() {
----------------



================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:419-420
       m_entry_point_address(), m_deps_filespec() {
   ::memset(&m_dos_header, 0, sizeof(m_dos_header));
   ::memset(&m_coff_header, 0, sizeof(m_coff_header));
 }
----------------
Looking at the header file, we define our own copy of these types, so we should add a bunch of "= 0;" to each member just like coff_opt_header_t does.

Then we can remove this memset stuff.


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:428
     : ObjectFile(module_sp, process_sp, header_addr, header_data_sp),
-      m_dos_header(), m_coff_header(), m_sect_headers(),
+      m_dos_header(), m_coff_header(), m_sect_headers(), m_image_base(),
       m_entry_point_address(), m_deps_filespec() {
----------------



================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:430-431
       m_entry_point_address(), m_deps_filespec() {
   ::memset(&m_dos_header, 0, sizeof(m_dos_header));
   ::memset(&m_coff_header, 0, sizeof(m_coff_header));
 }
----------------
Remove these memsets after we fix the constructors for all types we define in the ObjectFilePECOFF.h header file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130098/new/

https://reviews.llvm.org/D130098