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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
