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

[Jez Ng via Phabricator via llvm-commits]

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


================
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);
+//   };
----------------
thevinster wrote:
> 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:
>    ....
> };
sounds good!


================
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);
----------------
thevinster wrote:
> `type` looks unused here. Should this have been `type name##Offset`? 
no, `uint32_t` is indeed desired (it's the type of the offset, not the type of the field)

It's unused because `FOR_EACH_FIELD` expects a macro argument that itself takes 2 params. I'll rename this to `_` to make it clear that it's supposed to be unused


================
Comment at: lld/MachO/ObjC.cpp:231
+
+void ObjcCategoryChecker::parseCategory(const ConcatInputSection *catIsec) {
+  auto *classReloc = catIsec->getRelocAt(catLayout.klassOffset);
----------------
thevinster wrote:
> 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. 
does it? I mean I see that it parses & handles them as part of category merging. But I don't think it does any validation / raises any warnings for those -- at least, I didn't see any related warnings in ld64's objc pass


================
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);
+      }
+  }
----------------
thevinster wrote:
> 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. 
I want to wait till the category merging itself is implemented before attempting parallelization

I might also just delay landing this for a while


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142916