| Message ID | 20240814131334.3217622-1-hjl.tools@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Update LDPT_REGISTER_CLAIM_FILE_HOOK_V2 linker plugin hook | expand |
On Wed, Aug 14, 2024 at 3:15 PM H.J. Lu <hjl.tools@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.tools@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 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. > - 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) > { > /* 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 >
On Tue, Aug 20, 2024 at 2:03 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Wed, Aug 14, 2024 at 3:15 PM H.J. Lu <hjl.tools@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.tools@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. 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 > >
On Tue, Aug 20, 2024 at 3:24 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Tue, Aug 20, 2024 at 2:03 AM Richard Biener > <richard.guenther@gmail.com> wrote: > > > > On Wed, Aug 14, 2024 at 3:15 PM H.J. Lu <hjl.tools@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.tools@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.
On Wed, Aug 21, 2024 at 2:38 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Tue, Aug 20, 2024 at 3:24 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > On Tue, Aug 20, 2024 at 2:03 AM Richard Biener > > <richard.guenther@gmail.com> wrote: > > > > > > On Wed, Aug 14, 2024 at 3:15 PM H.J. Lu <hjl.tools@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.tools@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.
On Wed, Aug 21, 2024 at 2:27 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Wed, Aug 21, 2024 at 2:38 AM Richard Biener > <richard.guenther@gmail.com> wrote: > > > > On Tue, Aug 20, 2024 at 3:24 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > On Tue, Aug 20, 2024 at 2:03 AM Richard Biener > > > <richard.guenther@gmail.com> wrote: > > > > > > > > On Wed, Aug 14, 2024 at 3:15 PM H.J. Lu <hjl.tools@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.tools@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.
On Wed, Aug 21, 2024 at 6:23 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Wed, Aug 21, 2024 at 2:27 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > On Wed, Aug 21, 2024 at 2:38 AM Richard Biener > > <richard.guenther@gmail.com> wrote: > > > > > > On Tue, Aug 20, 2024 at 3:24 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > > > On Tue, Aug 20, 2024 at 2:03 AM Richard Biener > > > > <richard.guenther@gmail.com> wrote: > > > > > > > > > > On Wed, Aug 14, 2024 at 3:15 PM H.J. Lu <hjl.tools@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.tools@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.
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"); - 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) { /* 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
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.tools@gmail.com> --- lto-plugin/lto-plugin.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)