Page MenuHomePhabricator

[lld-macho] Mark dylib symbols coming from -weak_framework as weak-ref
ClosedPublic

Authored by oontvoo on Nov 22 2021, 2:16 PM.

Details

Summary

PR:52564

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
oontvoo requested review of this revision.Nov 22 2021, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2021, 2:16 PM
oontvoo planned changes to this revision.Nov 22 2021, 3:02 PM
oontvoo updated this revision to Diff 389079.Nov 22 2021, 7:01 PM

updated diff for real

test? :)

Sorry - forgot the --draft when running arc diff
Anyway, should be ready to review now :) (With tests! )

oontvoo edited the summary of this revision. (Show Details)Nov 22 2021, 7:02 PM
oontvoo updated this revision to Diff 389188.Nov 23 2021, 6:30 AM
oontvoo edited the summary of this revision. (Show Details)

minor fix

oontvoo added inline comments.Nov 23 2021, 6:32 AM
lld/test/MachO/weak-import.s
14–20

This is just renaming the output %t/test to different names so we don't overwrite the binaries. (makes it easier for debugging/examining the outputs afterwards)

thevinster added a subscriber: thevinster.EditedNov 23 2021, 1:35 PM

Probably should look at the failing tests. Seems to be related to your changes. [EDIT] - looks like it's fixed but my comment below still stands.

lld/MachO/Driver.cpp
391 ↗(On Diff #389079)

Hmm.... I'm not sure why this change is needed? I think the original code of forcefully setting forceWeakImport on the dylibFile should be fine. I think the only change missing here is the change in isWeakDef and isWeakRef to make sure it accounts for the forceWeakImport flag which you already did in Symbols.h. Everything else can stay as is.

Probably should look at the failing tests. Seems to be related to your changes. [EDIT] - looks like it's fixed but my comment below still stands.

Yes - it's green now :)

oontvoo added inline comments.Nov 23 2021, 2:12 PM
lld/MachO/Driver.cpp
391 ↗(On Diff #389079)

There were two problems:

  • (in DriverUtil::loadDylib) if the dylib has previously been loaded without the weak flag, and it is being loaded again now with the flag, then this flag will not mean anything because the loadDylib() would return pointer to the cached Dylib object.
  • consequently, symbols from such dylib cannot be effectively marked "weak" (because they were previously seen as regular/strong dylib)

So the change here is to ensure we propagate the flag to loadDylib

oontvoo marked an inline comment as done.Nov 23 2021, 2:42 PM
oontvoo added inline comments.
lld/MachO/Driver.cpp
391 ↗(On Diff #389079)

Ooh, I see what you're saying. Yes - you're right :P Sorry wasn't reading carefully

oontvoo planned changes to this revision.Nov 23 2021, 2:42 PM
oontvoo updated this revision to Diff 389326.Nov 23 2021, 2:56 PM

revert unnecessary change

thevinster added inline comments.Nov 23 2021, 6:17 PM
lld/MachO/SymbolTable.cpp
170 ↗(On Diff #389329)

Hmm... I still don't understand why this change is needed. This code never gets run after the first framework gets loaded because it would short circuit and return the cached file (https://github.com/llvm/llvm-project/blob/main/lld/MachO/DriverUtils.cpp#L212). I think the -weak_framework is just meant to set the the forceWeakImport flag which is being accounted for in Symbols.h.

I pulled the diff in, removed all the changes in SymbolTable and only kept Symbols.h changes, and the test still passed.

oontvoo added inline comments.Nov 23 2021, 6:47 PM
lld/MachO/SymbolTable.cpp
170 ↗(On Diff #389329)

hmmm .... Didn't work for me . (or rather this was needed to fix the missing symbol crash at runtime )
maybe I over simplified the test 🤔

oontvoo marked an inline comment as done.Nov 23 2021, 7:05 PM
oontvoo added inline comments.
lld/MachO/SymbolTable.cpp
170 ↗(On Diff #389329)

P.S : ignore that - PEBCAK

oontvoo updated this revision to Diff 389371.Nov 23 2021, 7:06 PM

reduce diff further

thevinster accepted this revision.Nov 23 2021, 7:44 PM

Sweet! LG

This revision is now accepted and ready to land.Nov 23 2021, 7:44 PM
int3 requested changes to this revision.Nov 24 2021, 12:10 AM

Catching up on this now... is this part of the commit message still accurate?

if a framework was previoulsy loaded as regular, then it was loaded again as weak, then the weak flag would not be propagated

also

Symbols imported from weak framework should be marked as weak-def.

The test is only checking that they are marked as weak *refs*, though :) are the changes to isWeakDef necessary?

Otherwise, we may get runtime crash due to symbols not found.

Would be good to add this as an explanatory comment to isWeakRef.

lld/MachO/Symbols.h
244

we can cast<> here, there's no doubt about what the underlying type is (and if it's wrong the current code would do a null deref anyway)

244

I think we may need to make this getFile()->umbrella here to ensure symbols in re-exports are handled correctly too. (should add a test for that too)

lld/test/MachO/weak-import.s
15

nit: the other filenames use hyphens instead of underscores, let's stick with that

also I assume here "core" means "CoreFoundation"? I think "cf" would be clearer... but also I don't think it is a super accurate name for this test 🤔since here we are testing 3 different weak-import flags, only one of which applies to CF. IMO we can just call this test %t/basic, since, well, that's what it is as far as weak-import flags are concerned

18–19

%t/test-strong-weak?

27–28

imo there's no need to create two additional binaries here; we can just reuse the binaries created in lines 16 and 17 above

This revision now requires changes to proceed.Nov 24 2021, 12:10 AM
oontvoo updated this revision to Diff 389538.Nov 24 2021, 10:23 AM
oontvoo marked 4 inline comments as done.

updated diff

Catching up on this now... is this part of the commit message still accurate?

Yes ... (ish)

if a framework was previoulsy loaded as regular, then it was loaded again as weak, then the weak flag would not be propagated

also

Symbols imported from weak framework should be marked as weak-def.

The test is only checking that they are marked as weak *refs*, though :) are the changes to isWeakDef necessary?

Ok agreed that the test is only for refs - but logically speaking it should apply to def too

Otherwise, we may get runtime crash due to symbols not found.

Would be good to add this as an explanatory comment to isWeakRef.

done

lld/test/MachO/weak-import.s
27–28

25 can be used with 16 - yes.
but for completeness, 26 is still needed :) (there's no weak-strong )

int3 added a comment.Nov 25 2021, 11:30 AM

Yes ... (ish)

Can you explain why you think "the implementation did not do that"? Lines 12 and 13 in the old test (before your changes) cover that case...

but logically speaking it should apply to def too

I'm not sure it follows logically. Did you see ld64 doing this too? Weak reference => a missing symbol at runtime is not an error. Weak definition => duplicate symbols at runtime is not an error. The former is definitely relevant for weak libraries, but I don't see how the latter comes in.

Would be good to add this as an explanatory comment to isWeakRef.

done

I don't see it, did you forget to update the diff? I also don't see a test for a re-exported symbol :)

lld/test/MachO/weak-import.s
27–28

16 tests strong-weak-strong, so it technically encompasses both strong-weak and weak-strong :p or at the very least it shows that weakness takes precedence over strong regardless of flag order.

I'm fine if you would like to break it up into two binaries, strong-weak and weak-strong, but we can def reuse it

oontvoo updated this revision to Diff 390347.Nov 29 2021, 7:08 AM
oontvoo marked 2 inline comments as done.

added comment on weakref

oontvoo added a comment.EditedNov 29 2021, 7:08 AM

Let me try responding again with more thinking
( Wed was sort of holiday-eve so I was in a rush... sorry about that)

Yes ... (ish)

Can you explain why you think "the implementation did not do that"? Lines 12 and 13 in the old test (before your changes) cover that case...

What I said was mainly on the handling of the symbols from weak libraries/frameworks.
IOWs, line 12-13 (in the old tests) only tested that the weak things had proper load-commands (LC_LOAD_WEAK_DYLIB vs LC_LOAD_DYLIB) but nothing on the semantic of the symbols in them. (hence this bug :) )

but logically speaking it should apply to def too

I'm not sure it follows logically.

Consider this test:

-arch x86_64 -platform_version macos 10.15 11.0 -syslibroot <path>lld/test/MachO/Inputs/MacOSX.sdk -weak_framework NewCoreFoundation  -framework CoreFoundation  test.o

(Imagine NewCoreFoundation is just a duplicate of CoreFoundation with the same symbols defined)

Which __CFBigNumGetInt128 do you think should be picked?

IMO, since the NewCoreFoundation is weak, the symbols should come from the non-weak one, ie CoreFoundation.

Did you see ld64 doing this too? Weak reference => a missing symbol at runtime is not an error. Weak definition => duplicate symbols at runtime is not an error. The former is definitely relevant for weak libraries, but I don't see how the latter comes in.

No - it doesn't do this - it just picks whichever comes first on the link-command.

Having said that, if you all think we should mimic LD64 here - i'm happy to revert the change in isWeakDef() - Just wanted to bring this up

Would be good to add this as an explanatory comment to isWeakRef.

done

I don't see it, did you forget to update the diff? I also don't see a test for a re-exported symbol :)

done (real)

int3 added a comment.Nov 29 2021, 12:12 PM

What I said was mainly on the handling of the symbols from weak libraries/frameworks.

Right, so there are two parts to your commit message...

Symbols imported from weak framework should be marked as weak-def. Otherwise, we may get runtime crash due to symbols not found.

This part is what your diff fixes

Additionally, frameworks/libraries can be specified multiple times (eg. -framework Foo -weak_frameworks Foo). When that happens,
the "forceWeakImport" should be "contagious"

This is already existing behavior

IMO, since the NewCoreFoundation is weak, the symbols should come from the non-weak one, ie CoreFoundation.

"weak" is an (unfortunately) overloaded term here. From reading ld64's manpage, it seems that "weak library" is just short for "weakly referenced library". There's nothing there that says that they contain weak definitions as well.

And in the future please highlight any intentional deviations from ld64's behavior in the commit message :)

done (real)

Explanatory comment is done, but we still need a test involving a re-exported symbol

lld/MachO/Symbols.h
240
243–244

not really sure this comment is important -- we don't actually call isWeakRef until after all the file parsing is done anyway. Also the computation involved is pretty trivial, I don't think we'd ever consider it as a candidate for caching

oontvoo edited the summary of this revision. (Show Details)EditedNov 29 2021, 12:44 PM

"weak" is an (unfortunately) overloaded term here. From reading ld64's manpage, it seems that "weak library" is just short for "weakly referenced library". There's nothing there that says that they contain weak definitions as well.

Ok, understood. Since it's now undocumented territory, do you 👎 or 👍 the weak-def change?

And in the future please highlight any intentional deviations from ld64's behavior in the commit message :)

updated

done (real)

Explanatory comment is done, but we still need a test involving a re-exported symbol

int3 added a comment.Nov 29 2021, 12:53 PM

do you 👎 or 👍 the weak-def change?

👎, there's nothing to justify it...

oontvoo retitled this revision from [lld-macho] Mark dylib symbols coming from -weak_framework as weak-def. to [lld-macho] Mark dylib symbols coming from -weak_framework as weak-ref.Nov 29 2021, 3:04 PM
oontvoo edited the summary of this revision. (Show Details)
oontvoo updated this revision to Diff 390482.Nov 29 2021, 3:04 PM
oontvoo marked 2 inline comments as done.

added reexport tests and reverted weak-def change

oontvoo updated this revision to Diff 390484.Nov 29 2021, 3:05 PM

reverted accidental change

do you 👎 or 👍 the weak-def change?

👎, there's nothing to justify it...

reverted . PTAL!

int3 accepted this revision.Nov 29 2021, 8:04 PM

Thanks!

lld/MachO/Symbols.h
240–242

Similar to our discussions about the commit message, I think there's some confusion here about what's getting fixed :) this has nothing to do with libraries being loaded multiple times...

lld/test/MachO/weak-import.s
31
33

nit: nice to be a bit more specific...

This revision is now accepted and ready to land.Nov 29 2021, 8:04 PM
oontvoo edited the summary of this revision. (Show Details)Nov 30 2021, 6:52 AM
oontvoo updated this revision to Diff 390704.Nov 30 2021, 6:53 AM
oontvoo marked 2 inline comments as done.
oontvoo edited the summary of this revision. (Show Details)

updated diff

This revision was landed with ongoing or failed builds.Nov 30 2021, 6:55 AM
This revision was automatically updated to reflect the committed changes.
oontvoo marked an inline comment as done.