[PATCH] D104353: [lld-macho] Avoid force-loading the same archive twice

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: lld/MachO/Driver.cpp:268
         for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
           if (Optional<InputFile *> file = loadArchiveMember(
                   member.mbref, member.modTime, path, /*objCOnly=*/false)) {
----------------
thakis wrote:
> thakis wrote:
> > thakis wrote:
> > > I don't know if that's what's happening, but if loadArchiveMember() loads a .o file that contains LC_LINKER_OPTIONS that has a -l flag that can recursively call addFile() again which can resize loadedArchives and invalidate the cachedFile ref.
> > Reverted in c9b241efd68c for now to green up bots. This sounds like a plausible theory to me.
> > Reverted in c9b241efd68c for now to green up bots. This sounds like a plausible theory to me.
> 
> Not sure why it's only on that bot, but the test failed 3 times in succession:
> 
> https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1608/overview
> https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1609/overview
> https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1610/overview
> 
> But after the revert this test is happy:
> 
> https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1611/overview
> 
> So it looks like the revert helped.
> I don't know if that's what's happening, but if loadArchiveMember() loads a .o file that contains LC_LINKER_OPTIONS that has a -l flag that can recursively call addFile() again which can resize loadedArchives and invalidate the cachedFile ref.

I see you removed the ref in the reland. For completeness, here's a test case to for this. It doesn't trigger a crash even with asan with the first iteration of this patch, but it does cause a recursive call of this function and things only work by luck. (But it's addressed in the reland.)

```
% cat g.cc
void g() {}
% clang -c g.cc
% libtool -static -o libfoo.a g.o
% cat autolink_libfoo.cc
asm(".linker_option \"-lfoo\"");
void g();
void f() { g(); }
% clang -c autolink_libfoo.cc
% libtool -static -o libbar.a autolink_libfoo.o
% cat main_f.cc
void g();
int main() { g(); }
% clang -c main_f.cc
% out/gn/bin/ld64.lld -dynamic -arch x86_64 main_f.o -force_load libbar.a -platform_version macos 10.15.0 10.15.0 -o a.out -L. -lSystem
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104353