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

[Jez Ng via Phabricator via llvm-commits]

int3 requested changes to this revision.
int3 added a comment.
This revision now requires changes to proceed.

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`.



================
Comment at: lld/MachO/Symbols.h:243
+    return refState == RefState::Weak ||
+           (file && dyn_cast<DylibFile>(file)->forceWeakImport);
+  }
----------------
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)


================
Comment at: lld/MachO/Symbols.h:243
+    return refState == RefState::Weak ||
+           (file && dyn_cast<DylibFile>(file)->forceWeakImport);
+  }
----------------
int3 wrote:
> 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)
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)


================
Comment at: lld/test/MachO/weak-import.s:13
+# RUN: %lld -weak-lSystem %t/test.o -weak_framework CoreFoundation -weak_library %t/libfoo.dylib -o %t/test_weak_core
+# RUN: llvm-objdump --macho --all-headers %t/test_weak_core | FileCheck %s -DDIR=%t --check-prefixes=WEAK-SYS,WEAK-FOO
 # RUN: %lld -weak-lSystem %t/test.o \
----------------
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


================
Comment at: lld/test/MachO/weak-import.s:16
 # 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/test_strong_weak_core
+# RUN: llvm-objdump --macho --all-headers %t/test_strong_weak_core | FileCheck %s -DDIR=%t --check-prefixes=WEAK-SYS,WEAK-FOO
----------------
`%t/test-strong-weak`?


================
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
----------------
imo there's no need to create two additional binaries here; we can just reuse the binaries created in lines 16 and 17 above 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114397