This is an archive of the discontinued LLVM Phabricator instance.

[lld/mac] Make -t work correctly with -flat_namespace
ClosedPublic

Authored by thakis on May 31 2021, 3:32 PM.

Details

Reviewers
int3
gkm
Group Reviewers
Restricted Project
Commits
rG222a88a24371: [lld/mac] Make -t work correctly with -flat_namespace
Summary

We used to not print dylibs referenced by other dylibs in -t mode. This
affected reexports, and with -flat_namespace also just dylibs loaded by
dylibs. Now we print them.

Fixes PR49514.

Diff Detail

Unit TestsFailed

Event Timeline

thakis created this revision.May 31 2021, 3:32 PM
Herald added a reviewer: gkm. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
int3 added a comment.Jun 1 2021, 11:38 AM

Hm, could we not just emit the message inside the DylibFile and ObjFile constructors?

Also nit: Could we have more detail in the commit message? Referencing bugzilla is a bit inconvenient (plus it could get confusing if more comments are left on the issue.)

Finally: There are a bunch of things listed in that task as "interesting things to check", not sure if we want to have more tests for those, but your call

lld/test/MachO/flat-namespace.s
19

nit: align with -NEXT: lines

thakis marked an inline comment as done.Jun 1 2021, 12:45 PM

Hm, could we not just emit the message inside the DylibFile and ObjFile constructors?

Yes, putting it in the two DylibFile ctors works just as well. It's the same +4 lines, and printing things from a ctor feels a bit weird, so I put it where I put it but if you want I can change it.

Also nit: Could we have more detail in the commit message? Referencing bugzilla is a bit inconvenient (plus it could get confusing if more comments are left on the issue.)

Sure, adding more details.

Finally: There are a bunch of things listed in that task as "interesting things to check", not sure if we want to have more tests for those, but your call

Yes, I should add more tests for some of these things, but I think they already work :)

thakis updated this revision to Diff 349068.Jun 1 2021, 12:47 PM
thakis edited the summary of this revision. (Show Details)

tweak test

int3 accepted this revision.Jun 1 2021, 1:02 PM

I'd prefer we put in in the ctors: I agree that having side-effects in a constructor is a bit weird, but OTOH it means that we'll never have to worry about new instantiation sites forgetting to add this logging.

This revision is now accepted and ready to land.Jun 1 2021, 1:02 PM
This revision was landed with ongoing or failed builds.Jun 1 2021, 4:24 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2021, 4:24 PM
thakis added a comment.Jun 1 2021, 4:35 PM

I'd prefer we put in in the ctors: I agree that having side-effects in a constructor is a bit weird, but OTOH it means that we'll never have to worry about new instantiation sites forgetting to add this logging.

I just realized I landed this without addressing this comment. Sorry, that wasn't intentional, I just forgot. I'll fix it up in a follow-up commit.