[PATCH] Support LDPT_GET_SYMBOLS_V3.

[PATCH] Support LDPT_GET_SYMBOLS_V3.

[Martin Liška]

That supports skipping of an object file (LDPS_NO_SYMS). The API will be used by mold
and is there for some time in linkers.

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

Ready to be installed?
Thanks,
Martin

lto-plugin/ChangeLog:

	* lto-plugin.c (struct plugin_file_info): Add skip_file flag.
	(write_resolution): Write resolution only if get_symbols != LDPS_NO_SYMS.
	(all_symbols_read_handler): Ignore file if skip_file is true.
	(onload): Handle LDPT_GET_SYMBOLS_V3.
---
 lto-plugin/lto-plugin.c | 42 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 33d49571d0e..aa5eab48b9d 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -136,6 +136,7 @@ struct plugin_file_info
   void *handle;
   struct plugin_symtab symtab;
   struct plugin_symtab conflicts;
+  bool skip_file;
 };
 
 /* List item with name of the file with offloading.  */
@@ -159,7 +160,7 @@ enum symbol_style
 static char *arguments_file_name;
 static ld_plugin_register_claim_file register_claim_file;
 static ld_plugin_register_all_symbols_read register_all_symbols_read;
-static ld_plugin_get_symbols get_symbols, get_symbols_v2;
+static ld_plugin_get_symbols get_symbols, get_symbols_v2, get_symbols_v3;
 static ld_plugin_register_cleanup register_cleanup;
 static ld_plugin_add_input_file add_input_file;
 static ld_plugin_add_input_library add_input_library;
@@ -547,15 +548,13 @@ free_symtab (struct plugin_symtab *symtab)
 static void
 write_resolution (void)
 {
-  unsigned int i;
+  unsigned int i, included_files = 0;
   FILE *f;
 
   check (resolution_file, LDPL_FATAL, "resolution file not specified");
   f = fopen (resolution_file, "w");
   check (f, LDPL_FATAL, "could not open file");
 
-  fprintf (f, "%d\n", num_claimed_files);
-
   for (i = 0; i < num_claimed_files; i++)
     {
       struct plugin_file_info *info = &claimed_files[i];
@@ -563,13 +562,38 @@ write_resolution (void)
       struct ld_plugin_symbol *syms = symtab->syms;
 
       /* Version 2 of API supports IRONLY_EXP resolution that is
-         accepted by GCC-4.7 and newer.  */
-      if (get_symbols_v2)
+	 accepted by GCC-4.7 and newer.
+	 Version 3 can return LDPS_NO_SYMS that means the object
+	 will not be used at all.  */
+      if (get_symbols_v3)
+	{
+	  enum ld_plugin_status status
+	    = get_symbols_v3 (info->handle, symtab->nsyms, syms);
+	  if (status == LDPS_NO_SYMS)
+	    {
+	      info->skip_file = true;
+	      continue;
+	    }
+	}
+      else if (get_symbols_v2)
         get_symbols_v2 (info->handle, symtab->nsyms, syms);
       else
         get_symbols (info->handle, symtab->nsyms, syms);
 
+      ++included_files;
+
       finish_conflict_resolution (symtab, &info->conflicts);
+    }
+
+  fprintf (f, "%d\n", included_files);
+
+  for (i = 0; i < num_claimed_files; i++)
+    {
+      struct plugin_file_info *info = &claimed_files[i];
+      struct plugin_symtab *symtab = &info->symtab;
+
+      if (info->skip_file)
+	continue;
 
       fprintf (f, "%s %d\n", info->name, symtab->nsyms + info->conflicts.nsyms);
       dump_symtab (f, symtab);
@@ -832,7 +856,8 @@ all_symbols_read_handler (void)
     {
       struct plugin_file_info *info = &claimed_files[i];
 
-      *lto_arg_ptr++ = info->name;
+      if (!info->skip_file)
+	*lto_arg_ptr++ = info->name;
     }
 
   *lto_arg_ptr++ = NULL;
@@ -1409,6 +1434,9 @@ onload (struct ld_plugin_tv *tv)
 	case LDPT_REGISTER_ALL_SYMBOLS_READ_HOOK:
 	  register_all_symbols_read = p->tv_u.tv_register_all_symbols_read;
 	  break;
+	case LDPT_GET_SYMBOLS_V3:
+	  get_symbols_v3 = p->tv_u.tv_get_symbols;
+	  break;
 	case LDPT_GET_SYMBOLS_V2:
 	  get_symbols_v2 = p->tv_u.tv_get_symbols;
 	  break;
-- 
2.36.0

[PATCH] lto-plugin: add support for feature detection

[Martin Liška]

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(+)

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;
-- 
2.36.0

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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;
> 

Re: [PATCH] lto-plugin: add support for feature detection

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

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;

[-- Attachment #2: 0001-lto-plugin-add-support-for-feature-detection.patch --]
[-- Type: text/x-patch, Size: 1219 bytes --]

From 7b5acd79a749e50bc825a49b283d4f9e82a5e1b5 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Tue, 1 Mar 2022 12:27:45 +0100
Subject: [PATCH] lto-plugin: add support for feature detection

lto-plugin/ChangeLog:

	* lto-plugin.c (supports_get_symbols_v3): Add symbol and
	initialize it to true;

lto-plugin/ChangeLog:

	* lto-plugin.map: Add supports_get_symbols_v3 symbol.
---
 lto-plugin/lto-plugin.c   | 5 +++++
 lto-plugin/lto-plugin.map | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 47378435612..6aac5867066 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;
 }
+
+/* Presence of the following symbols is used for dynamic detection of plug-in features
+   from linker side.  */
+
+bool supports_get_symbols_v3 = true;
diff --git a/lto-plugin/lto-plugin.map b/lto-plugin/lto-plugin.map
index 3d60e71cc2d..42dc50a819c 100644
--- a/lto-plugin/lto-plugin.map
+++ b/lto-plugin/lto-plugin.map
@@ -1,3 +1,6 @@
 {
-  global: onload; local: *;
+  global: onload;
+          supports_get_symbols_v3;
+
+  local: *;
 };
-- 
2.36.0

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Martin Liška]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Bernhard Reutner-Fischer]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Richard Biener]

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
>

Re: [PATCH] lto-plugin: add support for feature detection

[Richard Biener]

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
> >

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Martin Liška]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Martin Liška]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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.

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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.

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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?

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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?

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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.

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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/.

Re: [PATCH] lto-plugin: add support for feature detection

[Richard Biener]

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.

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Jan Hubicka]

> 
> 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.

Re: [PATCH] lto-plugin: add support for feature detection

[Martin Liška]

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.

Re: [PATCH] lto-plugin: add support for feature detection

[Jan Hubicka]

> 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.
> 

Re: [PATCH] lto-plugin: add support for feature detection

[Richard Biener]

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.
> >

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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.
>

Re: [PATCH] lto-plugin: add support for feature detection

[Richard Biener]

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.
> >

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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.
> > >

Re: [PATCH] lto-plugin: add support for feature detection

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 4536 bytes --]

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.
>>>

[-- Attachment #2: 0001-Implement-LDPT_REGISTER_GET_API_VERSION.patch --]
[-- Type: text/x-patch, Size: 4053 bytes --]

From 0ef5f678144b8ff3a1d247a992aa2e14128b82d1 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:01:52 +0200
Subject: [PATCH] Implement LDPT_REGISTER_GET_API_VERSION.

---
 include/plugin-api.h    | 14 ++++++++++++++
 lto-plugin/lto-plugin.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 8aebe2ff267..f01f4301fc9 100644
--- 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;
 };
 
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,
+		 const char **compiler_identifier,
+		 int *compiler_version)
+{
+  *compiler_identifier = "GCC";
+  *compiler_version = GCC_VERSION;
+
+  if (preferred_linker_api >= 2)
+    {
+      check (get_symbols_v3, LDPL_FATAL,
+	     "get_symbols_v3 requires for API version 2");
+      check (add_symbols_v2, LDPL_FATAL,
+	     "add_symbols_v2 requires for API version 2");
+
+      /* The plug-in supports version 2.  */
+      used_api_version = 2;
+    }
+
+  return used_api_version;
+}
+
 /* Called by a linker after loading the plugin. TV is the transfer vector. */
 
 enum ld_plugin_status
@@ -1467,12 +1495,22 @@ 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_REGISTER_GET_API_VERSION:
+	  register_get_api_version = p->tv_u.tv_register_get_api_version;
+	  break;
 	default:
 	  break;
 	}
       p++;
     }
 
+  if (register_get_api_version)
+    {
+      status = register_get_api_version (get_api_version);
+      check (status == LDPS_OK, LDPL_FATAL,
+	     "could not register the get_api_version callback");
+    }
+
   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);
-- 
2.36.1

Re: [PATCH] lto-plugin: add support for feature detection

[Rui Ueyama]

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.
> >>>

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Alexander Monakov]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Richard Biener]

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

Re: [PATCH] lto-plugin: add support for feature detection

[Martin Liška]

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

[PATCH 1/3] lto-plugin: support LDPT_GET_SYMBOLS_V3

[Martin Liška]

That supports skipping of an object file (LDPS_NO_SYMS).

lto-plugin/ChangeLog:

	* lto-plugin.c (struct plugin_file_info): Add skip_file flag.
	(write_resolution): Write resolution only if get_symbols != LDPS_NO_SYMS.
	(all_symbols_read_handler): Ignore file if skip_file is true.
	(onload): Handle LDPT_GET_SYMBOLS_V3.
---
 lto-plugin/lto-plugin.c | 42 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 47378435612..00b760636dc 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -136,6 +136,7 @@ struct plugin_file_info
   void *handle;
   struct plugin_symtab symtab;
   struct plugin_symtab conflicts;
+  bool skip_file;
 };
 
 /* List item with name of the file with offloading.  */
@@ -159,7 +160,7 @@ enum symbol_style
 static char *arguments_file_name;
 static ld_plugin_register_claim_file register_claim_file;
 static ld_plugin_register_all_symbols_read register_all_symbols_read;
-static ld_plugin_get_symbols get_symbols, get_symbols_v2;
+static ld_plugin_get_symbols get_symbols, get_symbols_v2, get_symbols_v3;
 static ld_plugin_register_cleanup register_cleanup;
 static ld_plugin_add_input_file add_input_file;
 static ld_plugin_add_input_library add_input_library;
@@ -547,15 +548,13 @@ free_symtab (struct plugin_symtab *symtab)
 static void
 write_resolution (void)
 {
-  unsigned int i;
+  unsigned int i, included_files = 0;
   FILE *f;
 
   check (resolution_file, LDPL_FATAL, "resolution file not specified");
   f = fopen (resolution_file, "w");
   check (f, LDPL_FATAL, "could not open file");
 
-  fprintf (f, "%d\n", num_claimed_files);
-
   for (i = 0; i < num_claimed_files; i++)
     {
       struct plugin_file_info *info = &claimed_files[i];
@@ -563,13 +562,38 @@ write_resolution (void)
       struct ld_plugin_symbol *syms = symtab->syms;
 
       /* Version 2 of API supports IRONLY_EXP resolution that is
-         accepted by GCC-4.7 and newer.  */
-      if (get_symbols_v2)
+	 accepted by GCC-4.7 and newer.
+	 Version 3 can return LDPS_NO_SYMS that means the object
+	 will not be used at all.  */
+      if (get_symbols_v3)
+	{
+	  enum ld_plugin_status status
+	    = get_symbols_v3 (info->handle, symtab->nsyms, syms);
+	  if (status == LDPS_NO_SYMS)
+	    {
+	      info->skip_file = true;
+	      continue;
+	    }
+	}
+      else if (get_symbols_v2)
         get_symbols_v2 (info->handle, symtab->nsyms, syms);
       else
         get_symbols (info->handle, symtab->nsyms, syms);
 
+      ++included_files;
+
       finish_conflict_resolution (symtab, &info->conflicts);
+    }
+
+  fprintf (f, "%d\n", included_files);
+
+  for (i = 0; i < num_claimed_files; i++)
+    {
+      struct plugin_file_info *info = &claimed_files[i];
+      struct plugin_symtab *symtab = &info->symtab;
+
+      if (info->skip_file)
+	continue;
 
       fprintf (f, "%s %d\n", info->name, symtab->nsyms + info->conflicts.nsyms);
       dump_symtab (f, symtab);
@@ -833,7 +857,8 @@ all_symbols_read_handler (void)
     {
       struct plugin_file_info *info = &claimed_files[i];
 
-      *lto_arg_ptr++ = info->name;
+      if (!info->skip_file)
+	*lto_arg_ptr++ = info->name;
     }
 
   *lto_arg_ptr++ = NULL;
@@ -1410,6 +1435,9 @@ onload (struct ld_plugin_tv *tv)
 	case LDPT_REGISTER_ALL_SYMBOLS_READ_HOOK:
 	  register_all_symbols_read = p->tv_u.tv_register_all_symbols_read;
 	  break;
+	case LDPT_GET_SYMBOLS_V3:
+	  get_symbols_v3 = p->tv_u.tv_get_symbols;
+	  break;
 	case LDPT_GET_SYMBOLS_V2:
 	  get_symbols_v2 = p->tv_u.tv_get_symbols;
 	  break;
-- 
2.36.1

Re: [PATCH 1/3] lto-plugin: support LDPT_GET_SYMBOLS_V3

[Richard Biener]

On Thu, Jun 16, 2022 at 9:00 AM Martin Liška <mliska@suse.cz> wrote:
>
> That supports skipping of an object file (LDPS_NO_SYMS).

OK.

Thanks,
Richard.

> lto-plugin/ChangeLog:
>
>         * lto-plugin.c (struct plugin_file_info): Add skip_file flag.
>         (write_resolution): Write resolution only if get_symbols != LDPS_NO_SYMS.
>         (all_symbols_read_handler): Ignore file if skip_file is true.
>         (onload): Handle LDPT_GET_SYMBOLS_V3.
> ---
>  lto-plugin/lto-plugin.c | 42 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> index 47378435612..00b760636dc 100644
> --- a/lto-plugin/lto-plugin.c
> +++ b/lto-plugin/lto-plugin.c
> @@ -136,6 +136,7 @@ struct plugin_file_info
>    void *handle;
>    struct plugin_symtab symtab;
>    struct plugin_symtab conflicts;
> +  bool skip_file;
>  };
>
>  /* List item with name of the file with offloading.  */
> @@ -159,7 +160,7 @@ enum symbol_style
>  static char *arguments_file_name;
>  static ld_plugin_register_claim_file register_claim_file;
>  static ld_plugin_register_all_symbols_read register_all_symbols_read;
> -static ld_plugin_get_symbols get_symbols, get_symbols_v2;
> +static ld_plugin_get_symbols get_symbols, get_symbols_v2, get_symbols_v3;
>  static ld_plugin_register_cleanup register_cleanup;
>  static ld_plugin_add_input_file add_input_file;
>  static ld_plugin_add_input_library add_input_library;
> @@ -547,15 +548,13 @@ free_symtab (struct plugin_symtab *symtab)
>  static void
>  write_resolution (void)
>  {
> -  unsigned int i;
> +  unsigned int i, included_files = 0;
>    FILE *f;
>
>    check (resolution_file, LDPL_FATAL, "resolution file not specified");
>    f = fopen (resolution_file, "w");
>    check (f, LDPL_FATAL, "could not open file");
>
> -  fprintf (f, "%d\n", num_claimed_files);
> -
>    for (i = 0; i < num_claimed_files; i++)
>      {
>        struct plugin_file_info *info = &claimed_files[i];
> @@ -563,13 +562,38 @@ write_resolution (void)
>        struct ld_plugin_symbol *syms = symtab->syms;
>
>        /* Version 2 of API supports IRONLY_EXP resolution that is
> -         accepted by GCC-4.7 and newer.  */
> -      if (get_symbols_v2)
> +        accepted by GCC-4.7 and newer.
> +        Version 3 can return LDPS_NO_SYMS that means the object
> +        will not be used at all.  */
> +      if (get_symbols_v3)
> +       {
> +         enum ld_plugin_status status
> +           = get_symbols_v3 (info->handle, symtab->nsyms, syms);
> +         if (status == LDPS_NO_SYMS)
> +           {
> +             info->skip_file = true;
> +             continue;
> +           }
> +       }
> +      else if (get_symbols_v2)
>          get_symbols_v2 (info->handle, symtab->nsyms, syms);
>        else
>          get_symbols (info->handle, symtab->nsyms, syms);
>
> +      ++included_files;
> +
>        finish_conflict_resolution (symtab, &info->conflicts);
> +    }
> +
> +  fprintf (f, "%d\n", included_files);
> +
> +  for (i = 0; i < num_claimed_files; i++)
> +    {
> +      struct plugin_file_info *info = &claimed_files[i];
> +      struct plugin_symtab *symtab = &info->symtab;
> +
> +      if (info->skip_file)
> +       continue;
>
>        fprintf (f, "%s %d\n", info->name, symtab->nsyms + info->conflicts.nsyms);
>        dump_symtab (f, symtab);
> @@ -833,7 +857,8 @@ all_symbols_read_handler (void)
>      {
>        struct plugin_file_info *info = &claimed_files[i];
>
> -      *lto_arg_ptr++ = info->name;
> +      if (!info->skip_file)
> +       *lto_arg_ptr++ = info->name;
>      }
>
>    *lto_arg_ptr++ = NULL;
> @@ -1410,6 +1435,9 @@ onload (struct ld_plugin_tv *tv)
>         case LDPT_REGISTER_ALL_SYMBOLS_READ_HOOK:
>           register_all_symbols_read = p->tv_u.tv_register_all_symbols_read;
>           break;
> +       case LDPT_GET_SYMBOLS_V3:
> +         get_symbols_v3 = p->tv_u.tv_get_symbols;
> +         break;
>         case LDPT_GET_SYMBOLS_V2:
>           get_symbols_v2 = p->tv_u.tv_get_symbols;
>           break;
> --
> 2.36.1
>
>

[PATCH 2/3] lto-plugin: make claim_file_handler thread-safe

[Martin Liška]

lto-plugin/ChangeLog:

	* lto-plugin.c (plugin_lock): New lock.
	(claim_file_handler): Use mutex for critical section.
	(onload): Initialize mutex.
---
 lto-plugin/lto-plugin.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 00b760636dc..13118c4983c 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -55,6 +55,7 @@ along with this program; see the file COPYING3.  If not see
 #include <unistd.h>
 #include <fcntl.h>
 #include <sys/types.h>
+#include <pthread.h>
 #ifdef HAVE_SYS_WAIT_H
 #include <sys/wait.h>
 #endif
@@ -157,6 +158,9 @@ enum symbol_style
   ss_uscore,	/* Underscore prefix all symbols.  */
 };
 
+/* Plug-in mutex.  */
+static pthread_mutex_t plugin_lock;
+
 static char *arguments_file_name;
 static ld_plugin_register_claim_file register_claim_file;
 static ld_plugin_register_all_symbols_read register_all_symbols_read;
@@ -1262,15 +1266,18 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
 			      lto_file.symtab.syms);
       check (status == LDPS_OK, LDPL_FATAL, "could not add symbols");
 
+      pthread_mutex_lock (&plugin_lock);
       num_claimed_files++;
       claimed_files =
 	xrealloc (claimed_files,
 		  num_claimed_files * sizeof (struct plugin_file_info));
       claimed_files[num_claimed_files - 1] = lto_file;
+      pthread_mutex_unlock (&plugin_lock);
 
       *claimed = 1;
     }
 
+  pthread_mutex_lock (&plugin_lock);
   if (offload_files == NULL)
     {
       /* Add dummy item to the start of the list.  */
@@ -1333,11 +1340,12 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
 	offload_files_last_lto = ofld;
       num_offload_files++;
     }
+  pthread_mutex_unlock (&plugin_lock);
 
   goto cleanup;
 
  err:
-  non_claimed_files++;
+  __atomic_fetch_add (&non_claimed_files, 1, __ATOMIC_RELAXED);
   free (lto_file.name);
 
  cleanup:
@@ -1415,6 +1423,12 @@ onload (struct ld_plugin_tv *tv)
   struct ld_plugin_tv *p;
   enum ld_plugin_status status;
 
+  if (pthread_mutex_init (&plugin_lock, NULL) != 0)
+    {
+      fprintf (stderr, "mutex init failed\n");
+      abort ();
+    }
+
   p = tv;
   while (p->tv_tag)
     {
-- 
2.36.1

Re: [PATCH 2/3] lto-plugin: make claim_file_handler thread-safe

[Richard Biener]

On Thu, Jun 16, 2022 at 9:01 AM Martin Liška <mliska@suse.cz> wrote:
>
> lto-plugin/ChangeLog:
>
>         * lto-plugin.c (plugin_lock): New lock.
>         (claim_file_handler): Use mutex for critical section.
>         (onload): Initialize mutex.
> ---
>  lto-plugin/lto-plugin.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> index 00b760636dc..13118c4983c 100644
> --- a/lto-plugin/lto-plugin.c
> +++ b/lto-plugin/lto-plugin.c
> @@ -55,6 +55,7 @@ along with this program; see the file COPYING3.  If not see
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <sys/types.h>
> +#include <pthread.h>

Not sure if we support any non-pthread target for building the LTO
plugin, but it
seems we have

  # Among non-ELF, only Windows platforms support the lto-plugin so far.
  # Build it unless LTO was explicitly disabled.
  case $target in
    *-cygwin* | *-mingw*) build_lto_plugin=$enable_lto ;;

which suggests that at least build validating the above with --enable-lto

IIRC we have gthr-*.h in libgcc/, not sure if that's usable in a
host linker plugin.

>  #ifdef HAVE_SYS_WAIT_H
>  #include <sys/wait.h>
>  #endif
> @@ -157,6 +158,9 @@ enum symbol_style
>    ss_uscore,   /* Underscore prefix all symbols.  */
>  };
>
> +/* Plug-in mutex.  */
> +static pthread_mutex_t plugin_lock;
> +
>  static char *arguments_file_name;
>  static ld_plugin_register_claim_file register_claim_file;
>  static ld_plugin_register_all_symbols_read register_all_symbols_read;
> @@ -1262,15 +1266,18 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>                               lto_file.symtab.syms);
>        check (status == LDPS_OK, LDPL_FATAL, "could not add symbols");
>
> +      pthread_mutex_lock (&plugin_lock);
>        num_claimed_files++;
>        claimed_files =
>         xrealloc (claimed_files,
>                   num_claimed_files * sizeof (struct plugin_file_info));
>        claimed_files[num_claimed_files - 1] = lto_file;
> +      pthread_mutex_unlock (&plugin_lock);
>
>        *claimed = 1;
>      }
>
> +  pthread_mutex_lock (&plugin_lock);
>    if (offload_files == NULL)
>      {
>        /* Add dummy item to the start of the list.  */
> @@ -1333,11 +1340,12 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>         offload_files_last_lto = ofld;
>        num_offload_files++;
>      }
> +  pthread_mutex_unlock (&plugin_lock);
>
>    goto cleanup;
>
>   err:
> -  non_claimed_files++;
> +  __atomic_fetch_add (&non_claimed_files, 1, __ATOMIC_RELAXED);

is it worth "optimizing" this with yet another need for target specific support
(just use pthread_mutex here as well?)

>    free (lto_file.name);
>
>   cleanup:
> @@ -1415,6 +1423,12 @@ onload (struct ld_plugin_tv *tv)
>    struct ld_plugin_tv *p;
>    enum ld_plugin_status status;
>
> +  if (pthread_mutex_init (&plugin_lock, NULL) != 0)
> +    {
> +      fprintf (stderr, "mutex init failed\n");
> +      abort ();
> +    }
> +
>    p = tv;
>    while (p->tv_tag)
>      {
> --
> 2.36.1
>
>

Re: [PATCH 2/3] lto-plugin: make claim_file_handler thread-safe

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 3381 bytes --]

On 6/20/22 11:32, Richard Biener wrote:
> On Thu, Jun 16, 2022 at 9:01 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> lto-plugin/ChangeLog:
>>
>>         * lto-plugin.c (plugin_lock): New lock.
>>         (claim_file_handler): Use mutex for critical section.
>>         (onload): Initialize mutex.
>> ---
>>  lto-plugin/lto-plugin.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
>> index 00b760636dc..13118c4983c 100644
>> --- a/lto-plugin/lto-plugin.c
>> +++ b/lto-plugin/lto-plugin.c
>> @@ -55,6 +55,7 @@ along with this program; see the file COPYING3.  If not see
>>  #include <unistd.h>
>>  #include <fcntl.h>
>>  #include <sys/types.h>
>> +#include <pthread.h>
> 
> Not sure if we support any non-pthread target for building the LTO
> plugin, but it
> seems we have
> 
>   # Among non-ELF, only Windows platforms support the lto-plugin so far.
>   # Build it unless LTO was explicitly disabled.
>   case $target in
>     *-cygwin* | *-mingw*) build_lto_plugin=$enable_lto ;;
> 
> which suggests that at least build validating the above with --enable-lto

Verified that it's fine.

> 
> IIRC we have gthr-*.h in libgcc/, not sure if that's usable in a
> host linker plugin.
> 
>>  #ifdef HAVE_SYS_WAIT_H
>>  #include <sys/wait.h>
>>  #endif
>> @@ -157,6 +158,9 @@ enum symbol_style
>>    ss_uscore,   /* Underscore prefix all symbols.  */
>>  };
>>
>> +/* Plug-in mutex.  */
>> +static pthread_mutex_t plugin_lock;
>> +
>>  static char *arguments_file_name;
>>  static ld_plugin_register_claim_file register_claim_file;
>>  static ld_plugin_register_all_symbols_read register_all_symbols_read;
>> @@ -1262,15 +1266,18 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>>                               lto_file.symtab.syms);
>>        check (status == LDPS_OK, LDPL_FATAL, "could not add symbols");
>>
>> +      pthread_mutex_lock (&plugin_lock);
>>        num_claimed_files++;
>>        claimed_files =
>>         xrealloc (claimed_files,
>>                   num_claimed_files * sizeof (struct plugin_file_info));
>>        claimed_files[num_claimed_files - 1] = lto_file;
>> +      pthread_mutex_unlock (&plugin_lock);
>>
>>        *claimed = 1;
>>      }
>>
>> +  pthread_mutex_lock (&plugin_lock);
>>    if (offload_files == NULL)
>>      {
>>        /* Add dummy item to the start of the list.  */
>> @@ -1333,11 +1340,12 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>>         offload_files_last_lto = ofld;
>>        num_offload_files++;
>>      }
>> +  pthread_mutex_unlock (&plugin_lock);
>>
>>    goto cleanup;
>>
>>   err:
>> -  non_claimed_files++;
>> +  __atomic_fetch_add (&non_claimed_files, 1, __ATOMIC_RELAXED);
> 
> is it worth "optimizing" this with yet another need for target specific support
> (just use pthread_mutex here as well?)

Sure.

May I install the patch with the change?

Cheers,
Martin

> 
>>    free (lto_file.name);
>>
>>   cleanup:
>> @@ -1415,6 +1423,12 @@ onload (struct ld_plugin_tv *tv)
>>    struct ld_plugin_tv *p;
>>    enum ld_plugin_status status;
>>
>> +  if (pthread_mutex_init (&plugin_lock, NULL) != 0)
>> +    {
>> +      fprintf (stderr, "mutex init failed\n");
>> +      abort ();
>> +    }
>> +
>>    p = tv;
>>    while (p->tv_tag)
>>      {
>> --
>> 2.36.1
>>
>>

[-- Attachment #2: 0001-lto-plugin-make-claim_file_handler-thread-safe.patch --]
[-- Type: text/x-patch, Size: 2478 bytes --]

From 12fb5f8fbb283313f9d5bcadb24c45904128804d Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:18:41 +0200
Subject: [PATCH 1/2] lto-plugin: make claim_file_handler thread-safe

lto-plugin/ChangeLog:

	* lto-plugin.c (plugin_lock): New lock.
	(claim_file_handler): Use mutex for critical section.
	(onload): Initialize mutex.
---
 lto-plugin/lto-plugin.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 00b760636dc..2d95b7b3803 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -55,6 +55,7 @@ along with this program; see the file COPYING3.  If not see
 #include <unistd.h>
 #include <fcntl.h>
 #include <sys/types.h>
+#include <pthread.h>
 #ifdef HAVE_SYS_WAIT_H
 #include <sys/wait.h>
 #endif
@@ -157,6 +158,9 @@ enum symbol_style
   ss_uscore,	/* Underscore prefix all symbols.  */
 };
 
+/* Plug-in mutex.  */
+static pthread_mutex_t plugin_lock;
+
 static char *arguments_file_name;
 static ld_plugin_register_claim_file register_claim_file;
 static ld_plugin_register_all_symbols_read register_all_symbols_read;
@@ -1262,15 +1266,18 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
 			      lto_file.symtab.syms);
       check (status == LDPS_OK, LDPL_FATAL, "could not add symbols");
 
+      pthread_mutex_lock (&plugin_lock);
       num_claimed_files++;
       claimed_files =
 	xrealloc (claimed_files,
 		  num_claimed_files * sizeof (struct plugin_file_info));
       claimed_files[num_claimed_files - 1] = lto_file;
+      pthread_mutex_unlock (&plugin_lock);
 
       *claimed = 1;
     }
 
+  pthread_mutex_lock (&plugin_lock);
   if (offload_files == NULL)
     {
       /* Add dummy item to the start of the list.  */
@@ -1333,11 +1340,14 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
 	offload_files_last_lto = ofld;
       num_offload_files++;
     }
+  pthread_mutex_unlock (&plugin_lock);
 
   goto cleanup;
 
  err:
+  pthread_mutex_lock (&plugin_lock);
   non_claimed_files++;
+  pthread_mutex_unlock (&plugin_lock);
   free (lto_file.name);
 
  cleanup:
@@ -1415,6 +1425,12 @@ onload (struct ld_plugin_tv *tv)
   struct ld_plugin_tv *p;
   enum ld_plugin_status status;
 
+  if (pthread_mutex_init (&plugin_lock, NULL) != 0)
+    {
+      fprintf (stderr, "mutex init failed\n");
+      abort ();
+    }
+
   p = tv;
   while (p->tv_tag)
     {
-- 
2.36.1

Re: [PATCH 2/3] lto-plugin: make claim_file_handler thread-safe

[Richard Biener]

On Mon, Jun 20, 2022 at 12:20 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 6/20/22 11:32, Richard Biener wrote:
> > On Thu, Jun 16, 2022 at 9:01 AM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> lto-plugin/ChangeLog:
> >>
> >>         * lto-plugin.c (plugin_lock): New lock.
> >>         (claim_file_handler): Use mutex for critical section.
> >>         (onload): Initialize mutex.
> >> ---
> >>  lto-plugin/lto-plugin.c | 16 +++++++++++++++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> >> index 00b760636dc..13118c4983c 100644
> >> --- a/lto-plugin/lto-plugin.c
> >> +++ b/lto-plugin/lto-plugin.c
> >> @@ -55,6 +55,7 @@ along with this program; see the file COPYING3.  If not see
> >>  #include <unistd.h>
> >>  #include <fcntl.h>
> >>  #include <sys/types.h>
> >> +#include <pthread.h>
> >
> > Not sure if we support any non-pthread target for building the LTO
> > plugin, but it
> > seems we have
> >
> >   # Among non-ELF, only Windows platforms support the lto-plugin so far.
> >   # Build it unless LTO was explicitly disabled.
> >   case $target in
> >     *-cygwin* | *-mingw*) build_lto_plugin=$enable_lto ;;
> >
> > which suggests that at least build validating the above with --enable-lto
>
> Verified that it's fine.
>
> >
> > IIRC we have gthr-*.h in libgcc/, not sure if that's usable in a
> > host linker plugin.
> >
> >>  #ifdef HAVE_SYS_WAIT_H
> >>  #include <sys/wait.h>
> >>  #endif
> >> @@ -157,6 +158,9 @@ enum symbol_style
> >>    ss_uscore,   /* Underscore prefix all symbols.  */
> >>  };
> >>
> >> +/* Plug-in mutex.  */
> >> +static pthread_mutex_t plugin_lock;
> >> +
> >>  static char *arguments_file_name;
> >>  static ld_plugin_register_claim_file register_claim_file;
> >>  static ld_plugin_register_all_symbols_read register_all_symbols_read;
> >> @@ -1262,15 +1266,18 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
> >>                               lto_file.symtab.syms);
> >>        check (status == LDPS_OK, LDPL_FATAL, "could not add symbols");
> >>
> >> +      pthread_mutex_lock (&plugin_lock);
> >>        num_claimed_files++;
> >>        claimed_files =
> >>         xrealloc (claimed_files,
> >>                   num_claimed_files * sizeof (struct plugin_file_info));
> >>        claimed_files[num_claimed_files - 1] = lto_file;
> >> +      pthread_mutex_unlock (&plugin_lock);
> >>
> >>        *claimed = 1;
> >>      }
> >>
> >> +  pthread_mutex_lock (&plugin_lock);
> >>    if (offload_files == NULL)
> >>      {
> >>        /* Add dummy item to the start of the list.  */
> >> @@ -1333,11 +1340,12 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
> >>         offload_files_last_lto = ofld;
> >>        num_offload_files++;
> >>      }
> >> +  pthread_mutex_unlock (&plugin_lock);
> >>
> >>    goto cleanup;
> >>
> >>   err:
> >> -  non_claimed_files++;
> >> +  __atomic_fetch_add (&non_claimed_files, 1, __ATOMIC_RELAXED);
> >
> > is it worth "optimizing" this with yet another need for target specific support
> > (just use pthread_mutex here as well?)
>
> Sure.
>
> May I install the patch with the change?

Can you at least add a configure check for pthread.h and maybe disable
locking when not found or erroring out?  I figure we have GCC_AC_THREAD_HEADER
for the gthr.h stuff using $target_thread_file (aka --enable-threads=XYZ),
but as said that's for the target and I don't see any host uses.  We might also
add an explicit list of hosts (*-linux*?) where we enable thread support for
lto-plugin, providing opt-in (so you'd have to wrap the mutex taking or
if-def it out).

I think you also need to link lto-plugin with -pthread, no?  On linux
it might work omitting that but I'm not sure other libc have serial pthread
stubs in their libc.  BFD ld definitely doesn't link against pthread so
dlopening lto-plugin will fail (also not all libc might like
initializing threads
from a dlopen _init).

Richard.

> Cheers,
> Martin
>
> >
> >>    free (lto_file.name);
> >>
> >>   cleanup:
> >> @@ -1415,6 +1423,12 @@ onload (struct ld_plugin_tv *tv)
> >>    struct ld_plugin_tv *p;
> >>    enum ld_plugin_status status;
> >>
> >> +  if (pthread_mutex_init (&plugin_lock, NULL) != 0)
> >> +    {
> >> +      fprintf (stderr, "mutex init failed\n");
> >> +      abort ();
> >> +    }
> >> +
> >>    p = tv;
> >>    while (p->tv_tag)
> >>      {
> >> --
> >> 2.36.1
> >>
> >>

Re: [PATCH 2/3] lto-plugin: make claim_file_handler thread-safe

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 4685 bytes --]

On 6/21/22 09:56, Richard Biener wrote:
> On Mon, Jun 20, 2022 at 12:20 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 6/20/22 11:32, Richard Biener wrote:
>>> On Thu, Jun 16, 2022 at 9:01 AM Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> lto-plugin/ChangeLog:
>>>>
>>>>         * lto-plugin.c (plugin_lock): New lock.
>>>>         (claim_file_handler): Use mutex for critical section.
>>>>         (onload): Initialize mutex.
>>>> ---
>>>>  lto-plugin/lto-plugin.c | 16 +++++++++++++++-
>>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
>>>> index 00b760636dc..13118c4983c 100644
>>>> --- a/lto-plugin/lto-plugin.c
>>>> +++ b/lto-plugin/lto-plugin.c
>>>> @@ -55,6 +55,7 @@ along with this program; see the file COPYING3.  If not see
>>>>  #include <unistd.h>
>>>>  #include <fcntl.h>
>>>>  #include <sys/types.h>
>>>> +#include <pthread.h>
>>>
>>> Not sure if we support any non-pthread target for building the LTO
>>> plugin, but it
>>> seems we have
>>>
>>>   # Among non-ELF, only Windows platforms support the lto-plugin so far.
>>>   # Build it unless LTO was explicitly disabled.
>>>   case $target in
>>>     *-cygwin* | *-mingw*) build_lto_plugin=$enable_lto ;;
>>>
>>> which suggests that at least build validating the above with --enable-lto
>>
>> Verified that it's fine.
>>
>>>
>>> IIRC we have gthr-*.h in libgcc/, not sure if that's usable in a
>>> host linker plugin.
>>>
>>>>  #ifdef HAVE_SYS_WAIT_H
>>>>  #include <sys/wait.h>
>>>>  #endif
>>>> @@ -157,6 +158,9 @@ enum symbol_style
>>>>    ss_uscore,   /* Underscore prefix all symbols.  */
>>>>  };
>>>>
>>>> +/* Plug-in mutex.  */
>>>> +static pthread_mutex_t plugin_lock;
>>>> +
>>>>  static char *arguments_file_name;
>>>>  static ld_plugin_register_claim_file register_claim_file;
>>>>  static ld_plugin_register_all_symbols_read register_all_symbols_read;
>>>> @@ -1262,15 +1266,18 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>>>>                               lto_file.symtab.syms);
>>>>        check (status == LDPS_OK, LDPL_FATAL, "could not add symbols");
>>>>
>>>> +      pthread_mutex_lock (&plugin_lock);
>>>>        num_claimed_files++;
>>>>        claimed_files =
>>>>         xrealloc (claimed_files,
>>>>                   num_claimed_files * sizeof (struct plugin_file_info));
>>>>        claimed_files[num_claimed_files - 1] = lto_file;
>>>> +      pthread_mutex_unlock (&plugin_lock);
>>>>
>>>>        *claimed = 1;
>>>>      }
>>>>
>>>> +  pthread_mutex_lock (&plugin_lock);
>>>>    if (offload_files == NULL)
>>>>      {
>>>>        /* Add dummy item to the start of the list.  */
>>>> @@ -1333,11 +1340,12 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>>>>         offload_files_last_lto = ofld;
>>>>        num_offload_files++;
>>>>      }
>>>> +  pthread_mutex_unlock (&plugin_lock);
>>>>
>>>>    goto cleanup;
>>>>
>>>>   err:
>>>> -  non_claimed_files++;
>>>> +  __atomic_fetch_add (&non_claimed_files, 1, __ATOMIC_RELAXED);
>>>
>>> is it worth "optimizing" this with yet another need for target specific support
>>> (just use pthread_mutex here as well?)
>>
>> Sure.
>>
>> May I install the patch with the change?
> 
> Can you at least add a configure check for pthread.h and maybe disable
> locking when not found or erroring out?  I figure we have GCC_AC_THREAD_HEADER

All right, let's error out then.

> for the gthr.h stuff using $target_thread_file (aka --enable-threads=XYZ),
> but as said that's for the target and I don't see any host uses.  We might also
> add an explicit list of hosts (*-linux*?) where we enable thread support for
> lto-plugin, providing opt-in (so you'd have to wrap the mutex taking or
> if-def it out).
> 
> I think you also need to link lto-plugin with -pthread, no?

Yep.

Please see the updated patch.

> On linux
> it might work omitting that but I'm not sure other libc have serial pthread
> stubs in their libc.  BFD ld definitely doesn't link against pthread so
> dlopening lto-plugin will fail (also not all libc might like
> initializing threads
> from a dlopen _init).

What initializing threads do you mean?

Martin

> 
> Richard.
> 
>> Cheers,
>> Martin
>>
>>>
>>>>    free (lto_file.name);
>>>>
>>>>   cleanup:
>>>> @@ -1415,6 +1423,12 @@ onload (struct ld_plugin_tv *tv)
>>>>    struct ld_plugin_tv *p;
>>>>    enum ld_plugin_status status;
>>>>
>>>> +  if (pthread_mutex_init (&plugin_lock, NULL) != 0)
>>>> +    {
>>>> +      fprintf (stderr, "mutex init failed\n");
>>>> +      abort ();
>>>> +    }
>>>> +
>>>>    p = tv;
>>>>    while (p->tv_tag)
>>>>      {
>>>> --
>>>> 2.36.1
>>>>
>>>>

[-- Attachment #2: 0001-lto-plugin-make-claim_file_handler-thread-safe.patch --]
[-- Type: text/x-patch, Size: 5476 bytes --]

From f1e2f84dbfdac5c7aee7036e78841cb33c3bad50 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:18:41 +0200
Subject: [PATCH] lto-plugin: make claim_file_handler thread-safe

lto-plugin/ChangeLog:

	* lto-plugin.c (plugin_lock): New lock.
	(claim_file_handler): Use mutex for critical section.
	(onload): Initialize mutex.
---
 lto-plugin/config.h.in  |  3 +++
 lto-plugin/configure    | 17 +++++++++++++++--
 lto-plugin/configure.ac |  7 +++++++
 lto-plugin/lto-plugin.c | 20 ++++++++++++++++++++
 4 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/lto-plugin/config.h.in b/lto-plugin/config.h.in
index ddbde7619d5..029e782f1ee 100644
--- a/lto-plugin/config.h.in
+++ b/lto-plugin/config.h.in
@@ -9,6 +9,9 @@
 /* Define to 1 if you have the <memory.h> header file. */
 #undef HAVE_MEMORY_H
 
+/* Define to 1 if pthread.h is present. */
+#undef HAVE_PTHREAD_H
+
 /* Define to 1 if you have the <stdint.h> header file. */
 #undef HAVE_STDINT_H
 
diff --git a/lto-plugin/configure b/lto-plugin/configure
index b820accfd65..6d452063cfb 100755
--- a/lto-plugin/configure
+++ b/lto-plugin/configure
@@ -5643,6 +5643,9 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 
 
 
+# The plug-in depends on pthreads
+LDFLAGS="-lpthread"
+
 # Check whether -static-libgcc is supported.
 saved_LDFLAGS="$LDFLAGS"
 LDFLAGS="$LDFLAGS -static-libgcc"
@@ -6010,6 +6013,16 @@ else
 fi
 
 
+# Check for thread headers.
+ac_fn_c_check_header_mongrel "$LINENO" "pthread.h" "ac_cv_header_pthread_h" "$ac_includes_default"
+if test "x$ac_cv_header_pthread_h" = xyes; then :
+
+$as_echo "#define HAVE_PTHREAD_H 1" >>confdefs.h
+
+fi
+
+
+
 case `pwd` in
   *\ * | *\	*)
     { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: Libtool does not cope well with whitespace in \`pwd\`" >&5
@@ -12081,7 +12094,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 12084 "configure"
+#line 12097 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -12187,7 +12200,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 12190 "configure"
+#line 12203 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
diff --git a/lto-plugin/configure.ac b/lto-plugin/configure.ac
index bc5b618a495..aafabd4fa7b 100644
--- a/lto-plugin/configure.ac
+++ b/lto-plugin/configure.ac
@@ -13,6 +13,9 @@ AC_PROG_CC
 AC_SYS_LARGEFILE
 ACX_PROG_CC_WARNING_OPTS([-Wall], [ac_lto_plugin_warn_cflags])
 
+# The plug-in depends on pthreads
+LDFLAGS="-lpthread"
+
 # Check whether -static-libgcc is supported.
 saved_LDFLAGS="$LDFLAGS"
 LDFLAGS="$LDFLAGS -static-libgcc"
@@ -87,6 +90,10 @@ AM_CONDITIONAL(LTO_PLUGIN_USE_SYMVER, [test "x$lto_plugin_use_symver" != xno])
 AM_CONDITIONAL(LTO_PLUGIN_USE_SYMVER_GNU, [test "x$lto_plugin_use_symver" = xgnu])
 AM_CONDITIONAL(LTO_PLUGIN_USE_SYMVER_SUN, [test "x$lto_plugin_use_symver" = xsun])
 
+# Check for thread headers.
+AC_CHECK_HEADER(pthread.h,
+  [AC_DEFINE(HAVE_PTHREAD_H, 1, [Define to 1 if pthread.h is present.])])
+
 AM_PROG_LIBTOOL
 ACX_LT_HOST_FLAGS
 AC_SUBST(target_noncanonical)
diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 00b760636dc..635e126946b 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -40,7 +40,11 @@ along with this program; see the file COPYING3.  If not see
 
 #ifdef HAVE_CONFIG_H
 #include "config.h"
+#if !HAVE_PTHREAD_H
+#error POSIX threads are mandatory dependency
 #endif
+#endif
+
 #if HAVE_STDINT_H
 #include <stdint.h>
 #endif
@@ -55,6 +59,7 @@ along with this program; see the file COPYING3.  If not see
 #include <unistd.h>
 #include <fcntl.h>
 #include <sys/types.h>
+#include <pthread.h>
 #ifdef HAVE_SYS_WAIT_H
 #include <sys/wait.h>
 #endif
@@ -157,6 +162,9 @@ enum symbol_style
   ss_uscore,	/* Underscore prefix all symbols.  */
 };
 
+/* Plug-in mutex.  */
+static pthread_mutex_t plugin_lock;
+
 static char *arguments_file_name;
 static ld_plugin_register_claim_file register_claim_file;
 static ld_plugin_register_all_symbols_read register_all_symbols_read;
@@ -1262,15 +1270,18 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
 			      lto_file.symtab.syms);
       check (status == LDPS_OK, LDPL_FATAL, "could not add symbols");
 
+      pthread_mutex_lock (&plugin_lock);
       num_claimed_files++;
       claimed_files =
 	xrealloc (claimed_files,
 		  num_claimed_files * sizeof (struct plugin_file_info));
       claimed_files[num_claimed_files - 1] = lto_file;
+      pthread_mutex_unlock (&plugin_lock);
 
       *claimed = 1;
     }
 
+  pthread_mutex_lock (&plugin_lock);
   if (offload_files == NULL)
     {
       /* Add dummy item to the start of the list.  */
@@ -1333,11 +1344,14 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
 	offload_files_last_lto = ofld;
       num_offload_files++;
     }
+  pthread_mutex_unlock (&plugin_lock);
 
   goto cleanup;
 
  err:
+  pthread_mutex_lock (&plugin_lock);
   non_claimed_files++;
+  pthread_mutex_unlock (&plugin_lock);
   free (lto_file.name);
 
  cleanup:
@@ -1415,6 +1429,12 @@ onload (struct ld_plugin_tv *tv)
   struct ld_plugin_tv *p;
   enum ld_plugin_status status;
 
+  if (pthread_mutex_init (&plugin_lock, NULL) != 0)
+    {
+      fprintf (stderr, "mutex init failed\n");
+      abort ();
+    }
+
   p = tv;
   while (p->tv_tag)
     {
-- 
2.36.1

Re: [PATCH 2/3] lto-plugin: make claim_file_handler thread-safe

[Richard Biener]

> Am 21.06.2022 um 10:43 schrieb Martin Liška <mliska@suse.cz>:
> 
> On 6/21/22 09:56, Richard Biener wrote:
>>> On Mon, Jun 20, 2022 at 12:20 PM Martin Liška <mliska@suse.cz> wrote:
>>> 
>>> On 6/20/22 11:32, Richard Biener wrote:
>>>> On Thu, Jun 16, 2022 at 9:01 AM Martin Liška <mliska@suse.cz> wrote:
>>>>> 
>>>>> lto-plugin/ChangeLog:
>>>>> 
>>>>>        * lto-plugin.c (plugin_lock): New lock.
>>>>>        (claim_file_handler): Use mutex for critical section.
>>>>>        (onload): Initialize mutex.
>>>>> ---
>>>>> lto-plugin/lto-plugin.c | 16 +++++++++++++++-
>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
>>>>> index 00b760636dc..13118c4983c 100644
>>>>> --- a/lto-plugin/lto-plugin.c
>>>>> +++ b/lto-plugin/lto-plugin.c
>>>>> @@ -55,6 +55,7 @@ along with this program; see the file COPYING3.  If not see
>>>>> #include <unistd.h>
>>>>> #include <fcntl.h>
>>>>> #include <sys/types.h>
>>>>> +#include <pthread.h>
>>>> 
>>>> Not sure if we support any non-pthread target for building the LTO
>>>> plugin, but it
>>>> seems we have
>>>> 
>>>>  # Among non-ELF, only Windows platforms support the lto-plugin so far.
>>>>  # Build it unless LTO was explicitly disabled.
>>>>  case $target in
>>>>    *-cygwin* | *-mingw*) build_lto_plugin=$enable_lto ;;
>>>> 
>>>> which suggests that at least build validating the above with --enable-lto
>>> 
>>> Verified that it's fine.
>>> 
>>>> 
>>>> IIRC we have gthr-*.h in libgcc/, not sure if that's usable in a
>>>> host linker plugin.
>>>> 
>>>>> #ifdef HAVE_SYS_WAIT_H
>>>>> #include <sys/wait.h>
>>>>> #endif
>>>>> @@ -157,6 +158,9 @@ enum symbol_style
>>>>>   ss_uscore,   /* Underscore prefix all symbols.  */
>>>>> };
>>>>> 
>>>>> +/* Plug-in mutex.  */
>>>>> +static pthread_mutex_t plugin_lock;
>>>>> +
>>>>> static char *arguments_file_name;
>>>>> static ld_plugin_register_claim_file register_claim_file;
>>>>> static ld_plugin_register_all_symbols_read register_all_symbols_read;
>>>>> @@ -1262,15 +1266,18 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>>>>>                              lto_file.symtab.syms);
>>>>>       check (status == LDPS_OK, LDPL_FATAL, "could not add symbols");
>>>>> 
>>>>> +      pthread_mutex_lock (&plugin_lock);
>>>>>       num_claimed_files++;
>>>>>       claimed_files =
>>>>>        xrealloc (claimed_files,
>>>>>                  num_claimed_files * sizeof (struct plugin_file_info));
>>>>>       claimed_files[num_claimed_files - 1] = lto_file;
>>>>> +      pthread_mutex_unlock (&plugin_lock);
>>>>> 
>>>>>       *claimed = 1;
>>>>>     }
>>>>> 
>>>>> +  pthread_mutex_lock (&plugin_lock);
>>>>>   if (offload_files == NULL)
>>>>>     {
>>>>>       /* Add dummy item to the start of the list.  */
>>>>> @@ -1333,11 +1340,12 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>>>>>        offload_files_last_lto = ofld;
>>>>>       num_offload_files++;
>>>>>     }
>>>>> +  pthread_mutex_unlock (&plugin_lock);
>>>>> 
>>>>>   goto cleanup;
>>>>> 
>>>>>  err:
>>>>> -  non_claimed_files++;
>>>>> +  __atomic_fetch_add (&non_claimed_files, 1, __ATOMIC_RELAXED);
>>>> 
>>>> is it worth "optimizing" this with yet another need for target specific support
>>>> (just use pthread_mutex here as well?)
>>> 
>>> Sure.
>>> 
>>> May I install the patch with the change?
>> 
>> Can you at least add a configure check for pthread.h and maybe disable
>> locking when not found or erroring out?  I figure we have GCC_AC_THREAD_HEADER
> 
> All right, let's error out then.
> 
>> for the gthr.h stuff using $target_thread_file (aka --enable-threads=XYZ),
>> but as said that's for the target and I don't see any host uses.  We might also
>> add an explicit list of hosts (*-linux*?) where we enable thread support for
>> lto-plugin, providing opt-in (so you'd have to wrap the mutex taking or
>> if-def it out).
>> 
>> I think you also need to link lto-plugin with -pthread, no?
> 
> Yep.
> 
> Please see the updated patch.
> 

Please use -pthread instead of -lpthread 

Otherwise looks OK to me.

>> On linux
>> it might work omitting that but I'm not sure other libc have serial pthread
>> stubs in their libc.  BFD ld definitely doesn't link against pthread so
>> dlopening lto-plugin will fail (also not all libc might like
>> initializing threads
>> from a dlopen _init).
> 
> What initializing threads do you mean?

It’s target dependent what kind of global state init need to be done when any pthread facility is used.

Richard 

> Martin
> 
>> 
>> Richard.
>> 
>>> Cheers,
>>> Martin
>>> 
>>>> 
>>>>>   free (lto_file.name);
>>>>> 
>>>>>  cleanup:
>>>>> @@ -1415,6 +1423,12 @@ onload (struct ld_plugin_tv *tv)
>>>>>   struct ld_plugin_tv *p;
>>>>>   enum ld_plugin_status status;
>>>>> 
>>>>> +  if (pthread_mutex_init (&plugin_lock, NULL) != 0)
>>>>> +    {
>>>>> +      fprintf (stderr, "mutex init failed\n");
>>>>> +      abort ();
>>>>> +    }
>>>>> +
>>>>>   p = tv;
>>>>>   while (p->tv_tag)
>>>>>     {
>>>>> --
>>>>> 2.36.1
>>>>> 
>>>>> 
> <0001-lto-plugin-make-claim_file_handler-thread-safe.patch>

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

[Martin Liška]

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(+)

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);
-- 
2.36.1

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

[Alexander Monakov]

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

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2789 bytes --]

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

[-- Attachment #2: 0001-lto-plugin-implement-LDPT_GET_API_VERSION.patch --]
[-- Type: text/x-patch, Size: 5077 bytes --]

From 3e01823f22282c7a46771c82f9622f7ff2929b18 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:01:52 +0200
Subject: [PATCH] lto-plugin: implement LDPT_GET_API_VERSION

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    | 31 +++++++++++++++++++++++++++++++
 lto-plugin/lto-plugin.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 8aebe2ff267..cc3a43b44a6 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -483,6 +483,35 @@ 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 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.
+  Identifier pointers remain valid as long as the plugin is loaded.  */
+
+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 +550,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 +586,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);
-- 
2.36.1

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

[Richard Biener]

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

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

[Martin Liška]

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

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

[Rui Ueyama]

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
>

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

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
> 

[-- Attachment #2: 0001-lto-plugin-implement-LDPT_GET_API_VERSION.patch --]
[-- Type: text/x-patch, Size: 5175 bytes --]

From f04b187240b786a868377c3854113598ba69ce1b Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:01:52 +0200
Subject: [PATCH] lto-plugin: implement LDPT_GET_API_VERSION

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    | 33 +++++++++++++++++++++++++++++++++
 lto-plugin/lto-plugin.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 8aebe2ff267..f6402151473 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -483,6 +483,37 @@ 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 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.
+  Both PLUGIN_VERSION and LINKER_VERSION are in the following format:
+  1000 * MAJOR + MINOR.
+  Identifier pointers remain valid as long as the plugin is loaded.  */
+
+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 +552,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 +588,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 635e126946b..06e3219de75 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -74,6 +74,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.  */
@@ -174,6 +175,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;
@@ -1421,6 +1426,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
@@ -1487,12 +1519,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);
-- 
2.36.1

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

[Richard Biener]

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
> >

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

[Martin Liška]

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
>>>

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

[Rui Ueyama]

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
> >>>
>
>

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 3550 bytes --]

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
>     >>>
> 

[-- Attachment #2: 0001-lto-plugin-implement-LDPT_GET_API_VERSION.patch --]
[-- Type: text/x-patch, Size: 6393 bytes --]

From c105ee05439929f1c1fd22d15f56cf398b5a8a0d Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:01:52 +0200
Subject: [PATCH] lto-plugin: implement LDPT_GET_API_VERSION

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.
	* Makefile.am: Add -DBASE_VERSION.
	* Makefile.in: Regenerate.
---
 include/plugin-api.h    | 32 +++++++++++++++++++++++++++++++
 lto-plugin/Makefile.am  |  2 +-
 lto-plugin/Makefile.in  |  2 +-
 lto-plugin/lto-plugin.c | 42 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 8aebe2ff267..1deda680081 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -483,6 +483,36 @@ 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 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.
+  Identifier pointers remain valid as long as the plugin is loaded.  */
+
+typedef
+enum linker_api_version
+(*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,
+			      const char **linker_identifier,
+			      const char **linker_version);
+
 /* Values for the tv_tag field of the transfer vector.  */
 
 enum ld_plugin_tag
@@ -521,6 +551,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 +587,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/Makefile.am b/lto-plugin/Makefile.am
index 81362eafc36..482946e4dd5 100644
--- a/lto-plugin/Makefile.am
+++ b/lto-plugin/Makefile.am
@@ -8,7 +8,7 @@ target_noncanonical := @target_noncanonical@
 libexecsubdir := $(libexecdir)/gcc/$(real_target_noncanonical)/$(gcc_version)$(accel_dir_suffix)
 
 AM_CPPFLAGS = -I$(top_srcdir)/../include $(DEFS)
-AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS)
+AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS) -DBASE_VERSION='"$(gcc_version)"'
 # The plug-in depends on pthreads.
 AM_LDFLAGS = -pthread @ac_lto_plugin_ldflags@
 AM_LIBTOOLFLAGS = --tag=disable-static
diff --git a/lto-plugin/Makefile.in b/lto-plugin/Makefile.in
index 2033dd9b7c2..9453bc7d607 100644
--- a/lto-plugin/Makefile.in
+++ b/lto-plugin/Makefile.in
@@ -343,7 +343,7 @@ AUTOMAKE_OPTIONS = no-dependencies
 gcc_version := $(shell @get_gcc_base_ver@ $(top_srcdir)/../gcc/BASE-VER)
 libexecsubdir := $(libexecdir)/gcc/$(real_target_noncanonical)/$(gcc_version)$(accel_dir_suffix)
 AM_CPPFLAGS = -I$(top_srcdir)/../include $(DEFS)
-AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS)
+AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS) -DBASE_VERSION='"$(gcc_version)"'
 # The plug-in depends on pthreads.
 AM_LDFLAGS = -pthread @ac_lto_plugin_ldflags@
 AM_LIBTOOLFLAGS = --tag=disable-static
diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 7927dca60a4..aad17b403e1 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -180,6 +180,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;
@@ -1428,6 +1432,38 @@ process_option (const char *option)
   verbose = verbose || debug;
 }
 
+/* Negotiate linker API version.  */
+
+static void
+negotiate_api_version (void)
+{
+  const char *linker_identifier;
+  const char *linker_version;
+
+  enum linker_api_version supported_api = LAPI_UNSPECIFIED;
+#if HAVE_PTHREAD_LOCKING
+  supported_api = LAPI_V1;
+#endif
+
+  api_version = get_api_version ("GCC", BASE_VERSION, LAPI_UNSPECIFIED,
+				 supported_api, &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
@@ -1496,12 +1532,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);
-- 
2.36.1

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

[Alexander Monakov]

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

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

[Rui Ueyama]

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.

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

[Alexander Monakov]

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

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

[Richard Biener]

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

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1925 bytes --]

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

[-- Attachment #2: 0001-lto-plugin-implement-LDPT_GET_API_VERSION.patch --]
[-- Type: text/x-patch, Size: 6597 bytes --]

From 7905b58c062774deffcd3d5adfa893e17bcc0f26 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:01:52 +0200
Subject: [PATCH] lto-plugin: implement LDPT_GET_API_VERSION

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.
	* Makefile.am: Add -DBASE_VERSION.
	* Makefile.in: Regenerate.
---
 include/plugin-api.h    | 33 +++++++++++++++++++++++++++++
 lto-plugin/Makefile.am  |  2 +-
 lto-plugin/Makefile.in  |  2 +-
 lto-plugin/lto-plugin.c | 47 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 8aebe2ff267..e4ed1d139bb 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -483,6 +483,37 @@ 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_V0,
+
+   /* API level v1.  The linker provides 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
+};
+
+/* 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.  The returned value
+  by linker must be in range [MINIMAL_API_SUPPORTED, MAXIMAL_API_SUPPORTED].
+  Identifier pointers remain valid as long as the plugin is loaded.  */
+
+typedef
+enum linker_api_version
+(*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,
+			      const char **linker_identifier,
+			      const char **linker_version);
+
 /* Values for the tv_tag field of the transfer vector.  */
 
 enum ld_plugin_tag
@@ -521,6 +552,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 +588,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/Makefile.am b/lto-plugin/Makefile.am
index 81362eafc36..482946e4dd5 100644
--- a/lto-plugin/Makefile.am
+++ b/lto-plugin/Makefile.am
@@ -8,7 +8,7 @@ target_noncanonical := @target_noncanonical@
 libexecsubdir := $(libexecdir)/gcc/$(real_target_noncanonical)/$(gcc_version)$(accel_dir_suffix)
 
 AM_CPPFLAGS = -I$(top_srcdir)/../include $(DEFS)
-AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS)
+AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS) -DBASE_VERSION='"$(gcc_version)"'
 # The plug-in depends on pthreads.
 AM_LDFLAGS = -pthread @ac_lto_plugin_ldflags@
 AM_LIBTOOLFLAGS = --tag=disable-static
diff --git a/lto-plugin/Makefile.in b/lto-plugin/Makefile.in
index 2033dd9b7c2..9453bc7d607 100644
--- a/lto-plugin/Makefile.in
+++ b/lto-plugin/Makefile.in
@@ -343,7 +343,7 @@ AUTOMAKE_OPTIONS = no-dependencies
 gcc_version := $(shell @get_gcc_base_ver@ $(top_srcdir)/../gcc/BASE-VER)
 libexecsubdir := $(libexecdir)/gcc/$(real_target_noncanonical)/$(gcc_version)$(accel_dir_suffix)
 AM_CPPFLAGS = -I$(top_srcdir)/../include $(DEFS)
-AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS)
+AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS) -DBASE_VERSION='"$(gcc_version)"'
 # The plug-in depends on pthreads.
 AM_LDFLAGS = -pthread @ac_lto_plugin_ldflags@
 AM_LIBTOOLFLAGS = --tag=disable-static
diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 7927dca60a4..e9afd2fb76d 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -180,6 +180,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_V0 if there is not negotiation.  */
+static enum linker_api_version api_version = LAPI_V0;
 
 static struct plugin_file_info *claimed_files = NULL;
 static unsigned int num_claimed_files = 0;
@@ -1428,6 +1432,43 @@ process_option (const char *option)
   verbose = verbose || debug;
 }
 
+/* Negotiate linker API version.  */
+
+static void
+negotiate_api_version (void)
+{
+  const char *linker_identifier;
+  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,
+				 supported_api, &linker_identifier, &linker_version);
+  if (api_version > supported_api)
+    {
+      fprintf (stderr, "requested an unsupported API version (%d)\n", api_version);
+      abort ();
+    }
+
+  switch (api_version)
+    {
+    case LAPI_V0:
+      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 (%d)\n", api_version);
+      abort ();
+    }
+}
+
 /* Called by a linker after loading the plugin. TV is the transfer vector. */
 
 enum ld_plugin_status
@@ -1496,12 +1537,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);
-- 
2.36.1

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

[Rui Ueyama]

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

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

[Alexander Monakov]

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

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

[Martin Liška]

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

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

[Rui Ueyama]

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
>

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

[Alexander Monakov]

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

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

[Richard Biener]

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

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

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

[-- Attachment #2: 0001-lto-plugin-implement-LDPT_GET_API_VERSION.patch --]
[-- Type: text/x-patch, Size: 6537 bytes --]

From 8f7372bd2b3d76feeca84cdbb0d392d93569bd57 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:01:52 +0200
Subject: [PATCH] lto-plugin: implement LDPT_GET_API_VERSION

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.
	* Makefile.am: Add -DBASE_VERSION.
	* Makefile.in: Regenerate.
---
 include/plugin-api.h    | 33 +++++++++++++++++++++++++++++
 lto-plugin/Makefile.am  |  2 +-
 lto-plugin/Makefile.in  |  2 +-
 lto-plugin/lto-plugin.c | 47 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 8aebe2ff267..0b61cfc0443 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -483,6 +483,37 @@ 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_V0,
+
+   /* API level v1.  The linker provides 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
+};
+
+/* 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.  The returned value
+  by linker must be in range [MINIMAL_API_SUPPORTED, MAXIMAL_API_SUPPORTED].
+  Identifier pointers remain valid as long as the plugin is loaded.  */
+
+typedef
+int
+(*ld_plugin_get_api_version) (const char *plugin_identifier,
+			      const char *plugin_version,
+			      int minimal_api_supported,
+			      int maximal_api_supported,
+			      const char **linker_identifier,
+			      const char **linker_version);
+
 /* Values for the tv_tag field of the transfer vector.  */
 
 enum ld_plugin_tag
@@ -521,6 +552,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 +588,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/Makefile.am b/lto-plugin/Makefile.am
index 81362eafc36..482946e4dd5 100644
--- a/lto-plugin/Makefile.am
+++ b/lto-plugin/Makefile.am
@@ -8,7 +8,7 @@ target_noncanonical := @target_noncanonical@
 libexecsubdir := $(libexecdir)/gcc/$(real_target_noncanonical)/$(gcc_version)$(accel_dir_suffix)
 
 AM_CPPFLAGS = -I$(top_srcdir)/../include $(DEFS)
-AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS)
+AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS) -DBASE_VERSION='"$(gcc_version)"'
 # The plug-in depends on pthreads.
 AM_LDFLAGS = -pthread @ac_lto_plugin_ldflags@
 AM_LIBTOOLFLAGS = --tag=disable-static
diff --git a/lto-plugin/Makefile.in b/lto-plugin/Makefile.in
index 2033dd9b7c2..9453bc7d607 100644
--- a/lto-plugin/Makefile.in
+++ b/lto-plugin/Makefile.in
@@ -343,7 +343,7 @@ AUTOMAKE_OPTIONS = no-dependencies
 gcc_version := $(shell @get_gcc_base_ver@ $(top_srcdir)/../gcc/BASE-VER)
 libexecsubdir := $(libexecdir)/gcc/$(real_target_noncanonical)/$(gcc_version)$(accel_dir_suffix)
 AM_CPPFLAGS = -I$(top_srcdir)/../include $(DEFS)
-AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS)
+AM_CFLAGS = @ac_lto_plugin_warn_cflags@ $(CET_HOST_FLAGS) -DBASE_VERSION='"$(gcc_version)"'
 # The plug-in depends on pthreads.
 AM_LDFLAGS = -pthread @ac_lto_plugin_ldflags@
 AM_LIBTOOLFLAGS = --tag=disable-static
diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 7927dca60a4..e9afd2fb76d 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -180,6 +180,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_V0 if there is not negotiation.  */
+static enum linker_api_version api_version = LAPI_V0;
 
 static struct plugin_file_info *claimed_files = NULL;
 static unsigned int num_claimed_files = 0;
@@ -1428,6 +1432,43 @@ process_option (const char *option)
   verbose = verbose || debug;
 }
 
+/* Negotiate linker API version.  */
+
+static void
+negotiate_api_version (void)
+{
+  const char *linker_identifier;
+  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,
+				 supported_api, &linker_identifier, &linker_version);
+  if (api_version > supported_api)
+    {
+      fprintf (stderr, "requested an unsupported API version (%d)\n", api_version);
+      abort ();
+    }
+
+  switch (api_version)
+    {
+    case LAPI_V0:
+      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 (%d)\n", api_version);
+      abort ();
+    }
+}
+
 /* Called by a linker after loading the plugin. TV is the transfer vector. */
 
 enum ld_plugin_status
@@ -1496,12 +1537,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);
-- 
2.37.0

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

[Rui Ueyama]

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

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

[Richard Biener]

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

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

[Martin Liška]

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

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

[Rui Ueyama]

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.