diff mbox series

[3/3] lto-plugin: implement LDPT_GET_API_VERSION

Message ID bb9bf388-21d7-80d8-8474-6453fae50a49@suse.cz
State New
Headers
Series [1/3] lto-plugin: support LDPT_GET_SYMBOLS_V3 |

Commit Message

Martin Liška June 16, 2022, 7:01 a.m. UTC 
  Hi.

I'm sending updated version of the patch where I addressed the comments.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

include/ChangeLog:

	* plugin-api.h (enum linker_api_version): New enum.
	(ld_plugin_get_api_version): New.
	(enum ld_plugin_tag): Add LDPT_GET_API_VERSION.
	(struct ld_plugin_tv): Add tv_get_api_version.

lto-plugin/ChangeLog:

	* lto-plugin.c (negotiate_api_version): New.
	(onload): Negotiate API version.
---
 include/plugin-api.h    | 30 ++++++++++++++++++++++++++++++
 lto-plugin/lto-plugin.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

Comments

Alexander Monakov June 16, 2022, 8 a.m. UTC | #1 
On Thu, 16 Jun 2022, Martin Liška wrote:

> Hi.
> 
> I'm sending updated version of the patch where I addressed the comments.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?

I noticed a typo (no objection on the substance on the patch from me):

> --- a/include/plugin-api.h
> +++ b/include/plugin-api.h
> @@ -483,6 +483,34 @@ enum ld_plugin_level
>    LDPL_FATAL
>  };
>  
> +/* Contract between a plug-in and a linker.  */
> +
> +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, add_symbols_v2,

This should be '*get_*symbols_v3, add_symbols_v2'.

> +      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
> +};
> +
> +/* The linker's interface for API version negotiation.  A plug-in calls
> +  the function (with its IDENTIFIER and VERSION), plus minimal and maximal
> +  version of linker_api_version is provided.  Linker then returns selected
> +  API version and provides its IDENTIFIER and VERSION.  */
> +
> +typedef
> +enum linker_api_version
> +(*ld_plugin_get_api_version) (const char *plugin_identifier, unsigned plugin_version,
> +			      enum linker_api_version minimal_api_supported,
> +			      enum linker_api_version maximal_api_supported,
> +			      const char **linker_identifier,
> +			      unsigned *linker_version);

IIRC Richi asked to mention which side owns the strings (does the receiver need
to 'free' or 'strdup' them). Perhaps we could say they are owned by the
originating side, but it might be even better to say they are unchanging to
allow simply using string literals. Perhaps add something like this to the
comment?

    Identifier pointers remain valid as long as the plugin is loaded.

>  /* Values for the tv_tag field of the transfer vector.  */
>  
>  enum ld_plugin_tag
> @@ -521,6 +549,7 @@ enum ld_plugin_tag
>    LDPT_REGISTER_NEW_INPUT_HOOK,
>    LDPT_GET_WRAP_SYMBOLS,
>    LDPT_ADD_SYMBOLS_V2,
> +  LDPT_GET_API_VERSION,
>  };

I went checking if this is in sync with Binutils header and noticed that
get_wrap_symbols and add_symbols_v2 are not even mentioned on the wiki page with
plugin API documentation.

Alexander
Martin Liška June 16, 2022, 12:25 p.m. UTC | #2 
On 6/16/22 10:00, Alexander Monakov wrote:
> On Thu, 16 Jun 2022, Martin Liška wrote:
> 
>> Hi.
>>
>> I'm sending updated version of the patch where I addressed the comments.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
> 
> I noticed a typo (no objection on the substance on the patch from me):

Good!

> 
>> --- a/include/plugin-api.h
>> +++ b/include/plugin-api.h
>> @@ -483,6 +483,34 @@ enum ld_plugin_level
>>    LDPL_FATAL
>>  };
>>  
>> +/* Contract between a plug-in and a linker.  */
>> +
>> +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, add_symbols_v2,
> 
> This should be '*get_*symbols_v3, add_symbols_v2'.

Sure, fixed.

> 
>> +      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
>> +};
>> +
>> +/* The linker's interface for API version negotiation.  A plug-in calls
>> +  the function (with its IDENTIFIER and VERSION), plus minimal and maximal
>> +  version of linker_api_version is provided.  Linker then returns selected
>> +  API version and provides its IDENTIFIER and VERSION.  */
>> +
>> +typedef
>> +enum linker_api_version
>> +(*ld_plugin_get_api_version) (const char *plugin_identifier, unsigned plugin_version,
>> +			      enum linker_api_version minimal_api_supported,
>> +			      enum linker_api_version maximal_api_supported,
>> +			      const char **linker_identifier,
>> +			      unsigned *linker_version);
> 
> IIRC Richi asked to mention which side owns the strings (does the receiver need
> to 'free' or 'strdup' them). Perhaps we could say they are owned by the
> originating side, but it might be even better to say they are unchanging to
> allow simply using string literals. Perhaps add something like this to the
> comment?
> 
>     Identifier pointers remain valid as long as the plugin is loaded.

I welcome the change and I'm sending a patch that incorporates that.

> 
>>  /* Values for the tv_tag field of the transfer vector.  */
>>  
>>  enum ld_plugin_tag
>> @@ -521,6 +549,7 @@ enum ld_plugin_tag
>>    LDPT_REGISTER_NEW_INPUT_HOOK,
>>    LDPT_GET_WRAP_SYMBOLS,
>>    LDPT_ADD_SYMBOLS_V2,
>> +  LDPT_GET_API_VERSION,
>>  };
> 
> I went checking if this is in sync with Binutils header and noticed that
> get_wrap_symbols and add_symbols_v2 are not even mentioned on the wiki page with
> plugin API documentation.

Yes, I know about that. I'm going to update wiki page once we get this in.

Cheers,
Martin

> 
> Alexander
Richard Biener June 20, 2022, 9:35 a.m. UTC | #3 
On Thu, Jun 16, 2022 at 2:25 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 6/16/22 10:00, Alexander Monakov wrote:
> > On Thu, 16 Jun 2022, Martin Liška wrote:
> >
> >> Hi.
> >>
> >> I'm sending updated version of the patch where I addressed the comments.
> >>
> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>
> >> Ready to be installed?
> >
> > I noticed a typo (no objection on the substance on the patch from me):
>
> Good!
>
> >
> >> --- a/include/plugin-api.h
> >> +++ b/include/plugin-api.h
> >> @@ -483,6 +483,34 @@ enum ld_plugin_level
> >>    LDPL_FATAL
> >>  };
> >>
> >> +/* Contract between a plug-in and a linker.  */
> >> +
> >> +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,

I'll note this is somewhat redundant with the presence of
ld_plugin_get_api_version,
but only somewhat since it also provides info about the linker/plugin
identifier.

> >> +   /* API level v1.  The linker provides add_symbols_v3, add_symbols_v2,
> >
> > This should be '*get_*symbols_v3, add_symbols_v2'.
>
> Sure, fixed.
>
> >
> >> +      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
> >> +};
> >> +
> >> +/* The linker's interface for API version negotiation.  A plug-in calls
> >> +  the function (with its IDENTIFIER and VERSION), plus minimal and maximal
> >> +  version of linker_api_version is provided.  Linker then returns selected
> >> +  API version and provides its IDENTIFIER and VERSION.  */
> >> +
> >> +typedef
> >> +enum linker_api_version
> >> +(*ld_plugin_get_api_version) (const char *plugin_identifier, unsigned plugin_version,
> >> +                          enum linker_api_version minimal_api_supported,
> >> +                          enum linker_api_version maximal_api_supported,
> >> +                          const char **linker_identifier,
> >> +                          unsigned *linker_version);
> >
> > IIRC Richi asked to mention which side owns the strings (does the receiver need
> > to 'free' or 'strdup' them). Perhaps we could say they are owned by the
> > originating side, but it might be even better to say they are unchanging to
> > allow simply using string literals. Perhaps add something like this to the
> > comment?
> >
> >     Identifier pointers remain valid as long as the plugin is loaded.
>
> I welcome the change and I'm sending a patch that incorporates that.
>
> >
> >>  /* Values for the tv_tag field of the transfer vector.  */
> >>
> >>  enum ld_plugin_tag
> >> @@ -521,6 +549,7 @@ enum ld_plugin_tag
> >>    LDPT_REGISTER_NEW_INPUT_HOOK,
> >>    LDPT_GET_WRAP_SYMBOLS,
> >>    LDPT_ADD_SYMBOLS_V2,
> >> +  LDPT_GET_API_VERSION,
> >>  };
> >
> > I went checking if this is in sync with Binutils header and noticed that
> > get_wrap_symbols and add_symbols_v2 are not even mentioned on the wiki page with
> > plugin API documentation.
>
> Yes, I know about that. I'm going to update wiki page once we get this in.

I think this is OK.  Can we get buy-in from mold people?

Thanks,
Richard.

> Cheers,
> Martin
>
> >
> > Alexander
Martin Liška June 20, 2022, 1:01 p.m. UTC | #4 
On 6/20/22 11:35, Richard Biener wrote:
> I think this is OK.  Can we get buy-in from mold people?

Sure, I've just pinged Rui:
https://github.com/rui314/mold/issues/454#issuecomment-1160419030

Martin
Rui Ueyama June 30, 2022, 6:43 a.m. UTC | #5 
Thanks Martin for creating this patch.

Here is a preliminary change for the mold side:
https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605

Overall the API is looking fine, though it is not clear what kind of value
is expected as a linker version. A linker version is not a single unsigned
integer but something like "1.3.0". Something like "1.3.0-rc2" can also be
a linker version. So I don't think we can represent a linker version as a
single integer.

On Mon, Jun 20, 2022 at 9:01 PM Martin Liška <mliska@suse.cz> wrote:

> On 6/20/22 11:35, Richard Biener wrote:
> > I think this is OK.  Can we get buy-in from mold people?
>
> Sure, I've just pinged Rui:
> https://github.com/rui314/mold/issues/454#issuecomment-1160419030
>
> Martin
>
Martin Liška June 30, 2022, 8:42 a.m. UTC | #6 
On 6/30/22 08:43, Rui Ueyama wrote:
> Thanks Martin for creating this patch.

You're welcome.

> 
> Here is a preliminary change for the mold side: https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605 <https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605>
> 
> Overall the API is looking fine,

Good then!

> though it is not clear what kind of value is expected as a linker version. A linker version is not a single unsigned integer but something like "1.3.0". Something like "1.3.0-rc2" can also be a linker version. So I don't think we can represent a linker version as a single integer.

Well, you can use the same what we use GCC_VERSION (plugin_version):

1000 * MAJOR + MINOR

Let me adjust the documentation of the API.

Richi: May I install the patch?

Thanks,
Martin

> 
> On Mon, Jun 20, 2022 at 9:01 PM Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz>> wrote:
> 
>     On 6/20/22 11:35, Richard Biener wrote:
>     > I think this is OK.  Can we get buy-in from mold people?
> 
>     Sure, I've just pinged Rui:
>     https://github.com/rui314/mold/issues/454#issuecomment-1160419030 <https://github.com/rui314/mold/issues/454#issuecomment-1160419030>
> 
>     Martin
>
Richard Biener July 1, 2022, 6:36 a.m. UTC | #7 
On Thu, Jun 30, 2022 at 10:42 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 6/30/22 08:43, Rui Ueyama wrote:
> > Thanks Martin for creating this patch.
>
> You're welcome.
>
> >
> > Here is a preliminary change for the mold side: https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605 <https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605>
> >
> > Overall the API is looking fine,
>
> Good then!
>
> > though it is not clear what kind of value is expected as a linker version. A linker version is not a single unsigned integer but something like "1.3.0". Something like "1.3.0-rc2" can also be a linker version. So I don't think we can represent a linker version as a single integer.
>
> Well, you can use the same what we use GCC_VERSION (plugin_version):
>
> 1000 * MAJOR + MINOR
>
> Let me adjust the documentation of the API.

Hmm, but then why not go back to the original suggestion merging
linker_identifier and linker_version into
a single string.  That of course puts the burden of parsing to the
consumer - still that's probably better
than imposing the constraint of encoding the version in an unsigned
integer.  Alternatively easing
parsing by separating out the version in a string would be possible as
well (but then you'd have
to care for 1.3.0-rc2+gitab4316174 or so, not sure what the advantage
over putting everything in
the identifier would be).

You usually cannot rely on a version anyway since distributors usually
apply patches.

> Richi: May I install the patch?

Let's sort out the version thing and consider simplifying the API.

Richard.

> Thanks,
> Martin
>
> >
> > On Mon, Jun 20, 2022 at 9:01 PM Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz>> wrote:
> >
> >     On 6/20/22 11:35, Richard Biener wrote:
> >     > I think this is OK.  Can we get buy-in from mold people?
> >
> >     Sure, I've just pinged Rui:
> >     https://github.com/rui314/mold/issues/454#issuecomment-1160419030 <https://github.com/rui314/mold/issues/454#issuecomment-1160419030>
> >
> >     Martin
> >
Martin Liška July 4, 2022, 2:17 p.m. UTC | #8 
On 7/1/22 08:36, Richard Biener wrote:
> On Thu, Jun 30, 2022 at 10:42 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 6/30/22 08:43, Rui Ueyama wrote:
>>> Thanks Martin for creating this patch.
>>
>> You're welcome.
>>
>>>
>>> Here is a preliminary change for the mold side: https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605 <https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605>
>>>
>>> Overall the API is looking fine,
>>
>> Good then!
>>
>>> though it is not clear what kind of value is expected as a linker version. A linker version is not a single unsigned integer but something like "1.3.0". Something like "1.3.0-rc2" can also be a linker version. So I don't think we can represent a linker version as a single integer.
>>
>> Well, you can use the same what we use GCC_VERSION (plugin_version):
>>
>> 1000 * MAJOR + MINOR
>>
>> Let me adjust the documentation of the API.
> 
> Hmm, but then why not go back to the original suggestion merging
> linker_identifier and linker_version into
> a single string.  That of course puts the burden of parsing to the
> consumer - still that's probably better
> than imposing the constraint of encoding the version in an unsigned
> integer.  Alternatively easing
> parsing by separating out the version in a string would be possible as
> well (but then you'd have
> to care for 1.3.0-rc2+gitab4316174 or so, not sure what the advantage
> over putting everything in
> the identifier would be).

I'm fine with the suggested 2 strings (linker_identifier and linker_version).

Does it work for you Rui?

Cheers,
Martin

> 
> You usually cannot rely on a version anyway since distributors usually
> apply patches.
> 
>> Richi: May I install the patch?
> 
> Let's sort out the version thing and consider simplifying the API.
> 
> Richard.
> 
>> Thanks,
>> Martin
>>
>>>
>>> On Mon, Jun 20, 2022 at 9:01 PM Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz>> wrote:
>>>
>>>     On 6/20/22 11:35, Richard Biener wrote:
>>>     > I think this is OK.  Can we get buy-in from mold people?
>>>
>>>     Sure, I've just pinged Rui:
>>>     https://github.com/rui314/mold/issues/454#issuecomment-1160419030 <https://github.com/rui314/mold/issues/454#issuecomment-1160419030>
>>>
>>>     Martin
>>>
Rui Ueyama July 7, 2022, 2:19 a.m. UTC | #9 
On Mon, Jul 4, 2022 at 10:17 PM Martin Liška <mliska@suse.cz> wrote:

> On 7/1/22 08:36, Richard Biener wrote:
> > On Thu, Jun 30, 2022 at 10:42 AM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 6/30/22 08:43, Rui Ueyama wrote:
> >>> Thanks Martin for creating this patch.
> >>
> >> You're welcome.
> >>
> >>>
> >>> Here is a preliminary change for the mold side:
> https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605
> <
> https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605
> >
> >>>
> >>> Overall the API is looking fine,
> >>
> >> Good then!
> >>
> >>> though it is not clear what kind of value is expected as a linker
> version. A linker version is not a single unsigned integer but something
> like "1.3.0". Something like "1.3.0-rc2" can also be a linker version. So I
> don't think we can represent a linker version as a single integer.
> >>
> >> Well, you can use the same what we use GCC_VERSION (plugin_version):
> >>
> >> 1000 * MAJOR + MINOR
> >>
> >> Let me adjust the documentation of the API.
> >
> > Hmm, but then why not go back to the original suggestion merging
> > linker_identifier and linker_version into
> > a single string.  That of course puts the burden of parsing to the
> > consumer - still that's probably better
> > than imposing the constraint of encoding the version in an unsigned
> > integer.  Alternatively easing
> > parsing by separating out the version in a string would be possible as
> > well (but then you'd have
> > to care for 1.3.0-rc2+gitab4316174 or so, not sure what the advantage
> > over putting everything in
> > the identifier would be).
>
> I'm fine with the suggested 2 strings (linker_identifier and
> linker_version).
>
> Does it work for you Rui?
>

Yes.


> Cheers,
> Martin
>
> >
> > You usually cannot rely on a version anyway since distributors usually
> > apply patches.
> >
> >> Richi: May I install the patch?
> >
> > Let's sort out the version thing and consider simplifying the API.
> >
> > Richard.
> >
> >> Thanks,
> >> Martin
> >>
> >>>
> >>> On Mon, Jun 20, 2022 at 9:01 PM Martin Liška <mliska@suse.cz <mailto:
> mliska@suse.cz>> wrote:
> >>>
> >>>     On 6/20/22 11:35, Richard Biener wrote:
> >>>     > I think this is OK.  Can we get buy-in from mold people?
> >>>
> >>>     Sure, I've just pinged Rui:
> >>>     https://github.com/rui314/mold/issues/454#issuecomment-1160419030
> <https://github.com/rui314/mold/issues/454#issuecomment-1160419030>
> >>>
> >>>     Martin
> >>>
>
>
Martin Liška July 8, 2022, 8:42 a.m. UTC | #10 
Hi.

All right, there's updated version of the patch that reflects the following suggestions:

1) strings are used for version identification
2) thread-safe API version (1) is not used if target does not support locking via pthreads

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

On 7/7/22 04:19, Rui Ueyama wrote:
> On Mon, Jul 4, 2022 at 10:17 PM Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz>> wrote:
> 
>     On 7/1/22 08:36, Richard Biener wrote:
>     > On Thu, Jun 30, 2022 at 10:42 AM Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz>> wrote:
>     >>
>     >> On 6/30/22 08:43, Rui Ueyama wrote:
>     >>> Thanks Martin for creating this patch.
>     >>
>     >> You're welcome.
>     >>
>     >>>
>     >>> Here is a preliminary change for the mold side: https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605 <https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605> <https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605 <https://github.com/rui314/mold/commit/9ad49d1c556bc963d06cca8233535183490de605>>
>     >>>
>     >>> Overall the API is looking fine,
>     >>
>     >> Good then!
>     >>
>     >>> though it is not clear what kind of value is expected as a linker version. A linker version is not a single unsigned integer but something like "1.3.0". Something like "1.3.0-rc2" can also be a linker version. So I don't think we can represent a linker version as a single integer.
>     >>
>     >> Well, you can use the same what we use GCC_VERSION (plugin_version):
>     >>
>     >> 1000 * MAJOR + MINOR
>     >>
>     >> Let me adjust the documentation of the API.
>     >
>     > Hmm, but then why not go back to the original suggestion merging
>     > linker_identifier and linker_version into
>     > a single string.  That of course puts the burden of parsing to the
>     > consumer - still that's probably better
>     > than imposing the constraint of encoding the version in an unsigned
>     > integer.  Alternatively easing
>     > parsing by separating out the version in a string would be possible as
>     > well (but then you'd have
>     > to care for 1.3.0-rc2+gitab4316174 or so, not sure what the advantage
>     > over putting everything in
>     > the identifier would be).
> 
>     I'm fine with the suggested 2 strings (linker_identifier and linker_version).
> 
>     Does it work for you Rui?
> 
> 
> Yes.
>  
> 
>     Cheers,
>     Martin
> 
>     >
>     > You usually cannot rely on a version anyway since distributors usually
>     > apply patches.
>     >
>     >> Richi: May I install the patch?
>     >
>     > Let's sort out the version thing and consider simplifying the API.
>     >
>     > Richard.
>     >
>     >> Thanks,
>     >> Martin
>     >>
>     >>>
>     >>> On Mon, Jun 20, 2022 at 9:01 PM Martin Liška <mliska@suse.cz <mailto:mliska@suse.cz> <mailto:mliska@suse.cz <mailto:mliska@suse.cz>>> wrote:
>     >>>
>     >>>     On 6/20/22 11:35, Richard Biener wrote:
>     >>>     > I think this is OK.  Can we get buy-in from mold people?
>     >>>
>     >>>     Sure, I've just pinged Rui:
>     >>>     https://github.com/rui314/mold/issues/454#issuecomment-1160419030 <https://github.com/rui314/mold/issues/454#issuecomment-1160419030> <https://github.com/rui314/mold/issues/454#issuecomment-1160419030 <https://github.com/rui314/mold/issues/454#issuecomment-1160419030>>
>     >>>
>     >>>     Martin
>     >>>
>
Alexander Monakov July 8, 2022, 12:41 p.m. UTC | #11 
On Fri, 8 Jul 2022, Martin Liška wrote:

> Hi.
> 
> All right, there's updated version of the patch that reflects the following suggestions:
> 
> 1) strings are used for version identification
> 2) thread-safe API version (1) is not used if target does not support locking via pthreads
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?

Note that mold side will need to be adjusted, because it did not really
implement the proposed contract. Maybe we should be more clear how the
linker is supposed to implement this? Preliminary mold patch does this:

static PluginLinkerAPIVersion
get_api_version(const char *plugin_identifier,
                unsigned plugin_version,
                PluginLinkerAPIVersion minimal_api_supported,
                PluginLinkerAPIVersion maximal_api_supported,
                const char **linker_identifier,
                unsigned *linker_version) {
  assert(maximal_api_supported >= LAPI_V1);
  *linker_identifier = "mold";
  *linker_version = get_mold_version();
  is_gcc_linker_api_v1 = true;
  return LAPI_V1;
}

but ignoring min_api_supported is wrong, and assuming max_api_supported > 0
is also wrong. It really should check how given [min; max] range intersects
with its own range of supported versions.

Alexande
Rui Ueyama July 11, 2022, 7:23 a.m. UTC | #12 
On Fri, Jul 8, 2022 at 8:41 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
> On Fri, 8 Jul 2022, Martin Liška wrote:
>
> > Hi.
> >
> > All right, there's updated version of the patch that reflects the following suggestions:
> >
> > 1) strings are used for version identification
> > 2) thread-safe API version (1) is not used if target does not support locking via pthreads
> >
> > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >
> > Ready to be installed?
>
> Note that mold side will need to be adjusted, because it did not really
> implement the proposed contract. Maybe we should be more clear how the
> linker is supposed to implement this? Preliminary mold patch does this:
>
> static PluginLinkerAPIVersion
> get_api_version(const char *plugin_identifier,
>                 unsigned plugin_version,
>                 PluginLinkerAPIVersion minimal_api_supported,
>                 PluginLinkerAPIVersion maximal_api_supported,
>                 const char **linker_identifier,
>                 unsigned *linker_version) {
>   assert(maximal_api_supported >= LAPI_V1);
>   *linker_identifier = "mold";
>   *linker_version = get_mold_version();
>   is_gcc_linker_api_v1 = true;
>   return LAPI_V1;
> }
>
> but ignoring min_api_supported is wrong, and assuming max_api_supported > 0
> is also wrong. It really should check how given [min; max] range intersects
> with its own range of supported versions.

Currently only one version is defined which is LAPI_V1. I don't think
LAPI_UNSPECIFIED is a version number; rather, it's an unspecified
value. No ordering should be defined between a defined value and an
unspecified value. If LAPI_UNSPECIFIED < LAPI_V1, it should be renamed
LAPI_V0.
Alexander Monakov July 11, 2022, 9:16 a.m. UTC | #13 
On Mon, 11 Jul 2022, Rui Ueyama wrote:

> > but ignoring min_api_supported is wrong, and assuming max_api_supported > 0
> > is also wrong. It really should check how given [min; max] range intersects
> > with its own range of supported versions.
> 
> Currently only one version is defined which is LAPI_V1. I don't think
> LAPI_UNSPECIFIED is a version number; rather, it's an unspecified
> value. No ordering should be defined between a defined value and an
> unspecified value. If LAPI_UNSPECIFIED < LAPI_V1, it should be renamed
> LAPI_V0.

You still cannot rely on API guarantees of LAPI_V1 when the plugin does not
advertise it (thread safety of claim_file in this particular case).

And you still should check the intersection of supported API ranges
and give a sane diagnostic when min_api_supported advertised by the plugin
exceeds LAPI_V1 (though, granted, the plugin could error out as well in this
case).

Alexander
Richard Biener July 11, 2022, 9:55 a.m. UTC | #14 
On Mon, Jul 11, 2022 at 11:16 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Mon, 11 Jul 2022, Rui Ueyama wrote:
>
> > > but ignoring min_api_supported is wrong, and assuming max_api_supported > 0
> > > is also wrong. It really should check how given [min; max] range intersects
> > > with its own range of supported versions.
> >
> > Currently only one version is defined which is LAPI_V1. I don't think
> > LAPI_UNSPECIFIED is a version number; rather, it's an unspecified
> > value. No ordering should be defined between a defined value and an
> > unspecified value. If LAPI_UNSPECIFIED < LAPI_V1, it should be renamed
> > LAPI_V0.
>
> You still cannot rely on API guarantees of LAPI_V1 when the plugin does not
> advertise it (thread safety of claim_file in this particular case).

So with LAPI_UNSPECIFIED all the plugin gets is the linker name and version.
Clarifying the documentation on LAPI_UNSPECIFIED might be nice, also
what the expectation is on the linker when the plugin returns
LAPI_UNSPECIFIED when it speficied minimal_api_supported == V1.
"minimal_api_supported == LAPI_UNSPECIFIED" does not make much
sense if using Ruis reading of this value?

> And you still should check the intersection of supported API ranges
> and give a sane diagnostic when min_api_supported advertised by the plugin
> exceeds LAPI_V1 (though, granted, the plugin could error out as well in this
> case).
>
> Alexander
Martin Liška July 11, 2022, 10:51 a.m. UTC | #15 
On 7/11/22 11:55, Richard Biener wrote:
> On Mon, Jul 11, 2022 at 11:16 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>>
>> On Mon, 11 Jul 2022, Rui Ueyama wrote:
>>
>>>> but ignoring min_api_supported is wrong, and assuming max_api_supported > 0
>>>> is also wrong. It really should check how given [min; max] range intersects
>>>> with its own range of supported versions.
>>>
>>> Currently only one version is defined which is LAPI_V1. I don't think
>>> LAPI_UNSPECIFIED is a version number; rather, it's an unspecified
>>> value. No ordering should be defined between a defined value and an
>>> unspecified value. If LAPI_UNSPECIFIED < LAPI_V1, it should be renamed
>>> LAPI_V0.
>>
>> You still cannot rely on API guarantees of LAPI_V1 when the plugin does not
>> advertise it (thread safety of claim_file in this particular case).
> 

Hi.

All right, I think we should rename LAPI_UNSPECIFIED to LAPI_V0 in order
to support minimal_api_supported == LAPI_V0.

> So with LAPI_UNSPECIFIED all the plugin gets is the linker name and version.
> Clarifying the documentation on LAPI_UNSPECIFIED might be nice, also
> what the expectation is on the linker when the plugin returns
> LAPI_UNSPECIFIED when it speficied minimal_api_supported == V1.

I've clarified that linker should return a value that is in range
[minimal_api_supported, maximal_api_supported] and added an abort
if it's not the case.

Having that, mold should respect if maximal_api_supported == LAPI_V0 is returned
by a plug-in (happens now as we miss locking for some targets).

Martin

> "minimal_api_supported == LAPI_UNSPECIFIED" does not make much
> sense if using Ruis reading of this value?
> 
>> And you still should check the intersection of supported API ranges
>> and give a sane diagnostic when min_api_supported advertised by the plugin
>> exceeds LAPI_V1 (though, granted, the plugin could error out as well in this
>> case).
>>
>> Alexander
Rui Ueyama July 11, 2022, 12:24 p.m. UTC | #16 
I updated my patch to support the proposed API:
https://github.com/rui314/mold/commit/22bbfa9bba9beeaf40b76481d175939ee2c62ec8

Martin,

I think you want to apply this patch. Currently, your API always
passes LAPI_V0 as the maximum API version.

diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index e9afd2fb76d..c97bda9de91 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -1441,15 +1441,15 @@ negotiate_api_version (void)
   const char *linker_version;

   enum linker_api_version supported_api = LAPI_V0;
 #if HAVE_PTHREAD_LOCKING
   supported_api = LAPI_V1;
 #endif

-  api_version = get_api_version ("GCC", BASE_VERSION, LAPI_V0,
+  api_version = get_api_version ("GCC", BASE_VERSION, LAPI_V1,
                                 supported_api, &linker_identifier,
&linker_version);
   if (api_version > supported_api)
     {
       fprintf (stderr, "requested an unsupported API version (%d)\n",
api_version);
       abort ();
     }

On Mon, Jul 11, 2022 at 6:51 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 7/11/22 11:55, Richard Biener wrote:
> > On Mon, Jul 11, 2022 at 11:16 AM Alexander Monakov <amonakov@ispras.ru> wrote:
> >>
> >> On Mon, 11 Jul 2022, Rui Ueyama wrote:
> >>
> >>>> but ignoring min_api_supported is wrong, and assuming max_api_supported > 0
> >>>> is also wrong. It really should check how given [min; max] range intersects
> >>>> with its own range of supported versions.
> >>>
> >>> Currently only one version is defined which is LAPI_V1. I don't think
> >>> LAPI_UNSPECIFIED is a version number; rather, it's an unspecified
> >>> value. No ordering should be defined between a defined value and an
> >>> unspecified value. If LAPI_UNSPECIFIED < LAPI_V1, it should be renamed
> >>> LAPI_V0.
> >>
> >> You still cannot rely on API guarantees of LAPI_V1 when the plugin does not
> >> advertise it (thread safety of claim_file in this particular case).
> >
>
> Hi.
>
> All right, I think we should rename LAPI_UNSPECIFIED to LAPI_V0 in order
> to support minimal_api_supported == LAPI_V0.
>
> > So with LAPI_UNSPECIFIED all the plugin gets is the linker name and version.
> > Clarifying the documentation on LAPI_UNSPECIFIED might be nice, also
> > what the expectation is on the linker when the plugin returns
> > LAPI_UNSPECIFIED when it speficied minimal_api_supported == V1.
>
> I've clarified that linker should return a value that is in range
> [minimal_api_supported, maximal_api_supported] and added an abort
> if it's not the case.
>
> Having that, mold should respect if maximal_api_supported == LAPI_V0 is returned
> by a plug-in (happens now as we miss locking for some targets).
>
> Martin
>
> > "minimal_api_supported == LAPI_UNSPECIFIED" does not make much
> > sense if using Ruis reading of this value?
> >
> >> And you still should check the intersection of supported API ranges
> >> and give a sane diagnostic when min_api_supported advertised by the plugin
> >> exceeds LAPI_V1 (though, granted, the plugin could error out as well in this
> >> case).
> >>
> >> Alexander
Alexander Monakov July 11, 2022, 12:38 p.m. UTC | #17 
On Mon, 11 Jul 2022, Rui Ueyama wrote:

> I updated my patch to support the proposed API:
> https://github.com/rui314/mold/commit/22bbfa9bba9beeaf40b76481d175939ee2c62ec8

This still seems to ignore the thread safety aspect.

Alexander
Martin Liška July 11, 2022, 12:51 p.m. UTC | #18 
On 7/11/22 14:24, Rui Ueyama wrote:
> I updated my patch to support the proposed API:
> https://github.com/rui314/mold/commit/22bbfa9bba9beeaf40b76481d175939ee2c62ec8
> 
> Martin,
> 
> I think you want to apply this patch. Currently, your API always
> passes LAPI_V0 as the maximum API version.

Are you sure?

The function signature is:

(*ld_plugin_get_api_version) (const char *plugin_identifier,
                             const char *plugin_version,
                             enum linker_api_version minimal_api_supported,
                             enum linker_api_version maximal_api_supported,
...

Which means the plug-in always set minimal as LAPI_V0 and maximal
LAPI_V0/V1. That seems correct to me.

Martin


> 
> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> index e9afd2fb76d..c97bda9de91 100644
> --- a/lto-plugin/lto-plugin.c
> +++ b/lto-plugin/lto-plugin.c
> @@ -1441,15 +1441,15 @@ negotiate_api_version (void)
>    const char *linker_version;
> 
>    enum linker_api_version supported_api = LAPI_V0;
>  #if HAVE_PTHREAD_LOCKING
>    supported_api = LAPI_V1;
>  #endif
> 
> -  api_version = get_api_version ("GCC", BASE_VERSION, LAPI_V0,
> +  api_version = get_api_version ("GCC", BASE_VERSION, LAPI_V1,
>                                  supported_api, &linker_identifier,
> &linker_version);
>    if (api_version > supported_api)
>      {
>        fprintf (stderr, "requested an unsupported API version (%d)\n",
> api_version);
>        abort ();
>      }
> 
> On Mon, Jul 11, 2022 at 6:51 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 7/11/22 11:55, Richard Biener wrote:
>>> On Mon, Jul 11, 2022 at 11:16 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>>>>
>>>> On Mon, 11 Jul 2022, Rui Ueyama wrote:
>>>>
>>>>>> but ignoring min_api_supported is wrong, and assuming max_api_supported > 0
>>>>>> is also wrong. It really should check how given [min; max] range intersects
>>>>>> with its own range of supported versions.
>>>>>
>>>>> Currently only one version is defined which is LAPI_V1. I don't think
>>>>> LAPI_UNSPECIFIED is a version number; rather, it's an unspecified
>>>>> value. No ordering should be defined between a defined value and an
>>>>> unspecified value. If LAPI_UNSPECIFIED < LAPI_V1, it should be renamed
>>>>> LAPI_V0.
>>>>
>>>> You still cannot rely on API guarantees of LAPI_V1 when the plugin does not
>>>> advertise it (thread safety of claim_file in this particular case).
>>>
>>
>> Hi.
>>
>> All right, I think we should rename LAPI_UNSPECIFIED to LAPI_V0 in order
>> to support minimal_api_supported == LAPI_V0.
>>
>>> So with LAPI_UNSPECIFIED all the plugin gets is the linker name and version.
>>> Clarifying the documentation on LAPI_UNSPECIFIED might be nice, also
>>> what the expectation is on the linker when the plugin returns
>>> LAPI_UNSPECIFIED when it speficied minimal_api_supported == V1.
>>
>> I've clarified that linker should return a value that is in range
>> [minimal_api_supported, maximal_api_supported] and added an abort
>> if it's not the case.
>>
>> Having that, mold should respect if maximal_api_supported == LAPI_V0 is returned
>> by a plug-in (happens now as we miss locking for some targets).
>>
>> Martin
>>
>>> "minimal_api_supported == LAPI_UNSPECIFIED" does not make much
>>> sense if using Ruis reading of this value?
>>>
>>>> And you still should check the intersection of supported API ranges
>>>> and give a sane diagnostic when min_api_supported advertised by the plugin
>>>> exceeds LAPI_V1 (though, granted, the plugin could error out as well in this
>>>> case).
>>>>
>>>> Alexander
Alexander Monakov July 11, 2022, 4:35 p.m. UTC | #19 
On Mon, 11 Jul 2022, Martin Liška wrote:

> I've clarified that linker should return a value that is in range
> [minimal_api_supported, maximal_api_supported] and added an abort
> if it's not the case.

I noticed that we are placing a trap for C++ consumers such as mold
by passing min/max_api_supported as enum values. Unlike C, C++ disallows
out-of-range enum values, so when mold does

enum PluginLinkerAPIVersion {
  LAPI_V0 = 0,
  LAPI_V1,
};

get_api_version(const char *plugin_identifier,
                unsigned plugin_version,
                PluginLinkerAPIVersion minimal_api_supported,
                PluginLinkerAPIVersion maximal_api_supported,
                const char **linker_identifier,
                const char **linker_version) {

checks such as 'min_api_supported > LAPI_V1' can be optimized out. Also,
if a future tool passes LAPI_V2, it will trigger Clang's UBSan (GCC
-fsanitize-undefined=enum instruments loads but not retrieval of function
arguments).

I'd suggest to fix this on both sides by changing the arguments to plain
integer types.

Alexander
Rui Ueyama July 12, 2022, 1:36 a.m. UTC | #20 
I updated my code so that it now acquires a lock before calling
claim_file if v0.
https://github.com/rui314/mold/commit/54fedf005fb35acdcb0408395aa403f94262190a

On Mon, Jul 11, 2022 at 8:51 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 7/11/22 14:24, Rui Ueyama wrote:
> > I updated my patch to support the proposed API:
> > https://github.com/rui314/mold/commit/22bbfa9bba9beeaf40b76481d175939ee2c62ec8
> >
> > Martin,
> >
> > I think you want to apply this patch. Currently, your API always
> > passes LAPI_V0 as the maximum API version.
>
> Are you sure?
>
> The function signature is:
>
> (*ld_plugin_get_api_version) (const char *plugin_identifier,
>                              const char *plugin_version,
>                              enum linker_api_version minimal_api_supported,
>                              enum linker_api_version maximal_api_supported,
> ...
>
> Which means the plug-in always set minimal as LAPI_V0 and maximal
> LAPI_V0/V1. That seems correct to me.

I'm sorry I misunderstood your code. It looks correct.

> Martin
>
>
> >
> > diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> > index e9afd2fb76d..c97bda9de91 100644
> > --- a/lto-plugin/lto-plugin.c
> > +++ b/lto-plugin/lto-plugin.c
> > @@ -1441,15 +1441,15 @@ negotiate_api_version (void)
> >    const char *linker_version;
> >
> >    enum linker_api_version supported_api = LAPI_V0;
> >  #if HAVE_PTHREAD_LOCKING
> >    supported_api = LAPI_V1;
> >  #endif
> >
> > -  api_version = get_api_version ("GCC", BASE_VERSION, LAPI_V0,
> > +  api_version = get_api_version ("GCC", BASE_VERSION, LAPI_V1,
> >                                  supported_api, &linker_identifier,
> > &linker_version);
> >    if (api_version > supported_api)
> >      {
> >        fprintf (stderr, "requested an unsupported API version (%d)\n",
> > api_version);
> >        abort ();
> >      }
> >
> > On Mon, Jul 11, 2022 at 6:51 PM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 7/11/22 11:55, Richard Biener wrote:
> >>> On Mon, Jul 11, 2022 at 11:16 AM Alexander Monakov <amonakov@ispras.ru> wrote:
> >>>>
> >>>> On Mon, 11 Jul 2022, Rui Ueyama wrote:
> >>>>
> >>>>>> but ignoring min_api_supported is wrong, and assuming max_api_supported > 0
> >>>>>> is also wrong. It really should check how given [min; max] range intersects
> >>>>>> with its own range of supported versions.
> >>>>>
> >>>>> Currently only one version is defined which is LAPI_V1. I don't think
> >>>>> LAPI_UNSPECIFIED is a version number; rather, it's an unspecified
> >>>>> value. No ordering should be defined between a defined value and an
> >>>>> unspecified value. If LAPI_UNSPECIFIED < LAPI_V1, it should be renamed
> >>>>> LAPI_V0.
> >>>>
> >>>> You still cannot rely on API guarantees of LAPI_V1 when the plugin does not
> >>>> advertise it (thread safety of claim_file in this particular case).
> >>>
> >>
> >> Hi.
> >>
> >> All right, I think we should rename LAPI_UNSPECIFIED to LAPI_V0 in order
> >> to support minimal_api_supported == LAPI_V0.
> >>
> >>> So with LAPI_UNSPECIFIED all the plugin gets is the linker name and version.
> >>> Clarifying the documentation on LAPI_UNSPECIFIED might be nice, also
> >>> what the expectation is on the linker when the plugin returns
> >>> LAPI_UNSPECIFIED when it speficied minimal_api_supported == V1.
> >>
> >> I've clarified that linker should return a value that is in range
> >> [minimal_api_supported, maximal_api_supported] and added an abort
> >> if it's not the case.
> >>
> >> Having that, mold should respect if maximal_api_supported == LAPI_V0 is returned
> >> by a plug-in (happens now as we miss locking for some targets).
> >>
> >> Martin
> >>
> >>> "minimal_api_supported == LAPI_UNSPECIFIED" does not make much
> >>> sense if using Ruis reading of this value?
> >>>
> >>>> And you still should check the intersection of supported API ranges
> >>>> and give a sane diagnostic when min_api_supported advertised by the plugin
> >>>> exceeds LAPI_V1 (though, granted, the plugin could error out as well in this
> >>>> case).
> >>>>
> >>>> Alexander
>
Richard Biener July 12, 2022, 6:28 a.m. UTC | #21 
On Mon, Jul 11, 2022 at 6:35 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Mon, 11 Jul 2022, Martin Liška wrote:
>
> > I've clarified that linker should return a value that is in range
> > [minimal_api_supported, maximal_api_supported] and added an abort
> > if it's not the case.
>
> I noticed that we are placing a trap for C++ consumers such as mold
> by passing min/max_api_supported as enum values. Unlike C, C++ disallows
> out-of-range enum values, so when mold does
>
> enum PluginLinkerAPIVersion {
>   LAPI_V0 = 0,
>   LAPI_V1,
> };
>
> get_api_version(const char *plugin_identifier,
>                 unsigned plugin_version,
>                 PluginLinkerAPIVersion minimal_api_supported,
>                 PluginLinkerAPIVersion maximal_api_supported,
>                 const char **linker_identifier,
>                 const char **linker_version) {
>
> checks such as 'min_api_supported > LAPI_V1' can be optimized out. Also,
> if a future tool passes LAPI_V2, it will trigger Clang's UBSan (GCC
> -fsanitize-undefined=enum instruments loads but not retrieval of function
> arguments).
>
> I'd suggest to fix this on both sides by changing the arguments to plain
> integer types.

That's a good point - the enum itself is then also somewhat redundant
if it just specifies symbolic names for 0, 1, ... but we can keep it for
documentation purposes.

>
> Alexander
Martin Liška July 12, 2022, 7:36 a.m. UTC | #22 
On 7/12/22 08:28, Richard Biener wrote:
> On Mon, Jul 11, 2022 at 6:35 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>>
>> On Mon, 11 Jul 2022, Martin Liška wrote:
>>
>>> I've clarified that linker should return a value that is in range
>>> [minimal_api_supported, maximal_api_supported] and added an abort
>>> if it's not the case.
>>
>> I noticed that we are placing a trap for C++ consumers such as mold
>> by passing min/max_api_supported as enum values. Unlike C, C++ disallows
>> out-of-range enum values, so when mold does
>>
>> enum PluginLinkerAPIVersion {
>>   LAPI_V0 = 0,	
>>   LAPI_V1,
>> };
>>
>> get_api_version(const char *plugin_identifier,
>>                 unsigned plugin_version,
>>                 PluginLinkerAPIVersion minimal_api_supported,
>>                 PluginLinkerAPIVersion maximal_api_supported,
>>                 const char **linker_identifier,
>>                 const char **linker_version) {
>>
>> checks such as 'min_api_supported > LAPI_V1' can be optimized out. Also,
>> if a future tool passes LAPI_V2, it will trigger Clang's UBSan (GCC
>> -fsanitize-undefined=enum instruments loads but not retrieval of function
>> arguments).
>>
>> I'd suggest to fix this on both sides by changing the arguments to plain
>> integer types.
> 
> That's a good point - the enum itself is then also somewhat redundant
> if it just specifies symbolic names for 0, 1, ... but we can keep it for
> documentation purposes.

All right, here we go with the integer types.

May I install the version now?

Thanks,
Martin

> 
>>
>> Alexander
Rui Ueyama July 12, 2022, 11:50 a.m. UTC | #23 
I'm fine, though I don't think I have a right to sign off.

On Tue, Jul 12, 2022 at 3:36 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 7/12/22 08:28, Richard Biener wrote:
> > On Mon, Jul 11, 2022 at 6:35 PM Alexander Monakov <amonakov@ispras.ru> wrote:
> >>
> >> On Mon, 11 Jul 2022, Martin Liška wrote:
> >>
> >>> I've clarified that linker should return a value that is in range
> >>> [minimal_api_supported, maximal_api_supported] and added an abort
> >>> if it's not the case.
> >>
> >> I noticed that we are placing a trap for C++ consumers such as mold
> >> by passing min/max_api_supported as enum values. Unlike C, C++ disallows
> >> out-of-range enum values, so when mold does
> >>
> >> enum PluginLinkerAPIVersion {
> >>   LAPI_V0 = 0,
> >>   LAPI_V1,
> >> };
> >>
> >> get_api_version(const char *plugin_identifier,
> >>                 unsigned plugin_version,
> >>                 PluginLinkerAPIVersion minimal_api_supported,
> >>                 PluginLinkerAPIVersion maximal_api_supported,
> >>                 const char **linker_identifier,
> >>                 const char **linker_version) {
> >>
> >> checks such as 'min_api_supported > LAPI_V1' can be optimized out. Also,
> >> if a future tool passes LAPI_V2, it will trigger Clang's UBSan (GCC
> >> -fsanitize-undefined=enum instruments loads but not retrieval of function
> >> arguments).
> >>
> >> I'd suggest to fix this on both sides by changing the arguments to plain
> >> integer types.
> >
> > That's a good point - the enum itself is then also somewhat redundant
> > if it just specifies symbolic names for 0, 1, ... but we can keep it for
> > documentation purposes.
>
> All right, here we go with the integer types.
>
> May I install the version now?
>
> Thanks,
> Martin
>
> >
> >>
> >> Alexander
Richard Biener July 12, 2022, 1:21 p.m. UTC | #24 
On Tue, Jul 12, 2022 at 1:50 PM Rui Ueyama <rui314@gmail.com> wrote:
>
> I'm fine, though I don't think I have a right to sign off.
>
> On Tue, Jul 12, 2022 at 3:36 PM Martin Liška <mliska@suse.cz> wrote:
> >
> > On 7/12/22 08:28, Richard Biener wrote:
> > > On Mon, Jul 11, 2022 at 6:35 PM Alexander Monakov <amonakov@ispras.ru> wrote:
> > >>
> > >> On Mon, 11 Jul 2022, Martin Liška wrote:
> > >>
> > >>> I've clarified that linker should return a value that is in range
> > >>> [minimal_api_supported, maximal_api_supported] and added an abort
> > >>> if it's not the case.
> > >>
> > >> I noticed that we are placing a trap for C++ consumers such as mold
> > >> by passing min/max_api_supported as enum values. Unlike C, C++ disallows
> > >> out-of-range enum values, so when mold does
> > >>
> > >> enum PluginLinkerAPIVersion {
> > >>   LAPI_V0 = 0,
> > >>   LAPI_V1,
> > >> };
> > >>
> > >> get_api_version(const char *plugin_identifier,
> > >>                 unsigned plugin_version,
> > >>                 PluginLinkerAPIVersion minimal_api_supported,
> > >>                 PluginLinkerAPIVersion maximal_api_supported,
> > >>                 const char **linker_identifier,
> > >>                 const char **linker_version) {
> > >>
> > >> checks such as 'min_api_supported > LAPI_V1' can be optimized out. Also,
> > >> if a future tool passes LAPI_V2, it will trigger Clang's UBSan (GCC
> > >> -fsanitize-undefined=enum instruments loads but not retrieval of function
> > >> arguments).
> > >>
> > >> I'd suggest to fix this on both sides by changing the arguments to plain
> > >> integer types.
> > >
> > > That's a good point - the enum itself is then also somewhat redundant
> > > if it just specifies symbolic names for 0, 1, ... but we can keep it for
> > > documentation purposes.
> >
> > All right, here we go with the integer types.
> >
> > May I install the version now?

OK.

Thanks,
Richard.

> > Thanks,
> > Martin
> >
> > >
> > >>
> > >> Alexander
Martin Liška July 12, 2022, 1:31 p.m. UTC | #25 
On 7/12/22 13:50, Rui Ueyama wrote:
> I'm fine, though I don't think I have a right to sign off.

I've just pushed that.

@Rui: Can you please merge the mold counter-part?

Thanks,
Martin
Rui Ueyama July 13, 2022, 7:44 a.m. UTC | #26 
On Tue, Jul 12, 2022 at 9:31 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 7/12/22 13:50, Rui Ueyama wrote:
> > I'm fine, though I don't think I have a right to sign off.
>
> I've just pushed that.
>
> @Rui: Can you please merge the mold counter-part?

I merged the mold counter-part as
https://github.com/rui314/mold/commit/b8a4df51ca574d411da2d51fbfdac9c9f10c76fb.
diff mbox series

Patch

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 8aebe2ff267..17b10180655 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -483,6 +483,34 @@  enum ld_plugin_level
   LDPL_FATAL
 };
 
+/* Contract between a plug-in and a linker.  */
+
+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, add_symbols_v2,
+      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
+};
+
+/* The linker's interface for API version negotiation.  A plug-in calls
+  the function (with its IDENTIFIER and VERSION), plus minimal and maximal
+  version of linker_api_version is provided.  Linker then returns selected
+  API version and provides its IDENTIFIER and VERSION.  */
+
+typedef
+enum linker_api_version
+(*ld_plugin_get_api_version) (const char *plugin_identifier, unsigned plugin_version,
+			      enum linker_api_version minimal_api_supported,
+			      enum linker_api_version maximal_api_supported,
+			      const char **linker_identifier,
+			      unsigned *linker_version);
+
 /* Values for the tv_tag field of the transfer vector.  */
 
 enum ld_plugin_tag
@@ -521,6 +549,7 @@  enum ld_plugin_tag
   LDPT_REGISTER_NEW_INPUT_HOOK,
   LDPT_GET_WRAP_SYMBOLS,
   LDPT_ADD_SYMBOLS_V2,
+  LDPT_GET_API_VERSION,
 };
 
 /* The plugin transfer vector.  */
@@ -556,6 +585,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_get_api_version tv_get_api_version;
   } tv_u;
 };
 
diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 13118c4983c..208b26d5c4b 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -70,6 +70,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.  */
@@ -170,6 +171,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_get_api_version get_api_version;
+
+/* By default, use version LAPI_UNSPECIFIED if there is not negotiation.  */
+static enum linker_api_version api_version = LAPI_UNSPECIFIED;
 
 static struct plugin_file_info *claimed_files = NULL;
 static unsigned int num_claimed_files = 0;
@@ -1415,6 +1420,33 @@  process_option (const char *option)
   verbose = verbose || debug;
 }
 
+/* Negotiate linker API version.  */
+
+static void
+negotiate_api_version (void)
+{
+  const char *linker_identifier;
+  unsigned linker_version;
+
+  api_version = get_api_version ("GCC", GCC_VERSION, LAPI_UNSPECIFIED,
+				 LAPI_V1, &linker_identifier, &linker_version);
+
+  switch (api_version)
+    {
+    case LAPI_UNSPECIFIED:
+      break;
+    case LAPI_V1:
+      check (get_symbols_v3, LDPL_FATAL,
+	     "get_symbols_v3 required for API version 1");
+      check (add_symbols_v2, LDPL_FATAL,
+	     "add_symbols_v2 required for API version 1");
+      break;
+    default:
+      fprintf (stderr, "unsupported API version\n");
+      abort ();
+    }
+}
+
 /* Called by a linker after loading the plugin. TV is the transfer vector. */
 
 enum ld_plugin_status
@@ -1481,12 +1513,18 @@  onload (struct ld_plugin_tv *tv)
 	  /* We only use this to make user-friendly temp file names.  */
 	  link_output_name = p->tv_u.tv_string;
 	  break;
+	case LDPT_GET_API_VERSION:
+	  get_api_version = p->tv_u.tv_get_api_version;
+	  break;
 	default:
 	  break;
 	}
       p++;
     }
 
+  if (get_api_version)
+    negotiate_api_version ();
+
   check (register_claim_file, LDPL_FATAL, "register_claim_file not found");
   check (add_symbols, LDPL_FATAL, "add_symbols not found");
   status = register_claim_file (claim_file_handler);