Re: [PATCH 3/3] lto-plugin: implement LDPT_GET_API_VERSION

[Alexander Monakov <amonakov@ispras.ru>]

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