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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added a comment.

In D114397#3150677 <https://reviews.llvm.org/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



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114397