On Wed, Aug 21, 2024 at 2:38 AM Richard Biener <richard.guent...@gmail.com> wrote: > > On Tue, Aug 20, 2024 at 3:24 PM H.J. Lu <hjl.to...@gmail.com> wrote: > > > > On Tue, Aug 20, 2024 at 2:03 AM Richard Biener > > <richard.guent...@gmail.com> wrote: > > > > > > On Wed, Aug 14, 2024 at 3:15 PM H.J. Lu <hjl.to...@gmail.com> wrote: > > > > > > > > The new hook allows the linker plugin to distinguish calls to > > > > claim_file_handler that know the object is being used by the linker > > > > (from ldmain.c:add_archive_element), from calls that don't know it's > > > > being used by the linker (from elf_link_is_defined_archive_symbol); in > > > > the latter case, the plugin should avoid including the unused LTO > > > > archive > > > > members in linker output. To get the proper support for archives with > > > > LTO common symbols, the linker fix for > > > > > > > > https://sourceware.org/bugzilla/show_bug.cgi?id=32083 > > > > > > > > is required. > > > > > > > > PR lto/116361 > > > > * lto-plugin.c (claim_file_handler_v2): Include the LTO object > > > > only if it is known to be used for link output. > > > > > > > > Signed-off-by: H.J. Lu <hjl.to...@gmail.com> > > > > --- > > > > lto-plugin/lto-plugin.c | 20 ++++++++++++-------- > > > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c > > > > index 152648338b9..2d2bfa60d42 100644 > > > > --- a/lto-plugin/lto-plugin.c > > > > +++ b/lto-plugin/lto-plugin.c > > > > @@ -1286,13 +1286,17 @@ claim_file_handler_v2 (const struct > > > > ld_plugin_input_file *file, int *claimed, > > > > lto_file.symtab.syms); > > > > check (status == LDPS_OK, LDPL_FATAL, "could not add symbols"); > > > > > > We are still doing add_symbols, shouldn't what we do depend on what > > > that does? The > > > > If status != LDPS_OK, the plugin will abort because of LDPL_FATAL. > > > > > function comment says > > > > > > If KNOWN_USED, the object is known by the linker > > > to be used, or an older API version is in use that does not provide > > > that > > > information; otherwise, the linker is only determining whether this is > > > a plugin object and it should not be registered as having offload data > > > if > > > not claimed by the plugin. > > > > > > where do you check "if not claimed by the plugin"? I think this at least > > > needs > > > clarification with the change. > > > > See my reply below. > > > > > > - LOCK_SECTION; > > > > - num_claimed_files++; > > > > - claimed_files = > > > > - xrealloc (claimed_files, > > > > - num_claimed_files * sizeof (struct plugin_file_info)); > > > > - claimed_files[num_claimed_files - 1] = lto_file; > > > > - UNLOCK_SECTION; > > > > + /* Include it only if it is known to be used for link output. */ > > > > + if (known_used) > > > > + { > > > > + LOCK_SECTION; > > > > + num_claimed_files++; > > > > + claimed_files = > > > > + xrealloc (claimed_files, > > > > + num_claimed_files * sizeof (struct > > > > plugin_file_info)); > > > > + claimed_files[num_claimed_files - 1] = lto_file; > > > > + UNLOCK_SECTION; > > > > + } > > > > > > > > *claimed = 1; > > > > } > > > > @@ -1313,7 +1317,7 @@ claim_file_handler_v2 (const struct > > > > ld_plugin_input_file *file, int *claimed, > > > > if (*claimed && !obj.offload && offload_files_last_lto == NULL) > > > > offload_files_last_lto = offload_files_last; > > > > > > > > - if (obj.offload && (known_used || obj.found > 0)) > > > > + if (obj.offload && known_used && obj.found > 0) > > > > The offload data is included when it is claimed by the plugin > > even if known_used is 0. It looks quite odd to me. > > To me the whole 'known_used' thing looks odd - I would have expected > the linker to do two round-trips for archives maybe; first with > knwon_used == 0, just getting the add_symbol calls (aka, get > the LTO symbol table), then the linker computes whether the archive > is used and if it is, re-do the claim_file hook with known_used == 1. > > Is that how it is done?
Yes. > Otherwise how should the plugin know whether the file should be added or not? > Will the linker take care of that then? Where is the API documented? I think Yes, linker will do the right thing after https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=a6f8fe0a9e9cbe871652e46ba7c22d5e9fb86208 > how known_used is to be used needs better documentation. The known documentation is in the comments for claim_file_handler_v2. > Did you look at how other linkers use known_used? BFD linker uses it for common symbol support in archive. BFD linker calls claim_file_handler_v2 with known_used == 0 to check for non-common definition in an archive member. If there is one, include the archive member in the output, otherwise, exclude it. Other linkers never do this for common symbols. > Sorry for just asking questions and having no answers ;) > > Richard. > > > Since > > can't test it and it isn't needed for PR lto/116361, I dropped > > this change in the v2 patch: > > > > https://gcc.gnu.org/pipermail/gcc-patches/2024-August/660539.html > > > > If you agree that this change is correct, I can include it and update > > comments. > > > > > > { > > > > /* Add file to the list. The order must be exactly the same as > > > > the final > > > > order after recompilation and linking, otherwise host and > > > > target tables > > > > -- > > > > 2.46.0 > > > > > > > > > > > > -- > > H.J. -- H.J.
