[Lldb-commits] [PATCH] D57413: Fix some warnings with gcc on Linux

[Zachary Turner via Phabricator via lldb-commits]

zturner marked an inline comment as done.
zturner added inline comments.


================
Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:304-314
-    union {
-      struct {
-        uint64_t _mutations;
-      };
-      struct {
-        uint32_t _muts;
-        uint32_t _used:25;
----------------
jingham wrote:
> I don't think this is right. The data formatter is reading a target side struct onto a host struct to read it in.  For instance, later on the code does:
> 
>     m_data_64 = new DataDescriptor_64();
>     process_sp->ReadMemory(data_location, m_data_64, sizeof(DataDescriptor_64),
>                            error);
> 
> So if you remove mutations you won't read in enough bytes and they won't be correctly aligned.
> 
> That's not safe if the host and target have different endian-ness, so this doesn't seem a great way to get this data into lldb.  It would be more correct to make a DataExtractor for this read, and then pull the fields out of it by size and type.  
> 
> But as it stands, you can't just delete these fields.
That is indeed how I assumed this works, but if you look carefully, it's a union of a) a 64-bit struct (struct with one uint64 member), and b) a struct with a uint32 followed by another uint32 bitfield.  So also 64-bits.  So deleting the first struct (which is never referenced) allows us to remove the union entirely, and should have no impact on the size of the final struct.


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

https://reviews.llvm.org/D57413