[PATCH] D93277: [lld-macho] Add implicit dylib support for frameworks

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Thanks! I tested this and put the details in https://bugs.llvm.org/show_bug.cgi?id=48511#c3 and https://bugs.llvm.org/show_bug.cgi?id=48511#c5 . Summary: This is a good change, but it's not sufficient for that bug. One comment about pedantic correctness below :)



================
Comment at: lld/MachO/InputFiles.cpp:514
+
+  // Match /System/Library/Frameworks/$FOO.framework/**/$FOO
+  if (path.consume_front("/System/Library/Frameworks/")) {
----------------
This matches `File<A>::isPublicLocation` in ld64 which lists a few special cases here https://github.com/apple-opensource/ld64/blob/fd3feabb0a1eb18ab5d7910f3c3a5eed99cef6ab/src/ld/parsers/generic_dylib_file.hpp#L490 that this code as-is doesn't quite get right yet I think:

```
		// but only top level framework
		// /System/Library/Frameworks/Foo.framework/Versions/A/Foo                 ==> true
		// /System/Library/Frameworks/Foo.framework/Resources/libBar.dylib         ==> false
		// /System/Library/Frameworks/Foo.framework/Frameworks/Bar.framework/Bar   ==> false
		// /System/Library/Frameworks/Foo.framework/Frameworks/Xfoo.framework/XFoo ==> false
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93277