tra added a comment. We could also use more test cases. E.g. weak symbols (should not cause object extraction)
================ Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1197 + + Resolved |= ((OldSym & Sym_Undefined && !(OldSym & Sym_Weak)) && + !(*FlagsOrErr & SymbolRef::SF_Undefined)) || ---------------- This could use some comments. @MaskRay's blog post https://maskray.me/blog/2021-06-20-symbol-processing#archive-processing would be helpful to explain what's going on here. I'd also refactor the condition a bit for readability: ``` bool ResolvesStrongReference = (OldSym & Sym_Undefined) && !(OldSym & Sym_Weak) && !(*FlagsOrErr & SymbolRef::SF_Undefined); bool NewGlobalSymbol = ((NewSymbol || (OldSym & Sym_Undefined)) && !(*FlagsOrErr & SymbolRef::SF_Undefined) && !(*FlagsOrErr & SymbolRef::SF_Hidden)); Resolved |= ResolvesStrongReference || NewGlobalSymbol; ``` ================ Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1217-1218 +/// 1) It defines an undefined symbol in a regular object filie. +/// 2) It defines a global symbol without hidden visibility that has not +/// yet been defined. +Expected<bool> getSymbols(StringRef Image, StringSaver &Saver, ---------------- jhuber6 wrote: > tra wrote: > > jhuber6 wrote: > > > tra wrote: > > > > How do we handle conflicting symbols defined more than once? > > > > > > > I just assume that will be caught by the actual linker, it's not relevant > > > for knowing which symbols to pull out. > > That would be the case when we were loading all libraries at once. > > However, now that we load them as needed one by one, we may end up loading > > only the first file which provides such symbol and would potentially ignore > > others, if no other symbol requires loading them. Given that we're checking > > the symbols anyways, it would be useful to diagnose it, IMO. > This is the expected behavior of static libraries right? `lld` has some > option to print out why a certain library was extracted, we could do > something similar? Yes, it is expected. It's also a common source of surprises. I'm not sure whether lld diagnoses it these days. We should follow whatever it does. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142484/new/ https://reviews.llvm.org/D142484 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
