This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Warn on method name collisions from category definitions
ClosedPublic

Authored by int3 on Jan 30 2023, 10:51 AM.

Details

Summary

This implements ld64's checks for duplicate method names in categories &
classes.

In addition, this sets us up for implementing Obj-C category merging.
This diff handles the most of the parsing work; what's left is rewriting
those category / class structures.

Numbers for chromium_framework:

           base           diff           difference (95% CI)
sys_time   2.182 ± 0.027  2.200 ± 0.047  [  -0.2% ..   +1.8%]
user_time  6.451 ± 0.034  6.479 ± 0.062  [  -0.0% ..   +0.9%]
wall_time  6.841 ± 0.048  6.885 ± 0.105  [  -0.1% ..   +1.4%]
samples    33             22

Fixes https://github.com/llvm/llvm-project/issues/54912.

Diff Detail

Event Timeline

int3 created this revision.Jan 30 2023, 10:51 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 30 2023, 10:51 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
int3 requested review of this revision.Jan 30 2023, 10:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 10:51 AM
int3 planned changes to this revision.Jan 30 2023, 10:51 AM
int3 updated this revision to Diff 496007.Feb 8 2023, 7:44 PM

update test

int3 edited the summary of this revision. (Show Details)Feb 8 2023, 7:45 PM
int3 updated this revision to Diff 496008.Feb 8 2023, 7:53 PM

update

int3 updated this revision to Diff 496074.Feb 9 2023, 3:41 AM

namespace

int3 updated this revision to Diff 496080.Feb 9 2023, 4:06 AM

update

int3 updated this revision to Diff 496125.Feb 9 2023, 7:34 AM

more test updates

thevinster accepted this revision.Feb 10 2023, 12:30 AM
thevinster added a subscriber: thevinster.
thevinster added inline comments.
lld/MachO/Layout.h
22–26

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:
   ....
};
28

type looks unused here. Should this have been type name##Offset?

lld/MachO/ObjC.cpp
196

Could we have a better variable name than just s? Maybe methodName?

231

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.

284–290

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.

This revision is now accepted and ready to land.Feb 10 2023, 12:30 AM
int3 marked an inline comment as done.Feb 10 2023, 6:52 AM
int3 added inline comments.
lld/MachO/Layout.h
22–26

sounds good!

28

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

lld/MachO/ObjC.cpp
231

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

284–290

I want to wait till the category merging itself is implemented before attempting parallelization

I might also just delay landing this for a while

int3 updated this revision to Diff 496461.Feb 10 2023, 7:11 AM
int3 marked 4 inline comments as done.

address comments

thevinster added inline comments.Feb 16 2023, 1:58 AM
lld/MachO/ObjC.cpp
231

👍 I didn't read it in close detail until now. Yep it looks like it's only being parsed for the actual objc-optimization as opposed to the warnings. Carry on...

oontvoo accepted this revision.Feb 16 2023, 5:57 AM
oontvoo added a subscriber: oontvoo.

Thanks!

lld/MachO/Driver.cpp
1923

maybe conditioned this under -ObjC ? (i mean, no reason to do the checks, walking thru lots of isecs if we know there's no objc code?)

lld/MachO/ObjC.cpp
284–290

We'd love to have this check asap =)

(This should fix https://github.com/llvm/llvm-project/issues/54912)

int3 marked an inline comment as done.Feb 16 2023, 1:10 PM
int3 added inline comments.
lld/MachO/Driver.cpp
1923

Heh we actually have builds that contain ObjC but don't use the -ObjC flag

lld/MachO/ObjC.cpp
284–290

ok, will land it soon :)

int3 marked an inline comment as done.Feb 16 2023, 2:19 PM
int3 added inline comments.
lld/MachO/ObjC.cpp
284–290

actually, it might be a while -- I'm seeing some crashes when running this on our builds

int3 edited the summary of this revision. (Show Details)Mar 7 2023, 2:35 PM
This revision was landed with ongoing or failed builds.Mar 7 2023, 2:49 PM
This revision was automatically updated to reflect the committed changes.

Seems to cause segfaults in a lot of our builds .... :( Should we possibly revert this while investigating?

int3 added a comment.Mar 8 2023, 3:59 PM

Reverted. A repro would be helpful!

Reverted. A repro would be helpful!

Thanks! Working on making a small repro - will share when ready

int3 added a comment.Mar 29 2023, 12:19 PM

JFYI I have a repro of this -- will put up the fix soon (I found other issues along the way)