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

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

Thanks for finding these uninitialized variables.  That's a valuable exercise.  Couple of comments;

Most of the time you set lldb::addr_t's to 0, they probably should have been set to LLDB_INVALID_ADDRESS.  Also as I said above, any ivars of CommandObject subclasses actually get initialized before use by the OptionParsingStarting method, so the initialized values don't actually matter, but to avoid confusion if set they should be the same as the values given in that method.  I found one that was different but didn't do an exhaustive check.

You are also inconsistent in setting int variables either to {} or 0.  I think you set the ones you could tell were int's to 0 and the ones you couldn't tell off the bat to {}?  But for all the ones that are typedef's to some kind of scalar, and explicit 0 (or LLDB_INVALID_ADDRESS in a bunch of places) would be easier to read.



================
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;
----------------
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.


================
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;
----------------
Invalid addresses in lldb are indicated by the value LLDB_INVALID_ADDRESS.  Not sure what {} does for an int value, but probably not that.


================
Comment at: lldb/source/Plugins/Language/ObjC/NSSet.cpp:79
   DataDescriptor_64 *m_data_64 = nullptr;
-  lldb::addr_t m_data_ptr;
+  lldb::addr_t m_data_ptr = {};
   std::vector<SetItemDescriptor> m_children;
----------------
Again, this should be LLDB_INVALID_ADDRESS


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.h:163
   lldb::addr_t m_msg_forward_addr;
-  lldb::addr_t m_msg_forward_stret_addr;
+  lldb::addr_t m_msg_forward_stret_addr = {};
   std::unique_ptr<AppleObjCVTables> m_vtables_up;
----------------
The other ones here are also all intialized to LLDB_INVALID_ADDRESS in the constructor, so this one should be too.  I also find it confusing to have some variables initialized in the class definition and some in the constructor definition.  Since all the other ivars are initialized in the constructor in the .cpp file, it would be better to either switch all the ivars over to in-class initialization or to add the missing ones to the constructor.  Either way is fine, but the mixed method seems confusing.


================
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 = {};
----------------
These are all typedef to int types.  Putting `{}` for the initializer seems overly mysterious.  And the address should be LLDB_INVALID_ADDRESS anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130098