Page MenuHomePhabricator

Fix some warnings with gcc on Linux
ClosedPublic

Authored by zturner on Jan 29 2019, 1:24 PM.

Details

Summary

Most of these are surrounding the ObjectiveC formatters. They use a lot of unions containing unnamed structs, which are not ISO C++ compliant and which gcc warns about. There are several other minor warnings like unused variables, etc.

The ObjectiveC ones look NFC to me, because they basically just delete some structure / union members that are never read, but I'm throwing this up here for review anyway just to be on the safe side.

Diff Detail

Event Timeline

zturner created this revision.Jan 29 2019, 1:24 PM
JDevlieghere accepted this revision.Jan 29 2019, 1:27 PM

Thanks Zachary, this LGTM.

This revision is now accepted and ready to land.Jan 29 2019, 1:27 PM
jingham requested changes to this revision.Jan 29 2019, 1:38 PM

I don't think the data formatter changes are right. Looks like whoever wrote these data formatters was directly reading target memory onto a host sized struct, expecting everything to line up. So you can't take fields out of it.

lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
304–314

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.

This revision now requires changes to proceed.Jan 29 2019, 1:38 PM
zturner marked an inline comment as done.Jan 29 2019, 2:11 PM
zturner added inline comments.
lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
304–314

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.

the changes that touch code I wrote lgtm.

lldb/source/Plugins/Language/ObjC/Cocoa.cpp
789–792

This bit is fine, and it's the only one I can comment on, because I wrote it.

zturner marked an inline comment as done.Jan 29 2019, 2:32 PM
zturner added inline comments.
lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
304–314

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.

shafik added a subscriber: shafik.Jan 29 2019, 2:34 PM
shafik added inline comments.
lldb/source/Plugins/Language/ObjC/Cocoa.cpp
789–792

Your using the inactive member of union which is UB in C++.

jingham accepted this revision.Jan 29 2019, 2:41 PM

Ah, missed that. Whoever was probably just copying down the way the struct was defined internally in Foundation... This seems fine then - assuming Target & Host have the same endianness is not fine, but you didn't do that...

This revision is now accepted and ready to land.Jan 29 2019, 2:41 PM
davide added inline comments.Jan 29 2019, 2:45 PM
lldb/source/Plugins/Language/ObjC/Cocoa.cpp
789–792

That's clearly orthogonal to the change he made, which I suppose is a warning about unnamed unions.

To clarify, I think we ought to fix the UB regardless, but Zachary's change can go in anyway as it doesn't make the situation worse than it was before.

This revision was automatically updated to reflect the committed changes.

To clarify, I think we ought to fix the UB regardless, but Zachary's change can go in anyway as it doesn't make the situation worse than it was before.

It does not, I was more comment on the "it is fine" part.