On Wed, Aug 21, 2024 at 6:23 AM Richard Biener <richard.guent...@gmail.com> wrote: > > 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
Since linker knows nothing about offload data, I assume "registered as having offload data" means: /* If this is an LTO file without offload, and it is the first LTO file, save the pointer to the last offload file in the list. Further offload LTO files will be inserted after it, if any. */ if (*claimed && !obj.offload && offload_files_last_lto == NULL) offload_files_last_lto = offload_files_last; if (obj.offload && (known_used || obj.found > 0)) { /* 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 with addresses wouldn't match. If a static library contains both LTO and non-LTO objects, ld and gold link them in a different order. */ struct plugin_offload_file *ofld = xmalloc (sizeof (struct plugin_offload_file)); ofld->name = lto_file.name; ofld->next = NULL; > when !KNOWN_USED. *CLAIMED == 1 tells linker that the file is an LTO object file and the LTO plugin also returns the LTO symbol table. KNOWN_USED == 0 tells the LTO plugin not to include this LTO file in the output yet. > 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. *CLAIMED is for linker and KNOWN_USED is for the LTO plugin. The LTO plugin must check KNOWN_USED before including the LTO file in the claimed file list. Maybe CLAIMED isn't a good name. Will CAN_BE_CLAIMED be a better name? > I think the offload hunk you omitted in v2 was correct - if the I will send the v3 patch with this change and update comments. Thanks. > 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. -- H.J.