On Thu, Jun 16, 2022 at 2:25 PM Martin Liška <mli...@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
