Fix some warnings with gcc on Linux

[zturner]

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.

[JDevlieghere]

Thanks Zachary, this LGTM.

[jingham]

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 ↗

(On Diff #184163)

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.

[zturner]

lldb/source/Plugins/Language/ObjC/NSDictionary.cpp

304–314 ↗

(On Diff #184163)

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.

[davide]

the changes that touch code I wrote lgtm.

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

789–792 ↗

(On Diff #184163)

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

[zturner]

lldb/source/Plugins/Language/ObjC/NSDictionary.cpp

304–314 ↗

(On Diff #184163)

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]

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

789–792 ↗

(On Diff #184163)

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

[jingham]

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

[davide]

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

789–792 ↗

(On Diff #184163)

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

[davide]

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.

[shafik]

In D57413#1376280, @davide wrote:

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.

source/Commands/CommandObjectReproducer.cpp

protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     if (!command.empty()) {
       result.AppendErrorWithFormat("'%s' takes no arguments",
                                    m_cmd_name.c_str());
       return false;
     }
 
     auto &r = repro::Reproducer::Instance();
-    if (auto generator = r.GetGenerator()) {
+    if (r.GetGenerator()) {
       result.GetOutputStream() << "Reproducer is in capture mode.\n";
-    } else if (auto generator = r.GetLoader()) {
+    } else if (r.GetLoader()) {
       result.GetOutputStream() << "Reproducer is in replay mode.\n";
     } else {
-
       result.GetOutputStream() << "Reproducer is off.\n";
     }
 
     result.SetStatus(eReturnStatusSuccessFinishResult);
     return result.Succeeded();
   }
 };
 

source/Plugins/Language/ObjC/Cocoa.cpp

 ///   distantFuture, except within about 1e-25 second of the reference date.
 const int TAGGED_DATE_EXPONENT_BIAS = 0x3ef;
 
 typedef union {
   struct {
     uint64_t fraction:52;  // unsigned
     uint64_t exponent:11;  // signed
     uint64_t sign:1;
-  };
+  } repr;
   uint64_t i;
   double d;
 } DoubleBits;
 typedef union {
   struct {
     uint64_t fraction:52;  // unsigned
     uint64_t exponent:7;   // signed
     uint64_t sign:1;
     uint64_t unused:4;  // placeholder for pointer tag bits
-  };
+  } repr;
   uint64_t i;
 } TaggedDoubleBits;
 
 static uint64_t decodeExponent(uint64_t exp) {
   // Tagged exponent field is 7-bit signed. Sign-extend the value to 64 bits
   // before performing arithmetic.
   return llvm::SignExtend64<7>(exp) + TAGGED_DATE_EXPONENT_BIAS;
 }
 
 static uint64_t decodeTaggedTimeInterval(uint64_t encodedTimeInterval) {
   if (encodedTimeInterval == 0)
     return 0.0;
   if (encodedTimeInterval == std::numeric_limits<uint64_t>::max())
     return (uint64_t)-0.0;
 
   TaggedDoubleBits encodedBits = {};
   encodedBits.i = encodedTimeInterval;
   DoubleBits decodedBits;
 
   // Sign and fraction are represented exactly.
   // Exponent is encoded.
-  assert(encodedBits.unused == 0);
-  decodedBits.sign = encodedBits.sign;
-  decodedBits.fraction = encodedBits.fraction;
-  decodedBits.exponent = decodeExponent(encodedBits.exponent);
+  assert(encodedBits.repr.unused == 0);
+  decodedBits.repr.sign = encodedBits.repr.sign;
+  decodedBits.repr.fraction = encodedBits.repr.fraction;
+  decodedBits.repr.exponent = decodeExponent(encodedBits.repr.exponent);
 
   return decodedBits.d;
 }
 
 bool lldb_private::formatters::NSDateSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
   ProcessSP process_sp = valobj.GetProcessSP();
   if (!process_sp)

source/Plugins/Language/ObjC/NSArray.cpp

 namespace Foundation1437 {
   template <typename PtrType>
   struct DataDescriptor {
     PtrType _cow;
     // __deque
     PtrType _data;
     uint32_t _offset;
     uint32_t _size;
-    union {
-      PtrType _mutations;
-      struct {
-        uint32_t _muts;
-        uint32_t _used;
-      };
-    };
-  };
     
   using NSArrayMSyntheticFrontEnd =
      GenericNSArrayMSyntheticFrontEnd<
         DataDescriptor<uint32_t>, DataDescriptor<uint64_t>>;
   
   template <typename DD>
   uint64_t
   __NSArrayMSize_Impl(lldb_private::Process &process,

source/Plugins/Language/ObjC/NSDictionary.cpp

static const uint64_t NSDictionaryCapacities[] = {
       6221311, 10066421, 16287743, 26354171, 42641881, 68996069,
       111638519, 180634607, 292272623, 472907251
   };
   
   static const size_t NSDictionaryNumSizeBuckets = sizeof(NSDictionaryCapacities) / sizeof(uint64_t);
   
   struct DataDescriptor_32 {
     uint32_t _buffer;
-    union {
-      struct {
-        uint32_t _mutations;
-      };
-      struct {
-        uint32_t _muts;
-        uint32_t _used:25;
-        uint32_t _kvo:1;
-        uint32_t _szidx:6;
-      };
-    };
-    
+    uint32_t _used : 25;
+    uint32_t _kvo : 1;
+    uint32_t _szidx : 6;
     uint64_t GetSize() {
       return (_szidx) >= NSDictionaryNumSizeBuckets ?
           0 : NSDictionaryCapacities[_szidx];
     }
   };
   
   struct DataDescriptor_64 {
     uint64_t _buffer;
-    union {
-      struct {
-        uint64_t _mutations;
-      };
-      struct {
-        uint32_t _muts;
-        uint32_t _used:25;
-        uint32_t _kvo:1;
-        uint32_t _szidx:6;
-      };
-    };
-    
+    uint32_t _used : 25;
+    uint32_t _kvo : 1;
+    uint32_t _szidx : 6;
     uint64_t GetSize() {
       return (_szidx) >= NSDictionaryNumSizeBuckets ?
           0 : NSDictionaryCapacities[_szidx];
     }
   };
   
   using NSDictionaryMSyntheticFrontEnd =
     GenericNSDictionaryMSyntheticFrontEnd<DataDescriptor_32, DataDescriptor_64>;

source/Plugins/Language/ObjC/NSSet.cpp

using NSSetMSyntheticFrontEnd =
       GenericNSSetMSyntheticFrontEnd<DataDescriptor_32, DataDescriptor_64>;
 }
   
 namespace Foundation1437 {
   struct DataDescriptor_32 {
     uint32_t _cow;
     // __table storage
     uint32_t _objs_addr;
-    union {
-      uint32_t _mutations;
-      struct {
-        uint32_t _muts;
-        uint32_t _used : 26;
-        uint32_t _szidx : 6;
-      };
-    };
-  };
   
   struct DataDescriptor_64 {
     uint64_t _cow;
     // __Table storage
     uint64_t _objs_addr;
-    union {
-      uint64_t _mutations;
-      struct {
-        uint32_t _muts;
-        uint32_t _used : 26;
-        uint32_t _szidx : 6;
-      };
-    };
-  };
   
   using NSSetMSyntheticFrontEnd =
       GenericNSSetMSyntheticFrontEnd<DataDescriptor_32, DataDescriptor_64>;
   
   template <typename DD>
   uint64_t
   __NSSetMSize_Impl(lldb_private::Process &process, lldb::addr_t valobj_addr,
                     Status &error) {

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

#endif
 
   return error;
 }
 
 void GDBRemoteCommunication::DumpHistory(Stream &strm) { m_history.Dump(strm); }
 
 void GDBRemoteCommunication::SetHistoryStream(llvm::raw_ostream *strm) {
   m_history.SetStream(strm);
-};
+}
 
 llvm::Error
 GDBRemoteCommunication::ConnectLocally(GDBRemoteCommunication &client,
                                        GDBRemoteCommunication &server) {
   const bool child_processes_inherit = false;
   const int backlog = 5;
   TCPSocket listen_socket(true, child_processes_inherit);
   if (llvm::Error error =

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

         assert(type_sp.get());
       } break;
 
       case DW_TAG_array_type: {
         // Set a bit that lets us know that we are currently parsing this
         dwarf->GetDIEToType()[die.GetDIE()] = DIE_IS_BEING_PARSED;
 
         DWARFFormValue type_die_form;
-        int64_t first_index = 0;
         uint32_t byte_stride = 0;
         uint32_t bit_stride = 0;
         bool is_vector = false;
         const size_t num_attributes = die.GetAttributes(attributes);
 
         if (num_attributes > 0) {
           uint32_t i;
           for (i = 0; i < num_attributes; ++i) {

                        DW_TAG_value_to_name(tag), type_name_cstr);
 
           DIERef type_die_ref(type_die_form);
           Type *element_type = dwarf->ResolveTypeUID(type_die_ref);
 
           if (element_type) {
             auto array_info = ParseChildArrayInfo(die);
             if (array_info) {
-              first_index = array_info->first_index;
               byte_stride = array_info->byte_stride;
               bit_stride = array_info->bit_stride;
             }
             if (byte_stride == 0 && bit_stride == 0)
               byte_stride = element_type->GetByteSize().getValueOr(0);
             CompilerType array_element_type =
                 element_type->GetForwardCompilerType();