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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

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



================
Comment at: lld/test/MachO/weak-import.s:25-26
 
+# RUN: %lld -framework CoreFoundation %t/test.o -weak_framework CoreFoundation -o %t/strong_weak_import.out
+# RUN: %lld -weak_framework CoreFoundation %t/test.o -framework CoreFoundation -o %t/weak_strong_import.out
+# RUN: llvm-objdump --macho --bind %t/strong_weak_import.out | FileCheck %s --check-prefix=WEAK-IMP
----------------
oontvoo wrote:
> int3 wrote:
> > imo there's no need to create two additional binaries here; we can just reuse the binaries created in lines 16 and 17 above 
> 25 can be used with 16 - yes.
> but for completeness,  26 is still needed :) (there's no weak-strong )
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114397