[PATCH] D114397: [lld-macho] Mark dylib symbols coming from -weak_framework as weak-def.

Mon Nov 29 07:08:49 PST 2021

oontvoo added a comment.

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 <https://reviews.llvm.org/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.

>>> 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)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114397