[PATCH] D142916: [lld-macho] Warn on method name collisions from category definitions

[Vincent Lee via Phabricator via llvm-commits]

thevinster accepted this revision.
thevinster added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/Layout.h:21-25
+//   struct FooLayout {
+//     uint32_t barOffset = 0;
+//     uint32_t bazOffset = sizeof(uintptr_t);
+//     uint32_t totalSize = sizeof(uintptr_t) + sizeof(uint32_t);
+//   };
----------------
Not against the macros, but it did take me a while to grok this and visualize what it produces. The "equivalent" is useful to some extent if I'm just trying to get an idea. But, I think it would be more useful to just show the exact text that it produces after preprocessing. 

In this example, it would be
```
struct FooLayout {
  uint32_t barOffset;
  uint32_t bazOffset;
  
  FooLayout(size_t wordSize) {
    if (wordSize == 8)
      init<uint64_t>(); 
    else {
      assert(wordSize == 4); 
      init<uint32_t>(); 
    }  
  }    
  
  private:
   ....
};


================
Comment at: lld/MachO/Layout.h:27
+
+#define _OFFSET_FOR_FIELD(type, name) uint32_t name##Offset;
+#define _INIT_OFFSET(type, name) name##Offset = offsetof(Layout<Ptr>, name);
----------------
`type` looks unused here. Should this have been `type name##Offset`? 


================
Comment at: lld/MachO/ObjC.cpp:196
+
+    CachedHashStringRef s(getReferentString(r));
+    auto &methodMap =
----------------
Could we have a better variable name than just `s`? Maybe `methodName`? 


================
Comment at: lld/MachO/ObjC.cpp:231
+
+void ObjcCategoryChecker::parseCategory(const ConcatInputSection *catIsec) {
+  auto *classReloc = catIsec->getRelocAt(catLayout.klassOffset);
----------------
ld64 also checks for class/instance properties. Just wondering, if the plan for that was to implement that in a follow up or if that was forgotten. 


================
Comment at: lld/MachO/ObjC.cpp:284-290
+  for (const InputSection *isec : inputSections) {
+    if (isec->getName() == section_names::objcCatList)
+      for (const Reloc &r : isec->relocs) {
+        auto *catIsec = cast<ConcatInputSection>(r.getReferentInputSection());
+        checker.parseCategory(catIsec);
+      }
+  }
----------------
Benchmarks don't show a huge regression, but would it better to parallelize this loop? The only downside I see is warning diagnostics being indeterministic, but that might be a blocker for others. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142916