lto-plugin: add support for feature detection

  The patch is a follow-up of the discussion we've got in:
https://gcc.gnu.org/pipermail/gcc-patches/2022-May/593901.html

Mold linker would appreciate knowing in advance if get_symbols_v3 is supported
by a GCC plug-in or not.

Ready to be installed?
Thanks,
Martin

lto-plugin/ChangeLog:

	* lto-plugin.c (supports_get_symbols_v3): Add symbol.
---
 lto-plugin/lto-plugin.c | 5 +++++
 1 file changed, 5 insertions(+)
  

On Wed, 4 May 2022, Martin Liška wrote:

> The patch is a follow-up of the discussion we've got in:
> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/593901.html
> 
> Mold linker would appreciate knowing in advance if get_symbols_v3 is supported
> by a GCC plug-in or not.
> 
> Ready to be installed?

Quick note: if the linker is supposed to check for presence of this symbol
via dlsym(), I expect it won't work as-is since lto-plugin.map hides every
symbol except 'onload'.

(also it might be nicer to reword the comment to say 'Presence of the following
symbols is used for ...', because you're leaving the value as 'false').

Alexander

> Thanks,
> Martin
> 
> lto-plugin/ChangeLog:
> 
> 	* lto-plugin.c (supports_get_symbols_v3): Add symbol.
> ---
>  lto-plugin/lto-plugin.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> index 47378435612..049f3841d5b 100644
> --- a/lto-plugin/lto-plugin.c
> +++ b/lto-plugin/lto-plugin.c
> @@ -1554,3 +1554,8 @@ onload (struct ld_plugin_tv *tv)
>  
>    return LDPS_OK;
>  }
> +
> +/* The following symbols are used for dynamic detection of plug-in features
> +   from linker side.  */
> +
> +bool supports_get_symbols_v3;
>
  

On 5/4/22 14:32, Alexander Monakov wrote:
> On Wed, 4 May 2022, Martin Liška wrote:
> 
>> The patch is a follow-up of the discussion we've got in:
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/593901.html
>>
>> Mold linker would appreciate knowing in advance if get_symbols_v3 is supported
>> by a GCC plug-in or not.
>>
>> Ready to be installed?
> 
> Quick note: if the linker is supposed to check for presence of this symbol
> via dlsym(), I expect it won't work as-is since lto-plugin.map hides every
> symbol except 'onload'.

Oh, good point!

Changing that, I get now:

$ nm ./lto-plugin/.libs/liblto_plugin.so | grep v3
0000000000015008 D supports_get_symbols_v3

> 
> (also it might be nicer to reword the comment to say 'Presence of the following
> symbols is used for ...', because you're leaving the value as 'false').

Sure. I'm also changing it's default value.

Sending v2.

Cheers,
Martin

> 
> Alexander
> 
>> Thanks,
>> Martin
>>
>> lto-plugin/ChangeLog:
>>
>> 	* lto-plugin.c (supports_get_symbols_v3): Add symbol.
>> ---
>>  lto-plugin/lto-plugin.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
>> index 47378435612..049f3841d5b 100644
>> --- a/lto-plugin/lto-plugin.c
>> +++ b/lto-plugin/lto-plugin.c
>> @@ -1554,3 +1554,8 @@ onload (struct ld_plugin_tv *tv)
>>  
>>    return LDPS_OK;
>>  }
>> +
>> +/* The following symbols are used for dynamic detection of plug-in features
>> +   from linker side.  */
>> +
>> +bool supports_get_symbols_v3;
  

On Wed, 4 May 2022, Martin Liška wrote:

> On 5/4/22 14:32, Alexander Monakov wrote:
> > On Wed, 4 May 2022, Martin Liška wrote:
> > 
> >> The patch is a follow-up of the discussion we've got in:
> >> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/593901.html
> >>
> >> Mold linker would appreciate knowing in advance if get_symbols_v3 is supported
> >> by a GCC plug-in or not.

Out of curiousity, I looked at mold, and I don't understand what problem this
detection is solving, nor why this is the best way to solve that. Was there
some discussion with mold author I should check out?

Note that mold takes this not only as 'v3 API is supported', but, more
importantly, as 'v2 entrypoint will not be called'.

Alexander
  

On 5/4/22 15:10, Alexander Monakov wrote:
> On Wed, 4 May 2022, Martin Liška wrote:
> 
>> On 5/4/22 14:32, Alexander Monakov wrote:
>>> On Wed, 4 May 2022, Martin Liška wrote:
>>>
>>>> The patch is a follow-up of the discussion we've got in:
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/593901.html
>>>>
>>>> Mold linker would appreciate knowing in advance if get_symbols_v3 is supported
>>>> by a GCC plug-in or not.
> 
> Out of curiousity, I looked at mold, and I don't understand what problem this
> detection is solving, nor why this is the best way to solve that. Was there
> some discussion with mold author I should check out?

Sure, please take a look at this issue:
https://github.com/rui314/mold/issues/454#issuecomment-1116849458

> 
> Note that mold takes this not only as 'v3 API is supported', but, more
> importantly, as 'v2 entrypoint will not be called'.

Yes, if they register get_symbols_v3, then it will be called. That's how the
plug-in works.

Martin

> 
> Alexander
  

On Wed, 4 May 2022 15:31:32 +0200
Martin Liška <mliska@suse.cz> wrote:

> On 5/4/22 15:10, Alexander Monakov wrote:
> > On Wed, 4 May 2022, Martin Liška wrote:
> >   
> >> On 5/4/22 14:32, Alexander Monakov wrote:  
> >>> On Wed, 4 May 2022, Martin Liška wrote:
> >>>  
> >>>> The patch is a follow-up of the discussion we've got in:
> >>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/593901.html
> >>>>
> >>>> Mold linker would appreciate knowing in advance if get_symbols_v3 is supported
> >>>> by a GCC plug-in or not.  
> > 
> > Out of curiousity, I looked at mold, and I don't understand what problem this
> > detection is solving, nor why this is the best way to solve that. Was there
> > some discussion with mold author I should check out?  
> 
> Sure, please take a look at this issue:
> https://github.com/rui314/mold/issues/454#issuecomment-1116849458
> 
> > 
> > Note that mold takes this not only as 'v3 API is supported', but, more
> > importantly, as 'v2 entrypoint will not be called'.  
> 
> Yes, if they register get_symbols_v3, then it will be called. That's how the
> plug-in works.

FWIW, I usually use an alias to some existing, exported symbol, like:
/* Announce that we are GPL.  */
#if defined __GNUC__
extern __typeof (plugin_init) plugin_is_GPL_compatible __attribute__ ((alias ("plugin_init")));
#else
unsigned char plugin_is_GPL_compatible;
#endif
  

On Wed, May 4, 2022 at 3:31 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/4/22 15:10, Alexander Monakov wrote:
> > On Wed, 4 May 2022, Martin Liška wrote:
> >
> >> On 5/4/22 14:32, Alexander Monakov wrote:
> >>> On Wed, 4 May 2022, Martin Liška wrote:
> >>>
> >>>> The patch is a follow-up of the discussion we've got in:
> >>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/593901.html
> >>>>
> >>>> Mold linker would appreciate knowing in advance if get_symbols_v3 is supported
> >>>> by a GCC plug-in or not.
> >
> > Out of curiousity, I looked at mold, and I don't understand what problem this
> > detection is solving, nor why this is the best way to solve that. Was there
> > some discussion with mold author I should check out?
>
> Sure, please take a look at this issue:
> https://github.com/rui314/mold/issues/454#issuecomment-1116849458
>
> >
> > Note that mold takes this not only as 'v3 API is supported', but, more
> > importantly, as 'v2 entrypoint will not be called'.
>
> Yes, if they register get_symbols_v3, then it will be called. That's how the
> plug-in works.

I think they should simply try to not register LDPT_GET_SYMBOLS or
LDPT_GET_SYMBOLS_V2 with the plugin in the onload hook and if
that fails they will know the plugin doesn't support V3 only.  I suppose
it should work to call onload() multiple times (when only increasing the
set of supported functions) until it returns LDPS_OK without intermediately
dlclosing it (maybe call cleanup_handler inbertween).  This should work
for old plugin versions.

That said, a better API extension compared to adding some random
symbol like you propose is to enhance the return value from onload(),
maybe returning an alternate transfer vector specifying symbol entries
that will not be used (or return a transfer vector that will be used).
We've been mostly versioning the symbol related hooks here.

That said, I do not like at all this proposed add of a special symbol
to flag exclusive v3 use.  That's a hack and not extensible at all.

Richard.

> Martin
>
> >
> > Alexander
>
  

On Thu, May 5, 2022 at 8:15 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Wed, May 4, 2022 at 3:31 PM Martin Liška <mliska@suse.cz> wrote:
> >
> > On 5/4/22 15:10, Alexander Monakov wrote:
> > > On Wed, 4 May 2022, Martin Liška wrote:
> > >
> > >> On 5/4/22 14:32, Alexander Monakov wrote:
> > >>> On Wed, 4 May 2022, Martin Liška wrote:
> > >>>
> > >>>> The patch is a follow-up of the discussion we've got in:
> > >>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/593901.html
> > >>>>
> > >>>> Mold linker would appreciate knowing in advance if get_symbols_v3 is supported
> > >>>> by a GCC plug-in or not.
> > >
> > > Out of curiousity, I looked at mold, and I don't understand what problem this
> > > detection is solving, nor why this is the best way to solve that. Was there
> > > some discussion with mold author I should check out?
> >
> > Sure, please take a look at this issue:
> > https://github.com/rui314/mold/issues/454#issuecomment-1116849458
> >
> > >
> > > Note that mold takes this not only as 'v3 API is supported', but, more
> > > importantly, as 'v2 entrypoint will not be called'.
> >
> > Yes, if they register get_symbols_v3, then it will be called. That's how the
> > plug-in works.
>
> I think they should simply try to not register LDPT_GET_SYMBOLS or
> LDPT_GET_SYMBOLS_V2 with the plugin in the onload hook and if
> that fails they will know the plugin doesn't support V3 only.  I suppose
> it should work to call onload() multiple times (when only increasing the
> set of supported functions) until it returns LDPS_OK without intermediately
> dlclosing it (maybe call cleanup_handler inbertween).  This should work
> for old plugin versions.
>
> That said, a better API extension compared to adding some random
> symbol like you propose is to enhance the return value from onload(),
> maybe returning an alternate transfer vector specifying symbol entries
> that will not be used (or return a transfer vector that will be used).
> We've been mostly versioning the symbol related hooks here.
>
> That said, I do not like at all this proposed add of a special symbol
> to flag exclusive v3 use.  That's a hack and not extensible at all.

Speaking of which, onload_v2 would be in order and should possibly
return some instantiation handle of the plugin that the linker could
instruct the plugin to dispose (reset ()?).  I see the GCC implementation
of the plugin just has a single global state and it doesn't seem that it's
prepared for multiple onload() calls (but it might work by accident if
you never remove things from the support vector but only add).

Without revamping the whole API onload_v2 could set the current
global state for the plugin based on the transfer vector and the reset()
API would discard the state (might also be redundant and implicitely
performed by the next onload_v2 call).

onload_v2 could then also have an "output" transfer vector where the
plugin simply copies the entries it picked and dropped those it will
never call.  We'd document the plugin may only pick _one_ of the versioned
API variants.

That said, the heuristic outlined above might still work with the present
onload() API and existing implementations.

Richard.

> Richard.
>
> > Martin
> >
> > >
> > > Alexander
> >
  

On Thu, 5 May 2022, Richard Biener via Gcc-patches wrote:

> > I think they should simply try to not register LDPT_GET_SYMBOLS or
> > LDPT_GET_SYMBOLS_V2 with the plugin in the onload hook and if
> > that fails they will know the plugin doesn't support V3 only.  I suppose
> > it should work to call onload() multiple times (when only increasing the
> > set of supported functions) until it returns LDPS_OK without intermediately
> > dlclosing it (maybe call cleanup_handler inbertween).  This should work
> > for old plugin versions.
> >
> > That said, a better API extension compared to adding some random
> > symbol like you propose is to enhance the return value from onload(),
> > maybe returning an alternate transfer vector specifying symbol entries
> > that will not be used (or return a transfer vector that will be used).
> > We've been mostly versioning the symbol related hooks here.
> >
> > That said, I do not like at all this proposed add of a special symbol
> > to flag exclusive v3 use.  That's a hack and not extensible at all.
> 
> Speaking of which, onload_v2 would be in order and should possibly
> return some instantiation handle of the plugin that the linker could
> instruct the plugin to dispose (reset ()?).  I see the GCC implementation
> of the plugin just has a single global state and it doesn't seem that it's
> prepared for multiple onload() calls (but it might work by accident if
> you never remove things from the support vector but only add).
> 
> Without revamping the whole API onload_v2 could set the current
> global state for the plugin based on the transfer vector and the reset()
> API would discard the state (might also be redundant and implicitely
> performed by the next onload_v2 call).
> 
> onload_v2 could then also have an "output" transfer vector where the
> plugin simply copies the entries it picked and dropped those it will
> never call.  We'd document the plugin may only pick _one_ of the versioned
> API variants.
> 
> That said, the heuristic outlined above might still work with the present
> onload() API and existing implementations.

Feels a bit weird to ask, but before entertaining such an API extension,
can we step back and understand the v3 variant of get_symbols? It is not
documented, and from what little I saw I did not get the "motivation" for
its existence (what it is doing that couldn't be done with the v2 api).

To me lack of documentation looks like a serious issue :/

Likewise, I don't really understand why mold cannot be flexible and
efficiently service both the v2 and v3 variants without committing 
to one of those in advance.

Alexander
  

On 5/5/22 12:52, Alexander Monakov wrote:
> Feels a bit weird to ask, but before entertaining such an API extension,
> can we step back and understand the v3 variant of get_symbols? It is not
> documented, and from what little I saw I did not get the "motivation" for
> its existence (what it is doing that couldn't be done with the v2 api).

Please see here:
https://github.com/rui314/mold/issues/181#issuecomment-1037927757

> 
> To me lack of documentation looks like a serious issue :/

Yes, documentation is missing. This is what can be seen from gold's implementation:

// Get the symbol resolution info for a plugin-claimed input file.

static enum ld_plugin_status
get_symbols(const void* handle, int nsyms, ld_plugin_symbol* syms)
...

// Version 2 of the above.  The only difference is that this version
// is allowed to return the resolution code LDPR_PREVAILING_DEF_IRONLY_EXP.


// Version 3 of the above.  The only difference from v2 is that it
// returns LDPS_NO_SYMS instead of LDPS_OK for the objects we never
// decided to include.

static enum ld_plugin_status
get_symbols_v3(const void* handle, int nsyms, ld_plugin_symbol* syms)

Which is something like documentation :(

Martin
  

On Thu, 5 May 2022, Martin Liška wrote:

> On 5/5/22 12:52, Alexander Monakov wrote:
> > Feels a bit weird to ask, but before entertaining such an API extension,
> > can we step back and understand the v3 variant of get_symbols? It is not
> > documented, and from what little I saw I did not get the "motivation" for
> > its existence (what it is doing that couldn't be done with the v2 api).
> 
> Please see here:
> https://github.com/rui314/mold/issues/181#issuecomment-1037927757

Thanks. I've also re-read [1] and [2] which provided some relevant ideas.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86490
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=23411


OK, so the crux of the issue is that sometimes the linker needs to feed the
compiler plugin with LTO .o files extracted from static archives. This is
not really obvious, because normally .a archives have an index that enumerates
symbols defined/used by its .o files, and even during LTO the linker can simply
consult the index to find out which members to extract.  In theory, at least.

The theory breaks in the following cases:

 - ld.bfd and common symbols (I wonder if weak/comdat code is also affected?):
 archive index does not indicate which definitions are common, so ld.bfd
 extracts the member and feeds it to the plugin to find out;

 - ld.gold and emulated archives via --start-lib a.o b.o ... --end-lib: here
 there's no index to consult and ld.gold feeds each .o to the plugin.

In those cases it may happen that the linker extracts an .o file that would
not be extracted during non-LTO link, and if that happens, the linker needs to
inform the plugin. This is not the same as marking each symbol from spuriously
extracted .o file as PREEMPTED when the .o file has constructors (the plugin
will assume the constructors are kept while the linker needs to discard them).

So get_symbols_v3 allows the linker to discard an LTO .o file to solve this.

In absence of get_symbols_v3 mold tries to ensure correctness by restarting
itself while appending a list of .o files to be discarded to its command line.

I wonder if mold can invoke plugin cleanup callback to solve this without
restarting.

(also, hm, it seems to confirm my idea that LTO .o files should have had the
correct symbol table so normal linker algorithms would work)

Hopefully this was useful.
Alexander
  

CCing mold's author.

On 5/6/22 16:46, Alexander Monakov wrote:
> 
> 
> On Thu, 5 May 2022, Martin Liška wrote:
> 
>> On 5/5/22 12:52, Alexander Monakov wrote:
>>> Feels a bit weird to ask, but before entertaining such an API extension,
>>> can we step back and understand the v3 variant of get_symbols? It is not
>>> documented, and from what little I saw I did not get the "motivation" for
>>> its existence (what it is doing that couldn't be done with the v2 api).
>>
>> Please see here:
>> https://github.com/rui314/mold/issues/181#issuecomment-1037927757
> 
> Thanks. I've also re-read [1] and [2] which provided some relevant ideas.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86490
> [2] https://sourceware.org/bugzilla/show_bug.cgi?id=23411
> 
> 
> OK, so the crux of the issue is that sometimes the linker needs to feed the
> compiler plugin with LTO .o files extracted from static archives. This is
> not really obvious, because normally .a archives have an index that enumerates
> symbols defined/used by its .o files, and even during LTO the linker can simply
> consult the index to find out which members to extract.  In theory, at least.
> 
> The theory breaks in the following cases:
> 
>  - ld.bfd and common symbols (I wonder if weak/comdat code is also affected?):
>  archive index does not indicate which definitions are common, so ld.bfd
>  extracts the member and feeds it to the plugin to find out;
> 
>  - ld.gold and emulated archives via --start-lib a.o b.o ... --end-lib: here
>  there's no index to consult and ld.gold feeds each .o to the plugin.
> 
> In those cases it may happen that the linker extracts an .o file that would
> not be extracted during non-LTO link, and if that happens, the linker needs to
> inform the plugin. This is not the same as marking each symbol from spuriously
> extracted .o file as PREEMPTED when the .o file has constructors (the plugin
> will assume the constructors are kept while the linker needs to discard them).
> 
> So get_symbols_v3 allows the linker to discard an LTO .o file to solve this.
> 
> In absence of get_symbols_v3 mold tries to ensure correctness by restarting
> itself while appending a list of .o files to be discarded to its command line.
> 
> I wonder if mold can invoke plugin cleanup callback to solve this without
> restarting.
> 
> (also, hm, it seems to confirm my idea that LTO .o files should have had the
> correct symbol table so normal linker algorithms would work)
> 
> Hopefully this was useful.
> Alexander
  

On Fri, May 6, 2022 at 10:47 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
>
> On Thu, 5 May 2022, Martin Liška wrote:
>
> > On 5/5/22 12:52, Alexander Monakov wrote:
> > > Feels a bit weird to ask, but before entertaining such an API extension,
> > > can we step back and understand the v3 variant of get_symbols? It is not
> > > documented, and from what little I saw I did not get the "motivation" for
> > > its existence (what it is doing that couldn't be done with the v2 api).
> >
> > Please see here:
> > https://github.com/rui314/mold/issues/181#issuecomment-1037927757
>
> Thanks. I've also re-read [1] and [2] which provided some relevant ideas.
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86490
> [2] https://sourceware.org/bugzilla/show_bug.cgi?id=23411
>
>
> OK, so the crux of the issue is that sometimes the linker needs to feed the
> compiler plugin with LTO .o files extracted from static archives. This is
> not really obvious, because normally .a archives have an index that enumerates
> symbols defined/used by its .o files, and even during LTO the linker can simply
> consult the index to find out which members to extract.  In theory, at least.
>
> The theory breaks in the following cases:
>
>  - ld.bfd and common symbols (I wonder if weak/comdat code is also affected?):
>  archive index does not indicate which definitions are common, so ld.bfd
>  extracts the member and feeds it to the plugin to find out;
>
>  - ld.gold and emulated archives via --start-lib a.o b.o ... --end-lib: here
>  there's no index to consult and ld.gold feeds each .o to the plugin.
>
> In those cases it may happen that the linker extracts an .o file that would
> not be extracted during non-LTO link, and if that happens, the linker needs to
> inform the plugin. This is not the same as marking each symbol from spuriously
> extracted .o file as PREEMPTED when the .o file has constructors (the plugin
> will assume the constructors are kept while the linker needs to discard them).
>
> So get_symbols_v3 allows the linker to discard an LTO .o file to solve this.
>
> In absence of get_symbols_v3 mold tries to ensure correctness by restarting
> itself while appending a list of .o files to be discarded to its command line.
>
> I wonder if mold can invoke plugin cleanup callback to solve this without
> restarting.

We can call the plugin cleanup callback from mold, but there are a few problems:

First of all, it looks like it is not clear what state the plugin
cleanup callback resets to.
It may reset it to the initial state with which we need to restart
everything from calling
`onload` callback, or it may not deregister functions registered by
the previous `onload`
call. Since the exact semantics is not documented, the LLVM gold
plugin may behave
differently than the GCC plugin.

Second, if we reset a plugin's internal state, we need to register all
input files by calling
the `claim_file_hook` callback, which in turn calls the `add_symbols`
callback. But we
don't need any symbol information at this point because mold already
knows what are
in LTO object files as it calls `claim_file_hook` already on the same
sets of files. So the
`add_symbols` invocations would be ignored, which is a waste of resources.

So, I prefer get_symbols_v3 over calling the plugin cleanup callback.

> (also, hm, it seems to confirm my idea that LTO .o files should have had the
> correct symbol table so normal linker algorithms would work)

I agree. If GCC LTO object file contains a correct ELF symbol table,
we can also eliminate
the need of the special LTO-aware ar command. It looks like it is a
very common error to
use an ar command that doesn't understand the LTO object file, which
results in mysterious
"undefined symbol" errors even though the object files in an archive
file provide that very
symbols.
  

On Sun, 15 May 2022, Rui Ueyama wrote:

[snip]
> > So get_symbols_v3 allows the linker to discard an LTO .o file to solve this.
> >
> > In absence of get_symbols_v3 mold tries to ensure correctness by restarting
> > itself while appending a list of .o files to be discarded to its command line.
> >
> > I wonder if mold can invoke plugin cleanup callback to solve this without
> > restarting.
> 
> We can call the plugin cleanup callback from mold, but there are a few problems:
> 
> First of all, it looks like it is not clear what state the plugin cleanup
> callback resets to.  It may reset it to the initial state with which we
> need to restart everything from calling `onload` callback, or it may not
> deregister functions registered by the previous `onload` call. Since the
> exact semantics is not documented, the LLVM gold plugin may behave
> differently than the GCC plugin.

Ack, that looks risky (and probably unnecessary, see below).

> Second, if we reset a plugin's internal state, we need to register all
> input files by calling the `claim_file_hook` callback, which in turn calls
> the `add_symbols` callback. But we don't need any symbol information at
> this point because mold already knows what are in LTO object files as it
> calls `claim_file_hook` already on the same sets of files. So the
> `add_symbols` invocations would be ignored, which is a waste of resources.
> 
> So, I prefer get_symbols_v3 over calling the plugin cleanup callback.

Oh, to be clear I wouldn't argue against implementing get_symbols_v3 in GCC.
I was wondering if mold can solve this in another fashion without the
self-restart trick.

If I understood your design correctly, mold disregards the index in static
archives, because it doesn't give you the dependency graph of contained
objects (it only lists defined symbols, not used, I was mistaken about that
in the previous email), and you wanted to let mold parse all archived
objects in parallel instead of doing a multiphase scan where each phase
extracts only the needed objects (in parallel). Is that correct?

Is that a good tradeoff in the LTO case though? I believe you cannot assume
the plugin to be thread-safe, so you're serializing its API calls, right?
But the plugin is doing a lot of work, so using the index to feed it with as
few LTO objects as possible should be a significant win, no? (even if it
was thread-safe)

And with the index, it should be rare that a file is spuriously added to the
plugin, so maybe you could get away with issuing a warning or an error when
the v2 API is used, but mold needs to discard a file?

> > (also, hm, it seems to confirm my idea that LTO .o files should have had the
> > correct symbol table so normal linker algorithms would work)
> 
> I agree. If GCC LTO object file contains a correct ELF symbol table, we
> can also eliminate the need of the special LTO-aware ar command. It looks
> like it is a very common error to use an ar command that doesn't
> understand the LTO object file, which results in mysterious "undefined
> symbol" errors even though the object files in an archive file provide
> that very symbols.

Thanks.
Alexander
  

On Sun, May 15, 2022 at 3:53 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Sun, 15 May 2022, Rui Ueyama wrote:
>
> [snip]
> > > So get_symbols_v3 allows the linker to discard an LTO .o file to solve this.
> > >
> > > In absence of get_symbols_v3 mold tries to ensure correctness by restarting
> > > itself while appending a list of .o files to be discarded to its command line.
> > >
> > > I wonder if mold can invoke plugin cleanup callback to solve this without
> > > restarting.
> >
> > We can call the plugin cleanup callback from mold, but there are a few problems:
> >
> > First of all, it looks like it is not clear what state the plugin cleanup
> > callback resets to.  It may reset it to the initial state with which we
> > need to restart everything from calling `onload` callback, or it may not
> > deregister functions registered by the previous `onload` call. Since the
> > exact semantics is not documented, the LLVM gold plugin may behave
> > differently than the GCC plugin.
>
> Ack, that looks risky (and probably unnecessary, see below).
>
> > Second, if we reset a plugin's internal state, we need to register all
> > input files by calling the `claim_file_hook` callback, which in turn calls
> > the `add_symbols` callback. But we don't need any symbol information at
> > this point because mold already knows what are in LTO object files as it
> > calls `claim_file_hook` already on the same sets of files. So the
> > `add_symbols` invocations would be ignored, which is a waste of resources.
> >
> > So, I prefer get_symbols_v3 over calling the plugin cleanup callback.
>
> Oh, to be clear I wouldn't argue against implementing get_symbols_v3 in GCC.
> I was wondering if mold can solve this in another fashion without the
> self-restart trick.
>
> If I understood your design correctly, mold disregards the index in static
> archives, because it doesn't give you the dependency graph of contained
> objects (it only lists defined symbols, not used, I was mistaken about that
> in the previous email), and you wanted to let mold parse all archived
> objects in parallel instead of doing a multiphase scan where each phase
> extracts only the needed objects (in parallel). Is that correct?

Correct.

> Is that a good tradeoff in the LTO case though? I believe you cannot assume
> the plugin to be thread-safe, so you're serializing its API calls, right?
> But the plugin is doing a lot of work, so using the index to feed it with as
> few LTO objects as possible should be a significant win, no? (even if it
> was thread-safe)

Oh, I didn't know that claim_file_hook isn't thread-safe. I need to
add a lock to
guard it then. But is it actually the case?

As to the tradeoff, speculatively loading all object files from
archives may not be
beneficial if the loaded files are LTO object files. But we do this
for consistency.
We don't have a multi-phase name resolution pass at all in mold; all symbols are
resolved at once in parallel. We don't want to implement another name resolution
pass just for LTO for the following reasons:

1. It bloats up the linker's code.

2. We don't know whether an archive file contains an LTO object file
or not until
we actually read archive members, so there's no chance to switch to
the multi-pass
name resolution algorithm before we read files.

3. If we have two different name resolution algorithms, it is very
hard to guarantee
that both algorithms produce the same result. As a result, the output
with -flto may
behave differently without -flto.

4. We still have to handle --start-libs and --end-libs, so feeding an
object file that
will end up not being included into the output is unavoidable.

> And with the index, it should be rare that a file is spuriously added to the
> plugin, so maybe you could get away with issuing a warning or an error when
> the v2 API is used, but mold needs to discard a file?
>
> > > (also, hm, it seems to confirm my idea that LTO .o files should have had the
> > > correct symbol table so normal linker algorithms would work)
> >
> > I agree. If GCC LTO object file contains a correct ELF symbol table, we
> > can also eliminate the need of the special LTO-aware ar command. It looks
> > like it is a very common error to use an ar command that doesn't
> > understand the LTO object file, which results in mysterious "undefined
> > symbol" errors even though the object files in an archive file provide
> > that very symbols.
>
> Thanks.
> Alexander
  

On Sun, 15 May 2022, Rui Ueyama wrote:

> > Is that a good tradeoff in the LTO case though? I believe you cannot assume
> > the plugin to be thread-safe, so you're serializing its API calls, right?
> > But the plugin is doing a lot of work, so using the index to feed it with as
> > few LTO objects as possible should be a significant win, no? (even if it
> > was thread-safe)
> 
> Oh, I didn't know that claim_file_hook isn't thread-safe. I need to add a
> lock to guard it then. But is it actually the case?

You can see for yourself at
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=lto-plugin/lto-plugin.c
(e.g. how claim_file_handler increments the global variable num_claimed_files)

> As to the tradeoff, speculatively loading all object files from archives
> may not be beneficial if the loaded files are LTO object files. But we do
> this for consistency.  We don't have a multi-phase name resolution pass at
> all in mold; all symbols are resolved at once in parallel. We don't want
> to implement another name resolution pass just for LTO for the following
> reasons:
> 
> 1. It bloats up the linker's code.
> 
> 2. We don't know whether an archive file contains an LTO object file or
> not until we actually read archive members, so there's no chance to switch
> to the multi-pass name resolution algorithm before we read files.
> 
> 3. If we have two different name resolution algorithms, it is very hard to
> guarantee that both algorithms produce the same result. As a result, the
> output with -flto may behave differently without -flto.

Well, -flto can result in observably different results for other reasons
anyway.

> 4. We still have to handle --start-libs and --end-libs, so feeding an
> object file that will end up not being included into the output is
> unavoidable.

Makes sense, but I still don't understand why mold wants to discover in
advance whether the plugin is going to use get_symbols_v3. How would it
help with what mold does today to handle the _v2 case?

Alexander
  

On Sun, May 15, 2022 at 4:51 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Sun, 15 May 2022, Rui Ueyama wrote:
>
> > > Is that a good tradeoff in the LTO case though? I believe you cannot assume
> > > the plugin to be thread-safe, so you're serializing its API calls, right?
> > > But the plugin is doing a lot of work, so using the index to feed it with as
> > > few LTO objects as possible should be a significant win, no? (even if it
> > > was thread-safe)
> >
> > Oh, I didn't know that claim_file_hook isn't thread-safe. I need to add a
> > lock to guard it then. But is it actually the case?
>
> You can see for yourself at
> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=lto-plugin/lto-plugin.c
> (e.g. how claim_file_handler increments the global variable num_claimed_files)
>
> > As to the tradeoff, speculatively loading all object files from archives
> > may not be beneficial if the loaded files are LTO object files. But we do
> > this for consistency.  We don't have a multi-phase name resolution pass at
> > all in mold; all symbols are resolved at once in parallel. We don't want
> > to implement another name resolution pass just for LTO for the following
> > reasons:
> >
> > 1. It bloats up the linker's code.
> >
> > 2. We don't know whether an archive file contains an LTO object file or
> > not until we actually read archive members, so there's no chance to switch
> > to the multi-pass name resolution algorithm before we read files.
> >
> > 3. If we have two different name resolution algorithms, it is very hard to
> > guarantee that both algorithms produce the same result. As a result, the
> > output with -flto may behave differently without -flto.
>
> Well, -flto can result in observably different results for other reasons
> anyway.
>
> > 4. We still have to handle --start-libs and --end-libs, so feeding an
> > object file that will end up not being included into the output is
> > unavoidable.
>
> Makes sense, but I still don't understand why mold wants to discover in
> advance whether the plugin is going to use get_symbols_v3. How would it
> help with what mold does today to handle the _v2 case?

Currently, mold restarts itself to reset the internal state of the plugin.
If we know in advance that get_symbols_v3 is supported, we can avoid that
restart. That should make the linker a bit faster. Also, restarting
the linker is
a hack, so we want to avoid it if possible.
  

On Sun, 15 May 2022, Rui Ueyama wrote:

> > Makes sense, but I still don't understand why mold wants to discover in
> > advance whether the plugin is going to use get_symbols_v3. How would it
> > help with what mold does today to handle the _v2 case?
> 
> Currently, mold restarts itself to reset the internal state of the plugin.
> If we know in advance that get_symbols_v3 is supported, we can avoid that
> restart. That should make the linker a bit faster. Also, restarting the
> linker is a hack, so we want to avoid it if possible.

Can you simply restart the linker on first call to get_symbols_v2 instead?

Alexander
  

On Sun, May 15, 2022 at 6:09 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Sun, 15 May 2022, Rui Ueyama wrote:
>
> > > Makes sense, but I still don't understand why mold wants to discover in
> > > advance whether the plugin is going to use get_symbols_v3. How would it
> > > help with what mold does today to handle the _v2 case?
> >
> > Currently, mold restarts itself to reset the internal state of the plugin.
> > If we know in advance that get_symbols_v3 is supported, we can avoid that
> > restart. That should make the linker a bit faster. Also, restarting the
> > linker is a hack, so we want to avoid it if possible.
>
> Can you simply restart the linker on first call to get_symbols_v2 instead?

I could, but it may not be a safe timing to call exec(2). I believe we
are expected to call cleanup_hook after calling all_symbols_read_hook,
and it is not clear what will happen if we abruptly terminate and
restart the current process. For example, doesn't it leave temporary
files on disk?
  

On Sun, 15 May 2022, Rui Ueyama wrote:

> > Can you simply restart the linker on first call to get_symbols_v2 instead?
> 
> I could, but it may not be a safe timing to call exec(2). I believe we
> are expected to call cleanup_hook after calling all_symbols_read_hook,
> and it is not clear what will happen if we abruptly terminate and
> restart the current process. For example, doesn't it leave temporary
> files on disk?

Regarding files, as far as I can tell, GCC plugin will leave a 'resolution file'
on disk, but after re-exec it would recreate it anyway.

Alexander
  

On Sun, May 15, 2022 at 7:37 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Sun, 15 May 2022, Rui Ueyama wrote:
>
> > > Can you simply restart the linker on first call to get_symbols_v2 instead?
> >
> > I could, but it may not be a safe timing to call exec(2). I believe we
> > are expected to call cleanup_hook after calling all_symbols_read_hook,
> > and it is not clear what will happen if we abruptly terminate and
> > restart the current process. For example, doesn't it leave temporary
> > files on disk?
>
> Regarding files, as far as I can tell, GCC plugin will leave a 'resolution file'
> on disk, but after re-exec it would recreate it anyway.

Does it recreate a temporary file with the same file name so that
there's no temporary file left on the disk after the linker finishes
doing LTO?
  

On Sun, 15 May 2022, Rui Ueyama wrote:

> > Regarding files, as far as I can tell, GCC plugin will leave a 'resolution file'
> > on disk, but after re-exec it would recreate it anyway.
> 
> Does it recreate a temporary file with the same file name so that
> there's no temporary file left on the disk after the linker finishes
> doing LTO?

Resolution file name is taken from the command line option '-fresolution=',
so it's a stable name (supplied by the compiler driver).

Alexander
  

On Sun, May 15, 2022 at 8:07 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Sun, 15 May 2022, Rui Ueyama wrote:
>
> > > Regarding files, as far as I can tell, GCC plugin will leave a 'resolution file'
> > > on disk, but after re-exec it would recreate it anyway.
> >
> > Does it recreate a temporary file with the same file name so that
> > there's no temporary file left on the disk after the linker finishes
> > doing LTO?
>
> Resolution file name is taken from the command line option '-fresolution=',
> so it's a stable name (supplied by the compiler driver).

If it is a guaranteed behavior that GCC of all versions that support
only get_symbols_v2 don't leave a temporary file behind if it is
suddenly disrupted during get_symbols_v2 execution, then yes, mold can
restart itself when get_symbols_v2 is called for the first time.

Is this what you want? I'm fine with that approach if it is guaranteed
to work by GCC developers.
  

On Mon, 16 May 2022, Rui Ueyama wrote:

> If it is a guaranteed behavior that GCC of all versions that support
> only get_symbols_v2 don't leave a temporary file behind if it is
> suddenly disrupted during get_symbols_v2 execution, then yes, mold can
> restart itself when get_symbols_v2 is called for the first time.
> 
> Is this what you want? I'm fine with that approach if it is guaranteed
> to work by GCC developers.

I cannot answer that, hopefully someone in Cc: will. This sub-thread started
with Richard proposing an alternative solution for API level discovery [1]
(invoking onload twice, first with only the _v3 entrypoint in the "transfer
vector"), and then suggesting an onload_v2 variant that would allow to
discover which entrypoints the plugin is going to use [2].

[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594058.html
[2] https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594059.html

... at which point I butted in, because the whole _v3 thing was shrouded in
mystery. Hopefully, now it makes more sense.


From my side I want to add that thread-safety remains a major unsolved point.
Compiler driver _always_ adds the plugin to linker command line, so I expect
that if you add a mutex around your claim_file hook invocation, you'll find
that it serializes the linker too much. Have you given some thought to that?
Will you be needing a plugin API upgrade to discover thread-safe entrypoints,
or do you have another solution in mind?

Alexander
  

On Mon, May 16, 2022 at 2:38 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Mon, 16 May 2022, Rui Ueyama wrote:
>
> > If it is a guaranteed behavior that GCC of all versions that support
> > only get_symbols_v2 don't leave a temporary file behind if it is
> > suddenly disrupted during get_symbols_v2 execution, then yes, mold can
> > restart itself when get_symbols_v2 is called for the first time.
> >
> > Is this what you want? I'm fine with that approach if it is guaranteed
> > to work by GCC developers.
>
> I cannot answer that, hopefully someone in Cc: will. This sub-thread started
> with Richard proposing an alternative solution for API level discovery [1]
> (invoking onload twice, first with only the _v3 entrypoint in the "transfer
> vector"), and then suggesting an onload_v2 variant that would allow to
> discover which entrypoints the plugin is going to use [2].
>
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594058.html
> [2] https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594059.html
>
> ... at which point I butted in, because the whole _v3 thing was shrouded in
> mystery. Hopefully, now it makes more sense.
>
>
> From my side I want to add that thread-safety remains a major unsolved point.
> Compiler driver _always_ adds the plugin to linker command line, so I expect
> that if you add a mutex around your claim_file hook invocation, you'll find
> that it serializes the linker too much. Have you given some thought to that?
> Will you be needing a plugin API upgrade to discover thread-safe entrypoints,
> or do you have another solution in mind?

From my linker's point of view, the easiest way to handle the
situation is to implement a logic like this:

if (gcc_version >= 12.2)
  assume that claim_file_hook is thread safe
else
  assume that claim_file_hook is not thread safe

And that is also _very_ useful to identify the existence of
get_symbols_v3, because we can do the same thing with s/12.2/12.1/.
  

On Mon, May 16, 2022 at 10:37 AM Rui Ueyama via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Mon, May 16, 2022 at 2:38 PM Alexander Monakov <amonakov@ispras.ru> wrote:
> >
> > On Mon, 16 May 2022, Rui Ueyama wrote:
> >
> > > If it is a guaranteed behavior that GCC of all versions that support
> > > only get_symbols_v2 don't leave a temporary file behind if it is
> > > suddenly disrupted during get_symbols_v2 execution, then yes, mold can
> > > restart itself when get_symbols_v2 is called for the first time.
> > >
> > > Is this what you want? I'm fine with that approach if it is guaranteed
> > > to work by GCC developers.
> >
> > I cannot answer that, hopefully someone in Cc: will. This sub-thread started
> > with Richard proposing an alternative solution for API level discovery [1]
> > (invoking onload twice, first with only the _v3 entrypoint in the "transfer
> > vector"), and then suggesting an onload_v2 variant that would allow to
> > discover which entrypoints the plugin is going to use [2].
> >
> > [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594058.html
> > [2] https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594059.html
> >
> > ... at which point I butted in, because the whole _v3 thing was shrouded in
> > mystery. Hopefully, now it makes more sense.
> >
> >
> > From my side I want to add that thread-safety remains a major unsolved point.
> > Compiler driver _always_ adds the plugin to linker command line, so I expect
> > that if you add a mutex around your claim_file hook invocation, you'll find
> > that it serializes the linker too much. Have you given some thought to that?
> > Will you be needing a plugin API upgrade to discover thread-safe entrypoints,
> > or do you have another solution in mind?
>
> From my linker's point of view, the easiest way to handle the
> situation is to implement a logic like this:
>
> if (gcc_version >= 12.2)
>   assume that claim_file_hook is thread safe
> else
>   assume that claim_file_hook is not thread safe
>
> And that is also _very_ useful to identify the existence of
> get_symbols_v3, because we can do the same thing with s/12.2/12.1/.

Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
is useful if bugs are discovered in old versions that you by definition cannot
fix but can apply workarounds to.  Note the actual compiler used might still
differ.  Note that still isn't clean API documentation / extension of the plugin
API itself.  As of thread safety we can either add a claim_file_v2_hook
or try to do broader-level versioning of the API with a new api_version
hook which could also specify that with say version 2 the plugin will
not use get_symbols_v2 but only newer, etc.  The linker would announce
the API version it likes to use and the plugin would return the
(closest) version
it can handle.  A v2 could then also specify that claim_file needs to be
thread safe, or that cleanup should allow a followup onload again with
a state identical to after dlopen, etc.

Is there an API document besides the header itself somewhere?

Richard.
  

On Mon, 16 May 2022, Richard Biener wrote:

> Is there an API document besides the header itself somewhere?

It's on the wiki: https://gcc.gnu.org/wiki/whopr/driver
(sadly the v3 entrypoint was added there without documentation)

Alexander
  

> 
> Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
> is useful if bugs are discovered in old versions that you by definition cannot
> fix but can apply workarounds to.  Note the actual compiler used might still
> differ.  Note that still isn't clean API documentation / extension of the plugin
> API itself.  As of thread safety we can either add a claim_file_v2_hook
> or try to do broader-level versioning of the API with a new api_version
> hook which could also specify that with say version 2 the plugin will
> not use get_symbols_v2 but only newer, etc.  The linker would announce
> the API version it likes to use and the plugin would return the
> (closest) version
> it can handle.  A v2 could then also specify that claim_file needs to be

Yep, I think having the API version handshake is quite reasonable way to
go as the _v2,_v3 symbols seems already getting bit out of hand and we
essentially have 3 revisions of API to think of
 (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding
 GET_SYMBOLS_V3 and now we plan third adding thread safety and solving
 the file handler problems)

> thread safe, or that cleanup should allow a followup onload again with
> a state identical to after dlopen, etc.
> 
> Is there an API document besides the header itself somewhere?

There is https://gcc.gnu.org/wiki/whopr/driver
I wonder if this can't be moved into more official looking place since
it looks like it is internal to GCC WHOPR implementation this way.

Honza
> 
> Richard.
  

On 5/16/22 11:25, Jan Hubicka via Gcc-patches wrote:
>>
>> Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
>> is useful if bugs are discovered in old versions that you by definition cannot
>> fix but can apply workarounds to.  Note the actual compiler used might still
>> differ.  Note that still isn't clean API documentation / extension of the plugin
>> API itself.  As of thread safety we can either add a claim_file_v2_hook
>> or try to do broader-level versioning of the API with a new api_version
>> hook which could also specify that with say version 2 the plugin will
>> not use get_symbols_v2 but only newer, etc.  The linker would announce
>> the API version it likes to use and the plugin would return the
>> (closest) version
>> it can handle.  A v2 could then also specify that claim_file needs to be
> 
> Yep, I think having the API version handshake is quite reasonable way to
> go as the _v2,_v3 symbols seems already getting bit out of hand and we
> essentially have 3 revisions of API to think of
>  (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding
>  GET_SYMBOLS_V3 and now we plan third adding thread safety and solving
>  the file handler problems)

How should be design such a version handshake?

> 
>> thread safe, or that cleanup should allow a followup onload again with
>> a state identical to after dlopen, etc.
>>
>> Is there an API document besides the header itself somewhere?
> 
> There is https://gcc.gnu.org/wiki/whopr/driver
> I wonder if this can't be moved into more official looking place since
> it looks like it is internal to GCC WHOPR implementation this way.

We can likely add it here:
https://gcc.gnu.org/onlinedocs/gccint/LTO.html#LTO

What do you think? I can port it.

Martin

> 
> Honza
>>
>> Richard.
  

> On 5/16/22 11:25, Jan Hubicka via Gcc-patches wrote:
> >>
> >> Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
> >> is useful if bugs are discovered in old versions that you by definition cannot
> >> fix but can apply workarounds to.  Note the actual compiler used might still
> >> differ.  Note that still isn't clean API documentation / extension of the plugin
> >> API itself.  As of thread safety we can either add a claim_file_v2_hook
> >> or try to do broader-level versioning of the API with a new api_version
> >> hook which could also specify that with say version 2 the plugin will
> >> not use get_symbols_v2 but only newer, etc.  The linker would announce
> >> the API version it likes to use and the plugin would return the
> >> (closest) version
> >> it can handle.  A v2 could then also specify that claim_file needs to be
> > 
> > Yep, I think having the API version handshake is quite reasonable way to
> > go as the _v2,_v3 symbols seems already getting bit out of hand and we
> > essentially have 3 revisions of API to think of
> >  (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding
> >  GET_SYMBOLS_V3 and now we plan third adding thread safety and solving
> >  the file handler problems)
> 
> How should be design such a version handshake?

As richi outlined.  We need to assign version number of the API (with 1,
2, 3 defind to match existing revisions) and define version 4 which will
have have way for plugin to announce maximal api version supported and
linker telling plugin what API version it wants to work with.  That
shold be min(linker_api, plugin_api)

Since revisions happens relatively rarely, I think we should be able to
stay with single number determining it.
> 
> > 
> >> thread safe, or that cleanup should allow a followup onload again with
> >> a state identical to after dlopen, etc.
> >>
> >> Is there an API document besides the header itself somewhere?
> > 
> > There is https://gcc.gnu.org/wiki/whopr/driver
> > I wonder if this can't be moved into more official looking place since
> > it looks like it is internal to GCC WHOPR implementation this way.
> 
> We can likely add it here:
> https://gcc.gnu.org/onlinedocs/gccint/LTO.html#LTO
> 
> What do you think? I can port it.

I am bit worried that it is place LLVM developers will not look at
since name "GCC internals" suggests it is internal to GCC. It is
not.  However perhaps we can link it from binutils docs, plugin header
and gcc plugin source to make it hopefully obvious enough.  Maybe
binutils docs would be an alternative. Yet we want other linkers support
it: I am really happy mold now supports plugin API, but if lld and MacOS
linker had it it would make our life easier.

Honza
> 
> Martin
> 
> > 
> > Honza
> >>
> >> Richard.
>
  

Version handshaking is doable, but it feels like we are over-designing
an API, given that the real providers of this plugin API are only GCC
and LLVM and the users of the API are BFD ld, gold and mold. It is
unlikely that we'll have dozens of more compilers or linkers in the
near future. So, I personally prefer the following style

if (!strcmp(plugin_compiler_name, "gcc") && plugin_major >= 12)

than versioning various API-provided functions. It'll just work and be
easy to understand.

Besides that, even if we version GCC-provided plugin API functions, we
still need a logic similar to the above to distinguish GCC from LLVM,
as they behave slightly differently in various corner cases. We can't
get rid of the heuristic version detection logic from the linker
anyways.

So, from the linker's point of view, exporting a compiler name and
version numbers is enough.


On Mon, May 16, 2022 at 5:38 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/16/22 11:25, Jan Hubicka via Gcc-patches wrote:
> >>
> >> Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
> >> is useful if bugs are discovered in old versions that you by definition cannot
> >> fix but can apply workarounds to.  Note the actual compiler used might still
> >> differ.  Note that still isn't clean API documentation / extension of the plugin
> >> API itself.  As of thread safety we can either add a claim_file_v2_hook
> >> or try to do broader-level versioning of the API with a new api_version
> >> hook which could also specify that with say version 2 the plugin will
> >> not use get_symbols_v2 but only newer, etc.  The linker would announce
> >> the API version it likes to use and the plugin would return the
> >> (closest) version
> >> it can handle.  A v2 could then also specify that claim_file needs to be
> >
> > Yep, I think having the API version handshake is quite reasonable way to
> > go as the _v2,_v3 symbols seems already getting bit out of hand and we
> > essentially have 3 revisions of API to think of
> >  (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding
> >  GET_SYMBOLS_V3 and now we plan third adding thread safety and solving
> >  the file handler problems)
>
> How should be design such a version handshake?
>
> >
> >> thread safe, or that cleanup should allow a followup onload again with
> >> a state identical to after dlopen, etc.
> >>
> >> Is there an API document besides the header itself somewhere?
> >
> > There is https://gcc.gnu.org/wiki/whopr/driver
> > I wonder if this can't be moved into more official looking place since
> > it looks like it is internal to GCC WHOPR implementation this way.
>
> We can likely add it here:
> https://gcc.gnu.org/onlinedocs/gccint/LTO.html#LTO
>
> What do you think? I can port it.
>
> Martin
>
> >
> > Honza
> >>
> >> Richard.
>
  

On Mon, May 16, 2022 at 11:50 AM Jan Hubicka <hubicka@kam.mff.cuni.cz> wrote:
>
> > On 5/16/22 11:25, Jan Hubicka via Gcc-patches wrote:
> > >>
> > >> Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
> > >> is useful if bugs are discovered in old versions that you by definition cannot
> > >> fix but can apply workarounds to.  Note the actual compiler used might still
> > >> differ.  Note that still isn't clean API documentation / extension of the plugin
> > >> API itself.  As of thread safety we can either add a claim_file_v2_hook
> > >> or try to do broader-level versioning of the API with a new api_version
> > >> hook which could also specify that with say version 2 the plugin will
> > >> not use get_symbols_v2 but only newer, etc.  The linker would announce
> > >> the API version it likes to use and the plugin would return the
> > >> (closest) version
> > >> it can handle.  A v2 could then also specify that claim_file needs to be
> > >
> > > Yep, I think having the API version handshake is quite reasonable way to
> > > go as the _v2,_v3 symbols seems already getting bit out of hand and we
> > > essentially have 3 revisions of API to think of
> > >  (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding
> > >  GET_SYMBOLS_V3 and now we plan third adding thread safety and solving
> > >  the file handler problems)
> >
> > How should be design such a version handshake?
>
> As richi outlined.  We need to assign version number of the API (with 1,
> 2, 3 defind to match existing revisions) and define version 4 which will
> have have way for plugin to announce maximal api version supported and
> linker telling plugin what API version it wants to work with.  That
> shold be min(linker_api, plugin_api)
>
> Since revisions happens relatively rarely, I think we should be able to
> stay with single number determining it.
> >
> > >
> > >> thread safe, or that cleanup should allow a followup onload again with
> > >> a state identical to after dlopen, etc.
> > >>
> > >> Is there an API document besides the header itself somewhere?
> > >
> > > There is https://gcc.gnu.org/wiki/whopr/driver
> > > I wonder if this can't be moved into more official looking place since
> > > it looks like it is internal to GCC WHOPR implementation this way.
> >
> > We can likely add it here:
> > https://gcc.gnu.org/onlinedocs/gccint/LTO.html#LTO
> >
> > What do you think? I can port it.
>
> I am bit worried that it is place LLVM developers will not look at
> since name "GCC internals" suggests it is internal to GCC. It is
> not.  However perhaps we can link it from binutils docs, plugin header
> and gcc plugin source to make it hopefully obvious enough.  Maybe
> binutils docs would be an alternative. Yet we want other linkers support
> it: I am really happy mold now supports plugin API, but if lld and MacOS
> linker had it it would make our life easier.

Yep.  Or if the docs are not too extensive pasting them into
plugin-api.h itself might be not the worst idea either ... basically
provide markup that doxygen or similar tools can re-create sth
like the wiki page?

Richard.

> Honza
> >
> > Martin
> >
> > >
> > > Honza
> > >>
> > >> Richard.
> >
  

On Mon, May 16, 2022 at 11:58 AM Rui Ueyama <rui314@gmail.com> wrote:
>
> Version handshaking is doable, but it feels like we are over-designing
> an API, given that the real providers of this plugin API are only GCC
> and LLVM and the users of the API are BFD ld, gold and mold. It is
> unlikely that we'll have dozens of more compilers or linkers in the
> near future. So, I personally prefer the following style
>
> if (!strcmp(plugin_compiler_name, "gcc") && plugin_major >= 12)
>
> than versioning various API-provided functions. It'll just work and be
> easy to understand.
>
> Besides that, even if we version GCC-provided plugin API functions, we
> still need a logic similar to the above to distinguish GCC from LLVM,
> as they behave slightly differently in various corner cases. We can't
> get rid of the heuristic version detection logic from the linker
> anyways.
>
> So, from the linker's point of view, exporting a compiler name and
> version numbers is enough.

I agree that this might be convenient enough but the different behaviors
are because of inadequate documentation of the API - that's something
we should fix.  And for this I think a plugin API version might help
as we can this way also handle your case of the need of querying
whether v2 will be used or not.

I wouldn't go to enumerate past API versions - the version handshake
hook requirement alone makes that not so useful.  Instead I'd require
everybody implementing the handshare hook implementing also all
other hooks defined at that point in time and set the version to 1.

I'd eventually keep version 2 to indicate thread safety (of a part of the API).

That said, I'm not opposed to add a "GCC 12.1" provider, maybe the
version handshake should be

int api_version (int linker, const char **identifier);

where the linker specifies the desired API version and passes an
identifier identifying itself ("mold 1.0") and it will get back the API
version the plugin intends to use plus an identifier of the plugin
("GCC 12.1").

Richard.

>
>
> On Mon, May 16, 2022 at 5:38 PM Martin Liška <mliska@suse.cz> wrote:
> >
> > On 5/16/22 11:25, Jan Hubicka via Gcc-patches wrote:
> > >>
> > >> Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
> > >> is useful if bugs are discovered in old versions that you by definition cannot
> > >> fix but can apply workarounds to.  Note the actual compiler used might still
> > >> differ.  Note that still isn't clean API documentation / extension of the plugin
> > >> API itself.  As of thread safety we can either add a claim_file_v2_hook
> > >> or try to do broader-level versioning of the API with a new api_version
> > >> hook which could also specify that with say version 2 the plugin will
> > >> not use get_symbols_v2 but only newer, etc.  The linker would announce
> > >> the API version it likes to use and the plugin would return the
> > >> (closest) version
> > >> it can handle.  A v2 could then also specify that claim_file needs to be
> > >
> > > Yep, I think having the API version handshake is quite reasonable way to
> > > go as the _v2,_v3 symbols seems already getting bit out of hand and we
> > > essentially have 3 revisions of API to think of
> > >  (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding
> > >  GET_SYMBOLS_V3 and now we plan third adding thread safety and solving
> > >  the file handler problems)
> >
> > How should be design such a version handshake?
> >
> > >
> > >> thread safe, or that cleanup should allow a followup onload again with
> > >> a state identical to after dlopen, etc.
> > >>
> > >> Is there an API document besides the header itself somewhere?
> > >
> > > There is https://gcc.gnu.org/wiki/whopr/driver
> > > I wonder if this can't be moved into more official looking place since
> > > it looks like it is internal to GCC WHOPR implementation this way.
> >
> > We can likely add it here:
> > https://gcc.gnu.org/onlinedocs/gccint/LTO.html#LTO
> >
> > What do you think? I can port it.
> >
> > Martin
> >
> > >
> > > Honza
> > >>
> > >> Richard.
> >
  

On Mon, May 16, 2022 at 6:28 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, May 16, 2022 at 11:58 AM Rui Ueyama <rui314@gmail.com> wrote:
> >
> > Version handshaking is doable, but it feels like we are over-designing
> > an API, given that the real providers of this plugin API are only GCC
> > and LLVM and the users of the API are BFD ld, gold and mold. It is
> > unlikely that we'll have dozens of more compilers or linkers in the
> > near future. So, I personally prefer the following style
> >
> > if (!strcmp(plugin_compiler_name, "gcc") && plugin_major >= 12)
> >
> > than versioning various API-provided functions. It'll just work and be
> > easy to understand.
> >
> > Besides that, even if we version GCC-provided plugin API functions, we
> > still need a logic similar to the above to distinguish GCC from LLVM,
> > as they behave slightly differently in various corner cases. We can't
> > get rid of the heuristic version detection logic from the linker
> > anyways.
> >
> > So, from the linker's point of view, exporting a compiler name and
> > version numbers is enough.
>
> I agree that this might be convenient enough but the different behaviors
> are because of inadequate documentation of the API - that's something
> we should fix.  And for this I think a plugin API version might help
> as we can this way also handle your case of the need of querying
> whether v2 will be used or not.
>
> I wouldn't go to enumerate past API versions - the version handshake
> hook requirement alone makes that not so useful.  Instead I'd require
> everybody implementing the handshare hook implementing also all
> other hooks defined at that point in time and set the version to 1.
>
> I'd eventually keep version 2 to indicate thread safety (of a part of the API).
>
> That said, I'm not opposed to add a "GCC 12.1" provider, maybe the
> version handshake should be
>
> int api_version (int linker, const char **identifier);
>
> where the linker specifies the desired API version and passes an
> identifier identifying itself ("mold 1.0") and it will get back the API
> version the plugin intends to use plus an identifier of the plugin
> ("GCC 12.1").

void api_version(char *linker_identifier, const char
**compiler_identifier, int *compiler_version);

might be a bit better, where compiler_identifier is something like
"gcc" or "clang" and
comipler_version is 12001000 for 12.1.0.


In the longer term, it feels to me that gcc should migrate to LLVM's
libLTO-compatible API
(https://llvm.org/docs/LinkTimeOptimization.html). It has resolved
various problems of GCC's
plugin API. A few notable examples are:

- libLTO API separates a function to read a symbol table from an IR
object from adding that object to the LTO final result

- libLTO API functions don't depend on a global state of the plugin
API, while GCC LTO plugin saves its internal state to a global
variable (so we can't have two linker instances in a single process
with GCC LTO, for example)

- libLTO API doesn't use callbacks. It looks much more straightforward
than GCC's plugin API.

> Richard.
>
> >
> >
> > On Mon, May 16, 2022 at 5:38 PM Martin Liška <mliska@suse.cz> wrote:
> > >
> > > On 5/16/22 11:25, Jan Hubicka via Gcc-patches wrote:
> > > >>
> > > >> Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
> > > >> is useful if bugs are discovered in old versions that you by definition cannot
> > > >> fix but can apply workarounds to.  Note the actual compiler used might still
> > > >> differ.  Note that still isn't clean API documentation / extension of the plugin
> > > >> API itself.  As of thread safety we can either add a claim_file_v2_hook
> > > >> or try to do broader-level versioning of the API with a new api_version
> > > >> hook which could also specify that with say version 2 the plugin will
> > > >> not use get_symbols_v2 but only newer, etc.  The linker would announce
> > > >> the API version it likes to use and the plugin would return the
> > > >> (closest) version
> > > >> it can handle.  A v2 could then also specify that claim_file needs to be
> > > >
> > > > Yep, I think having the API version handshake is quite reasonable way to
> > > > go as the _v2,_v3 symbols seems already getting bit out of hand and we
> > > > essentially have 3 revisions of API to think of
> > > >  (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding
> > > >  GET_SYMBOLS_V3 and now we plan third adding thread safety and solving
> > > >  the file handler problems)
> > >
> > > How should be design such a version handshake?
> > >
> > > >
> > > >> thread safe, or that cleanup should allow a followup onload again with
> > > >> a state identical to after dlopen, etc.
> > > >>
> > > >> Is there an API document besides the header itself somewhere?
> > > >
> > > > There is https://gcc.gnu.org/wiki/whopr/driver
> > > > I wonder if this can't be moved into more official looking place since
> > > > it looks like it is internal to GCC WHOPR implementation this way.
> > >
> > > We can likely add it here:
> > > https://gcc.gnu.org/onlinedocs/gccint/LTO.html#LTO
> > >
> > > What do you think? I can port it.
> > >
> > > Martin
> > >
> > > >
> > > > Honza
> > > >>
> > > >> Richard.
> > >
  

On 5/16/22 12:28, Richard Biener wrote:
> On Mon, May 16, 2022 at 11:58 AM Rui Ueyama <rui314@gmail.com> wrote:
>>
>> Version handshaking is doable, but it feels like we are over-designing
>> an API, given that the real providers of this plugin API are only GCC
>> and LLVM and the users of the API are BFD ld, gold and mold. It is
>> unlikely that we'll have dozens of more compilers or linkers in the
>> near future. So, I personally prefer the following style
>>
>> if (!strcmp(plugin_compiler_name, "gcc") && plugin_major >= 12)
>>
>> than versioning various API-provided functions. It'll just work and be
>> easy to understand.
>>
>> Besides that, even if we version GCC-provided plugin API functions, we
>> still need a logic similar to the above to distinguish GCC from LLVM,
>> as they behave slightly differently in various corner cases. We can't
>> get rid of the heuristic version detection logic from the linker
>> anyways.
>>
>> So, from the linker's point of view, exporting a compiler name and
>> version numbers is enough.
> 
> I agree that this might be convenient enough but the different behaviors
> are because of inadequate documentation of the API - that's something
> we should fix.  And for this I think a plugin API version might help
> as we can this way also handle your case of the need of querying
> whether v2 will be used or not.
> 
> I wouldn't go to enumerate past API versions - the version handshake
> hook requirement alone makes that not so useful.  Instead I'd require
> everybody implementing the handshare hook implementing also all
> other hooks defined at that point in time and set the version to 1.
> 
> I'd eventually keep version 2 to indicate thread safety (of a part of the API).

That seems reasonable to me.

> 
> That said, I'm not opposed to add a "GCC 12.1" provider, maybe the
> version handshake should be
> 
> int api_version (int linker, const char **identifier);
> 
> where the linker specifies the desired API version and passes an
> identifier identifying itself ("mold 1.0") and it will get back the API
> version the plugin intends to use plus an identifier of the plugin
> ("GCC 12.1").

I've implemented first version of the patch, please take a look.

Note there's a bit name collision of api_version with:

/* The version of the API specification.  */

enum ld_plugin_api_version
{
  LD_PLUGIN_API_VERSION = 1
};

@Rui: Am I correct that you're interested in thread-safe claim_file? Is there any
other function being called paralely?

Thoughts?

> 
> Richard.
> 
>>
>>
>> On Mon, May 16, 2022 at 5:38 PM Martin Liška <mliska@suse.cz> wrote:
>>>
>>> On 5/16/22 11:25, Jan Hubicka via Gcc-patches wrote:
>>>>>
>>>>> Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
>>>>> is useful if bugs are discovered in old versions that you by definition cannot
>>>>> fix but can apply workarounds to.  Note the actual compiler used might still
>>>>> differ.  Note that still isn't clean API documentation / extension of the plugin
>>>>> API itself.  As of thread safety we can either add a claim_file_v2_hook
>>>>> or try to do broader-level versioning of the API with a new api_version
>>>>> hook which could also specify that with say version 2 the plugin will
>>>>> not use get_symbols_v2 but only newer, etc.  The linker would announce
>>>>> the API version it likes to use and the plugin would return the
>>>>> (closest) version
>>>>> it can handle.  A v2 could then also specify that claim_file needs to be
>>>>
>>>> Yep, I think having the API version handshake is quite reasonable way to
>>>> go as the _v2,_v3 symbols seems already getting bit out of hand and we
>>>> essentially have 3 revisions of API to think of
>>>>  (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding
>>>>  GET_SYMBOLS_V3 and now we plan third adding thread safety and solving
>>>>  the file handler problems)
>>>
>>> How should be design such a version handshake?
>>>
>>>>
>>>>> thread safe, or that cleanup should allow a followup onload again with
>>>>> a state identical to after dlopen, etc.
>>>>>
>>>>> Is there an API document besides the header itself somewhere?
>>>>
>>>> There is https://gcc.gnu.org/wiki/whopr/driver
>>>> I wonder if this can't be moved into more official looking place since
>>>> it looks like it is internal to GCC WHOPR implementation this way.
>>>
>>> We can likely add it here:
>>> https://gcc.gnu.org/onlinedocs/gccint/LTO.html#LTO
>>>
>>> What do you think? I can port it.
>>>
>>> Martin
>>>
>>>>
>>>> Honza
>>>>>
>>>>> Richard.
>>>
  

On Mon, May 16, 2022 at 8:04 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/16/22 12:28, Richard Biener wrote:
> > On Mon, May 16, 2022 at 11:58 AM Rui Ueyama <rui314@gmail.com> wrote:
> >>
> >> Version handshaking is doable, but it feels like we are over-designing
> >> an API, given that the real providers of this plugin API are only GCC
> >> and LLVM and the users of the API are BFD ld, gold and mold. It is
> >> unlikely that we'll have dozens of more compilers or linkers in the
> >> near future. So, I personally prefer the following style
> >>
> >> if (!strcmp(plugin_compiler_name, "gcc") && plugin_major >= 12)
> >>
> >> than versioning various API-provided functions. It'll just work and be
> >> easy to understand.
> >>
> >> Besides that, even if we version GCC-provided plugin API functions, we
> >> still need a logic similar to the above to distinguish GCC from LLVM,
> >> as they behave slightly differently in various corner cases. We can't
> >> get rid of the heuristic version detection logic from the linker
> >> anyways.
> >>
> >> So, from the linker's point of view, exporting a compiler name and
> >> version numbers is enough.
> >
> > I agree that this might be convenient enough but the different behaviors
> > are because of inadequate documentation of the API - that's something
> > we should fix.  And for this I think a plugin API version might help
> > as we can this way also handle your case of the need of querying
> > whether v2 will be used or not.
> >
> > I wouldn't go to enumerate past API versions - the version handshake
> > hook requirement alone makes that not so useful.  Instead I'd require
> > everybody implementing the handshare hook implementing also all
> > other hooks defined at that point in time and set the version to 1.
> >
> > I'd eventually keep version 2 to indicate thread safety (of a part of the API).
>
> That seems reasonable to me.
>
> >
> > That said, I'm not opposed to add a "GCC 12.1" provider, maybe the
> > version handshake should be
> >
> > int api_version (int linker, const char **identifier);
> >
> > where the linker specifies the desired API version and passes an
> > identifier identifying itself ("mold 1.0") and it will get back the API
> > version the plugin intends to use plus an identifier of the plugin
> > ("GCC 12.1").
>
> I've implemented first version of the patch, please take a look.
>
> Note there's a bit name collision of api_version with:
>
> /* The version of the API specification.  */
>
> enum ld_plugin_api_version
> {
>   LD_PLUGIN_API_VERSION = 1
> };
>
> @Rui: Am I correct that you're interested in thread-safe claim_file? Is there any
> other function being called paralely?

Yes, I want a thread-safe claim_file. And that function seems to be
the only function in mold that is called in parallel.

> Thoughts?
>
> >
> > Richard.
> >
> >>
> >>
> >> On Mon, May 16, 2022 at 5:38 PM Martin Liška <mliska@suse.cz> wrote:
> >>>
> >>> On 5/16/22 11:25, Jan Hubicka via Gcc-patches wrote:
> >>>>>
> >>>>> Sure having a 'plugin was compiled from sources of the GCC N.M compiler'
> >>>>> is useful if bugs are discovered in old versions that you by definition cannot
> >>>>> fix but can apply workarounds to.  Note the actual compiler used might still
> >>>>> differ.  Note that still isn't clean API documentation / extension of the plugin
> >>>>> API itself.  As of thread safety we can either add a claim_file_v2_hook
> >>>>> or try to do broader-level versioning of the API with a new api_version
> >>>>> hook which could also specify that with say version 2 the plugin will
> >>>>> not use get_symbols_v2 but only newer, etc.  The linker would announce
> >>>>> the API version it likes to use and the plugin would return the
> >>>>> (closest) version
> >>>>> it can handle.  A v2 could then also specify that claim_file needs to be
> >>>>
> >>>> Yep, I think having the API version handshake is quite reasonable way to
> >>>> go as the _v2,_v3 symbols seems already getting bit out of hand and we
> >>>> essentially have 3 revisions of API to think of
> >>>>  (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding
> >>>>  GET_SYMBOLS_V3 and now we plan third adding thread safety and solving
> >>>>  the file handler problems)
> >>>
> >>> How should be design such a version handshake?
> >>>
> >>>>
> >>>>> thread safe, or that cleanup should allow a followup onload again with
> >>>>> a state identical to after dlopen, etc.
> >>>>>
> >>>>> Is there an API document besides the header itself somewhere?
> >>>>
> >>>> There is https://gcc.gnu.org/wiki/whopr/driver
> >>>> I wonder if this can't be moved into more official looking place since
> >>>> it looks like it is internal to GCC WHOPR implementation this way.
> >>>
> >>> We can likely add it here:
> >>> https://gcc.gnu.org/onlinedocs/gccint/LTO.html#LTO
> >>>
> >>> What do you think? I can port it.
> >>>
> >>> Martin
> >>>
> >>>>
> >>>> Honza
> >>>>>
> >>>>> Richard.
> >>>
  

On Mon, 16 May 2022, Rui Ueyama wrote:

> > @Rui: Am I correct that you're interested in thread-safe claim_file? Is there any
> > other function being called paralely?
> 
> Yes, I want a thread-safe claim_file. And that function seems to be
> the only function in mold that is called in parallel.

But note that you'll have to provide a guarantee that all entrypoints that
the plugin may invoke when multithreaded (i.e. add_symbols, which is called
from claim_file) are also thread-safe.

Alexander
  

On Mon, 16 May 2022, Martin Liška wrote:

> I've implemented first version of the patch, please take a look.

I'll comment on the patch, feel free to inform me when I should back off
with forcing my opinion in this thread :)

> --- a/include/plugin-api.h
> +++ b/include/plugin-api.h
> @@ -483,6 +483,18 @@ enum ld_plugin_level
>    LDPL_FATAL
>  };
>  
> +/* The linker's interface for API version negotiation.  */
> +
> +typedef
> +int (*ld_plugin_get_api_version) (char *linker_identifier, int linker_version,
> +				  int preferred_linker_api,
> +				  const char **compiler_identifier,
> +				  int *compiler_version);
> +
> +typedef
> +enum ld_plugin_status
> +(*ld_plugin_register_get_api_version) (ld_plugin_get_api_version handler);
> +
>  /* Values for the tv_tag field of the transfer vector.  */
>  
>  enum ld_plugin_tag
> @@ -521,6 +533,7 @@ enum ld_plugin_tag
>    LDPT_REGISTER_NEW_INPUT_HOOK,
>    LDPT_GET_WRAP_SYMBOLS,
>    LDPT_ADD_SYMBOLS_V2,
> +  LDPT_REGISTER_GET_API_VERSION,
>  };
>  
>  /* The plugin transfer vector.  */
> @@ -556,6 +569,7 @@ struct ld_plugin_tv
>      ld_plugin_get_input_section_size tv_get_input_section_size;
>      ld_plugin_register_new_input tv_register_new_input;
>      ld_plugin_get_wrap_symbols tv_get_wrap_symbols;
> +    ld_plugin_register_get_api_version tv_register_get_api_version;
>    } tv_u;
>  };

Here I disagree with the overall design. Rui already pointed out how plugin
API seems to consist of callbacks-that-register-callbacks, and I'm with him
on that, let's not make that worse. On a more serious note, this pattern:

* the linker provides register_get_api_version entrypoint
* the plugin registers its get_api_version implementation
* the linker uses the provided function pointer

is problematic because the plugin doesn't know when the linker is going to
invoke its callback (or maybe the linker won't do that at all).

I'd recommend to reduce the level of indirection, remove the register_
callback, and simply require that if LDPT_GET_API_VERSION is provided,
the plugin MUST invoke it before returning from onload, i.e.:

* the linker invokes onload with LDPT_GET_API_VERSION in 'transfer vector'
* the plugin iterates over the transfer vector and notes if LDPT_GET_API_VERSION
  is seen
  * if not, the plugin knows the linker is predates its introduction
  * if yes, the plugin invokes it before returning from onload
* the linker now knows the plugin version (either one provided via
  LDPT_GET_API_VERSION, or 'old' if the callback wasn't invoked).

> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> index 00b760636dc..49484decd89 100644
> --- a/lto-plugin/lto-plugin.c
> +++ b/lto-plugin/lto-plugin.c
> @@ -69,6 +69,7 @@ along with this program; see the file COPYING3.  If not see
>  #include "../gcc/lto/common.h"
>  #include "simple-object.h"
>  #include "plugin-api.h"
> +#include "ansidecl.h"
>  
>  /* We need to use I64 instead of ll width-specifier on native Windows.
>     The reason for this is that older MS-runtimes don't support the ll.  */
> @@ -166,6 +167,10 @@ static ld_plugin_add_input_file add_input_file;
>  static ld_plugin_add_input_library add_input_library;
>  static ld_plugin_message message;
>  static ld_plugin_add_symbols add_symbols, add_symbols_v2;
> +static ld_plugin_register_get_api_version register_get_api_version;
> +
> +/* By default, use version 1 if there is not negotiation.  */
> +static int used_api_version = 1;
>  
>  static struct plugin_file_info *claimed_files = NULL;
>  static unsigned int num_claimed_files = 0;
> @@ -1407,6 +1412,29 @@ process_option (const char *option)
>    verbose = verbose || debug;
>  }
>  
> +static int
> +get_api_version (char *linker_identifier, int linker_version,
> +		 int preferred_linker_api,

The 'preferred' qualifier seems vague. If you go with my suggestion above,
I'd suggest to pass lowest and highest supported version number, and the linker
can check if that intersects with its range of supported versions, and error out
if the intersection is empty (and otherwise return the highest version they both
support as the 'negotiated' one).

> +		 const char **compiler_identifier,
> +		 int *compiler_version)
> +{
> +  *compiler_identifier = "GCC";
> +  *compiler_version = GCC_VERSION;

Note that I'm chiming in here because I worked on a tool that used LTO plugin
API to discover symbol/section dependencies with high accuracy. So I'd like to
remind that implementors/consumers of this API are not necessarily compilers!

Alexander
  

On Mon, May 16, 2022 at 5:16 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Mon, 16 May 2022, Martin Liška wrote:
>
> > I've implemented first version of the patch, please take a look.
>
> I'll comment on the patch, feel free to inform me when I should back off
> with forcing my opinion in this thread :)

I think your comments are very useful.

> > --- a/include/plugin-api.h
> > +++ b/include/plugin-api.h
> > @@ -483,6 +483,18 @@ enum ld_plugin_level
> >    LDPL_FATAL
> >  };
> >
> > +/* The linker's interface for API version negotiation.  */
> > +
> > +typedef
> > +int (*ld_plugin_get_api_version) (char *linker_identifier, int linker_version,

const char * for the linker_identifier?

> > +                               int preferred_linker_api,
> > +                               const char **compiler_identifier,
> > +                               int *compiler_version);

I think a signed int for the version isn't a good fit so use unsigned?

I miss documentation on who owns linker_identifier and *compiler_identifier.

I also miss documentation on the supported linker_apis and what they mean.
Maybe we want an enum here?

enum linker_api_version
{
   /* The linker/plugin do not implement any of the API levels below, the API
       is determined solely via the transfer vector.  */
   LAPI_UNSPECIFIED = 0,
   /* API level v1.  The linker provides add_symbols_v3, the plugin will use
      that and not any lower versions.  claim_file is thread-safe on the plugin
      side and add_symbols on the linker side.  */
   LAPI_V1 = 1
};

> > +
> > +typedef
> > +enum ld_plugin_status
> > +(*ld_plugin_register_get_api_version) (ld_plugin_get_api_version handler);
> > +
> >  /* Values for the tv_tag field of the transfer vector.  */
> >
> >  enum ld_plugin_tag
> > @@ -521,6 +533,7 @@ enum ld_plugin_tag
> >    LDPT_REGISTER_NEW_INPUT_HOOK,
> >    LDPT_GET_WRAP_SYMBOLS,
> >    LDPT_ADD_SYMBOLS_V2,
> > +  LDPT_REGISTER_GET_API_VERSION,
> >  };
> >
> >  /* The plugin transfer vector.  */
> > @@ -556,6 +569,7 @@ struct ld_plugin_tv
> >      ld_plugin_get_input_section_size tv_get_input_section_size;
> >      ld_plugin_register_new_input tv_register_new_input;
> >      ld_plugin_get_wrap_symbols tv_get_wrap_symbols;
> > +    ld_plugin_register_get_api_version tv_register_get_api_version;
> >    } tv_u;
> >  };
>
> Here I disagree with the overall design. Rui already pointed out how plugin
> API seems to consist of callbacks-that-register-callbacks, and I'm with him
> on that, let's not make that worse. On a more serious note, this pattern:
>
> * the linker provides register_get_api_version entrypoint
> * the plugin registers its get_api_version implementation
> * the linker uses the provided function pointer
>
> is problematic because the plugin doesn't know when the linker is going to
> invoke its callback (or maybe the linker won't do that at all).
>
> I'd recommend to reduce the level of indirection, remove the register_
> callback, and simply require that if LDPT_GET_API_VERSION is provided,
> the plugin MUST invoke it before returning from onload, i.e.:
>
> * the linker invokes onload with LDPT_GET_API_VERSION in 'transfer vector'
> * the plugin iterates over the transfer vector and notes if LDPT_GET_API_VERSION
>   is seen
>   * if not, the plugin knows the linker is predates its introduction
>   * if yes, the plugin invokes it before returning from onload
> * the linker now knows the plugin version (either one provided via
>   LDPT_GET_API_VERSION, or 'old' if the callback wasn't invoked).

That sounds reasonable.  It then basically extends the onload() API without
introducing onload_v2().

> > diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> > index 00b760636dc..49484decd89 100644
> > --- a/lto-plugin/lto-plugin.c
> > +++ b/lto-plugin/lto-plugin.c
> > @@ -69,6 +69,7 @@ along with this program; see the file COPYING3.  If not see
> >  #include "../gcc/lto/common.h"
> >  #include "simple-object.h"
> >  #include "plugin-api.h"
> > +#include "ansidecl.h"
> >
> >  /* We need to use I64 instead of ll width-specifier on native Windows.
> >     The reason for this is that older MS-runtimes don't support the ll.  */
> > @@ -166,6 +167,10 @@ static ld_plugin_add_input_file add_input_file;
> >  static ld_plugin_add_input_library add_input_library;
> >  static ld_plugin_message message;
> >  static ld_plugin_add_symbols add_symbols, add_symbols_v2;
> > +static ld_plugin_register_get_api_version register_get_api_version;
> > +
> > +/* By default, use version 1 if there is not negotiation.  */
> > +static int used_api_version = 1;
> >
> >  static struct plugin_file_info *claimed_files = NULL;
> >  static unsigned int num_claimed_files = 0;
> > @@ -1407,6 +1412,29 @@ process_option (const char *option)
> >    verbose = verbose || debug;
> >  }
> >
> > +static int
> > +get_api_version (char *linker_identifier, int linker_version,
> > +              int preferred_linker_api,
>
> The 'preferred' qualifier seems vague. If you go with my suggestion above,
> I'd suggest to pass lowest and highest supported version number, and the linker
> can check if that intersects with its range of supported versions, and error out
> if the intersection is empty (and otherwise return the highest version they both
> support as the 'negotiated' one).

That sounds better indeed.  As said above the API level means nothing if
constraints are not thoroughly documented.

> > +              const char **compiler_identifier,
> > +              int *compiler_version)
> > +{
> > +  *compiler_identifier = "GCC";
> > +  *compiler_version = GCC_VERSION;
>
> Note that I'm chiming in here because I worked on a tool that used LTO plugin
> API to discover symbol/section dependencies with high accuracy. So I'd like to
> remind that implementors/consumers of this API are not necessarily compilers!

Indeed.  For example nm/ar when auto-loading the plugin might operate on
LTO IL that is produced by a compiler newer than the sources the linker-plugin
was compiled with.  So maybe it should be 'linker plugin identifier', not
'compiler identifier', likewise for version.  For the GCC source based plugin
it can of course still use GCCs identifier / version.

Richard.

> Alexander
  

On 5/16/22 17:16, Alexander Monakov wrote:
> On Mon, 16 May 2022, Martin Liška wrote:
> 
>> I've implemented first version of the patch, please take a look.
> 
> I'll comment on the patch, feel free to inform me when I should back off
> with forcing my opinion in this thread :)

I do really welcome your suggestions Alexander ;)

> 
>> --- a/include/plugin-api.h
>> +++ b/include/plugin-api.h
>> @@ -483,6 +483,18 @@ enum ld_plugin_level
>>    LDPL_FATAL
>>  };
>>  
>> +/* The linker's interface for API version negotiation.  */
>> +
>> +typedef
>> +int (*ld_plugin_get_api_version) (char *linker_identifier, int linker_version,
>> +				  int preferred_linker_api,
>> +				  const char **compiler_identifier,
>> +				  int *compiler_version);
>> +
>> +typedef
>> +enum ld_plugin_status
>> +(*ld_plugin_register_get_api_version) (ld_plugin_get_api_version handler);
>> +
>>  /* Values for the tv_tag field of the transfer vector.  */
>>  
>>  enum ld_plugin_tag
>> @@ -521,6 +533,7 @@ enum ld_plugin_tag
>>    LDPT_REGISTER_NEW_INPUT_HOOK,
>>    LDPT_GET_WRAP_SYMBOLS,
>>    LDPT_ADD_SYMBOLS_V2,
>> +  LDPT_REGISTER_GET_API_VERSION,
>>  };
>>  
>>  /* The plugin transfer vector.  */
>> @@ -556,6 +569,7 @@ struct ld_plugin_tv
>>      ld_plugin_get_input_section_size tv_get_input_section_size;
>>      ld_plugin_register_new_input tv_register_new_input;
>>      ld_plugin_get_wrap_symbols tv_get_wrap_symbols;
>> +    ld_plugin_register_get_api_version tv_register_get_api_version;
>>    } tv_u;
>>  };
> 
> Here I disagree with the overall design. Rui already pointed out how plugin
> API seems to consist of callbacks-that-register-callbacks, and I'm with him
> on that, let's not make that worse. On a more serious note, this pattern:
> 
> * the linker provides register_get_api_version entrypoint
> * the plugin registers its get_api_version implementation
> * the linker uses the provided function pointer
> 
> is problematic because the plugin doesn't know when the linker is going to
> invoke its callback (or maybe the linker won't do that at all).

Yes, depends on what direction of the communication do we want to implement.
My implementation was that linker provides API version request, its version
and then it's plugin which decides about a version and that value is returned
to linker (via its callback).

> 
> I'd recommend to reduce the level of indirection, remove the register_
> callback, and simply require that if LDPT_GET_API_VERSION is provided,
> the plugin MUST invoke it before returning from onload, i.e.:
> 
> * the linker invokes onload with LDPT_GET_API_VERSION in 'transfer vector'
> * the plugin iterates over the transfer vector and notes if LDPT_GET_API_VERSION
>   is seen
>   * if not, the plugin knows the linker is predates its introduction
>   * if yes, the plugin invokes it before returning from onload
> * the linker now knows the plugin version (either one provided via
>   LDPT_GET_API_VERSION, or 'old' if the callback wasn't invoked).

All right, so it will be linker who will make a decision...

> 
>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
>> index 00b760636dc..49484decd89 100644
>> --- a/lto-plugin/lto-plugin.c
>> +++ b/lto-plugin/lto-plugin.c
>> @@ -69,6 +69,7 @@ along with this program; see the file COPYING3.  If not see
>>  #include "../gcc/lto/common.h"
>>  #include "simple-object.h"
>>  #include "plugin-api.h"
>> +#include "ansidecl.h"
>>  
>>  /* We need to use I64 instead of ll width-specifier on native Windows.
>>     The reason for this is that older MS-runtimes don't support the ll.  */
>> @@ -166,6 +167,10 @@ static ld_plugin_add_input_file add_input_file;
>>  static ld_plugin_add_input_library add_input_library;
>>  static ld_plugin_message message;
>>  static ld_plugin_add_symbols add_symbols, add_symbols_v2;
>> +static ld_plugin_register_get_api_version register_get_api_version;
>> +
>> +/* By default, use version 1 if there is not negotiation.  */
>> +static int used_api_version = 1;
>>  
>>  static struct plugin_file_info *claimed_files = NULL;
>>  static unsigned int num_claimed_files = 0;
>> @@ -1407,6 +1412,29 @@ process_option (const char *option)
>>    verbose = verbose || debug;
>>  }
>>  
>> +static int
>> +get_api_version (char *linker_identifier, int linker_version,
>> +		 int preferred_linker_api,
> 
> The 'preferred' qualifier seems vague. If you go with my suggestion above,
> I'd suggest to pass lowest and highest supported version number, and the linker
> can check if that intersects with its range of supported versions, and error out
> if the intersection is empty (and otherwise return the highest version they both
> support as the 'negotiated' one).

... so the plug-in tells the linker about range of versions and linker will make a decision.
Got it, lemme prepare v2 of the patch.

> 
>> +		 const char **compiler_identifier,
>> +		 int *compiler_version)
>> +{
>> +  *compiler_identifier = "GCC";
>> +  *compiler_version = GCC_VERSION;
> 
> Note that I'm chiming in here because I worked on a tool that used LTO plugin
> API to discover symbol/section dependencies with high accuracy. So I'd like to
> remind that implementors/consumers of this API are not necessarily compilers!

Sure!

Martin

> 
> Alexander