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? 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 how known_used is to be used needs better documentation. Did you look at how other linkers use known_used? 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.
