[Openmp-commits] [PATCH] D142484: [LinkerWrapper] Only import static libraries with needed symbols

Tue Jan 24 12:09:02 PST 2023

jhuber6 marked an inline comment as done.
jhuber6 added a comment.

In D142484#4077979 <https://reviews.llvm.org/D142484#4077979>, @tra wrote:

> We could also use more test cases. E.g. weak symbols (should not cause object extraction)

Yeah, I'll try to add a reasonable test here. It's a little difficult due to the indirect nature of this however.

================
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,
----------------
tra wrote:
> 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.
> 
I just tested it with the same library defining a global `x` for `main.c`.
```
> clang main.c libx.a libx.a libx.a -Wl,--why-extract=-
reference	extracted	symbol
/tmp/main-8a5566.o	libx.a(x.o)	x
```
I could maybe add a similar option here to at least let us know if it extracts. Maybe for some tests.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142484