On Wed, Aug 21, 2024 at 2:27 PM H.J. Lu <hjl.to...@gmail.com> wrote: > > 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.
OK, I find that lacking. Specifically ", 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." what is "registered as having offload data", isn't that callinng add_symbols? The docs do not say what to do with *CLAIMED when !KNOWN_USED. That said, since you said the above is what BFD will do the change is reasonable, never CLAIM the file when !KNOWN_USED, but your patch still sets *CLAIM to 1 it just avoids adding to the plugin-internal claimed_files? But how does the linker know it should call the hook again then? The interface is a bit weird. I think the offload hunk you omitted in v2 was correct - if the linker will now call the claim file handler twice, once with !known_used and possibly once with known_used we shouldn't add the object two times. Richard. > > 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.
