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