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

[oontvoo]

Summary

PR:52564

[int3]

test? :)

[oontvoo]

updated diff for real

[oontvoo]

In D114397#3147426, @int3 wrote:

test? :)

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

[oontvoo]

minor fix

[oontvoo]

lld/test/MachO/weak-import.s

12–18

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]

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.

[oontvoo]

In D114397#3149741, @thevinster wrote:

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]

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]

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]

revert unnecessary change

[oontvoo]

{}

[thevinster]

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]

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]

lld/MachO/SymbolTable.cpp

170 ↗

(On Diff #389329)

P.S : ignore that - PEBCAK

[oontvoo]

reduce diff further

[thevinster]

Sweet! LG

[int3]

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

243

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)

243

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

13

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

16–17

%t/test-strong-weak?

25–26

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

[oontvoo]

updated diff

[oontvoo]

In D114397#3150677, @int3 wrote:

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

25–26

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

[int3]

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

25–26

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]

added comment on weakref

[oontvoo]

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

In D114397#3154518, @int3 wrote:

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]

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

239

242–243

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]

In D114397#3159178, @int3 wrote:

"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]

do you 👎 or 👍 the weak-def change?

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

[oontvoo]

added reexport tests and reverted weak-def change

[oontvoo]

reverted accidental change

[oontvoo]

rebase

[oontvoo]

In D114397#3159306, @int3 wrote:

do you 👎 or 👍 the weak-def change?

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

reverted . PTAL!

[int3]

Thanks!

lld/MachO/Symbols.h

239–241

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

29

31

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

[oontvoo]

updated diff

[oontvoo]

thanks!

[oontvoo]

...

lld/MachO/Symbols.h

DylibSymbol(DylibFile *file, StringRefZ name, bool isWeakDef,
               RefState refState, bool isTlv)
       : Symbol(DylibKind, name, file), refState(refState), weakDef(isWeakDef),
         tlv(isTlv) {
     if (file && refState > RefState::Unreferenced)
       file->numReferencedSymbols++;
   }
 
   uint64_t getVA() const override;
-  bool isWeakDef() const override { return weakDef; }
-  bool isWeakRef() const override { return refState == RefState::Weak; }
+
+  // Libraries and framework can be loaded multiple times with different
+  // forceWeakImport (eg., first loaded as strong then as weak)
+  // When that happens we must ensure the weak-property sticks.
+  // Do not cache the result of this expression because `file` could change
+  // as we loaded more libraries/frameworks.
+  bool isWeakDef() const override {
+    return weakDef || (file && getFile()->umbrella->forceWeakImport);
+  }
+  bool isWeakRef() const override {
+    return refState == RefState::Weak ||
+           (file && getFile()->umbrella->forceWeakImport);
+  }
   bool isReferenced() const { return refState != RefState::Unreferenced; }
   bool isTlv() const override { return tlv; }
   bool isDynamicLookup() const { return file == nullptr; }
   bool hasStubsHelper() const { return stubsHelperIndex != UINT32_MAX; }
 
   DylibFile *getFile() const {
     assert(!isDynamicLookup());
     return cast<DylibFile>(file);

lld/test/MachO/weak-import.s

 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/weak-ref-only.s -o %t/weak-ref-only.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/weak-ref-sub-library.s -o %t/weak-ref-sub-library.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/mixed-ref.s -o %t/mixed-ref.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/bar.s -o %t/bar.o
 # RUN: %lld -lSystem -dylib %t/bar.o -o %t/libbar.dylib
 # RUN: %lld -lSystem -dylib %t/foo.o %t/libbar.dylib -sub_library libbar -o %t/libfoo.dylib
 
-# RUN: %lld -weak-lSystem %t/test.o -weak_framework CoreFoundation -weak_library %t/libfoo.dylib -o %t/test
-# RUN: llvm-objdump --macho --all-headers %t/test | FileCheck %s -DDIR=%t --check-prefixes=WEAK-SYS,WEAK-FOO
+# RUN: %lld -weak-lSystem %t/test.o -weak_framework CoreFoundation -weak_library %t/libfoo.dylib -o %t/basic
+# RUN: llvm-objdump --macho --all-headers %t/basic | FileCheck %s -DDIR=%t --check-prefixes=WEAK-SYS,WEAK-FOO
 # RUN: %lld -weak-lSystem %t/test.o \
 # RUN:   -framework CoreFoundation -weak_framework CoreFoundation -framework CoreFoundation \
-# RUN:   %t/libfoo.dylib -weak_library %t/libfoo.dylib %t/libfoo.dylib -o %t/test
-# RUN: llvm-objdump --macho --all-headers %t/test | FileCheck %s -DDIR=%t --check-prefixes=WEAK-SYS,WEAK-FOO
+# RUN:   %t/libfoo.dylib -weak_library %t/libfoo.dylib %t/libfoo.dylib -o %t/basic-weak-strong
+# RUN: llvm-objdump --macho --all-headers %t/basic-weak-strong | FileCheck %s -DDIR=%t --check-prefixes=WEAK-SYS,WEAK-FOO
 # RUN: %lld -lSystem -dylib %t/libfoo.dylib %t/weak-ref-only.o -o %t/weak-ref-only
 # RUN: llvm-objdump --macho --all-headers %t/weak-ref-only | FileCheck %s -DDIR=%t --check-prefixes=SYS,WEAK-FOO
 # RUN: %lld -lSystem -dylib %t/libfoo.dylib %t/weak-ref-sub-library.o -o %t/weak-ref-sub-library
 # RUN: llvm-objdump --macho --all-headers %t/weak-ref-sub-library | FileCheck %s -DDIR=%t --check-prefixes=SYS,WEAK-FOO
 # RUN: %lld -lSystem -dylib %t/libfoo.dylib %t/mixed-ref.o -o %t/mixed-ref
 # RUN: llvm-objdump --macho --all-headers %t/mixed-ref | FileCheck %s -DDIR=%t --check-prefixes=SYS,FOO
 
+# RUN: %lld -framework CoreFoundation %t/test.o -weak_framework CoreFoundation -o %t/strong-weak-import.out
+# RUN: llvm-objdump --macho --bind %t/strong-weak-import.out | FileCheck %s --check-prefix=WEAK-IMP
+# RUN: llvm-objdump --macho --bind %t/basic-weak-strong | FileCheck %s --check-prefix=WEAK-IMP
+
 # WEAK-SYS:          cmd LC_LOAD_WEAK_DYLIB
 # WEAK-SYS-NEXT: cmdsize
 # WEAK-SYS-NEXT:    name /usr/lib/libSystem.dylib
 
 # WEAK-SYS:          cmd LC_LOAD_WEAK_DYLIB
 # WEAK-SYS-NEXT: cmdsize
 # WEAK-SYS-NEXT:    name /System/Library/Frameworks/CoreFoundation.framework/CoreFoundation
 
 # SYS:               cmd LC_LOAD_DYLIB
 # SYS-NEXT:      cmdsize
 # SYS-NEXT:         name /usr/lib/libSystem.dylib
 
 # WEAK-FOO:          cmd LC_LOAD_WEAK_DYLIB
 # WEAK-FOO-NEXT: cmdsize
 # WEAK-FOO-NEXT:    name [[DIR]]/libfoo.dylib
 
 # FOO:               cmd LC_LOAD_DYLIB
 # FOO-NEXT:      cmdsize
 # FOO-NEXT:         name [[DIR]]/libfoo.dylib
 
+# WEAK-IMP: {{.+}} pointer         0 CoreFoundation   __CFBigNumGetInt128 (weak_import)
 #--- foo.s
 .globl _foo
 _foo:
 
 #--- bar.s
 .globl _bar
 _bar:
 

 .weak_definition _foo
 .data
 .quad _foo
 .quad _bar
 
 #--- test.s
 .globl _main
 _main:
+  movq __CFBigNumGetInt128@GOTPCREL(%rip), %rax        
   ret