[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;
----------------
zturner wrote:
> 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.
Just to be sure, I added this:

```
  struct DataDescriptor_64 {
    uint64_t _buffer;
    uint32_t _muts;
    uint32_t _used : 25;
    uint32_t _kvo : 1;
    uint32_t _szidx : 6;

    uint64_t GetSize() {
      return (_szidx) >= NSDictionaryNumSizeBuckets ?
          0 : NSDictionaryCapacities[_szidx];
    }
  };

  
  struct DataDescriptor_64_2 {
    uint64_t _buffer;
    union {
      struct {
        uint64_t _mutations;
      };
      struct {
        uint32_t _muts;
        uint32_t _used : 25;
        uint32_t _kvo : 1;
        uint32_t _szidx : 6;
      };
    };
  };

  static_assert(sizeof(DataDescriptor_64_2) == sizeof(DataDescriptor_64), "mismatched size!");

```

And the static assert passes.  I wouldn't want to leave that in the final code, but the point is that I it's still NFC even in the face of deleting the fields.


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

https://reviews.llvm.org/D57413