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

[Rui Ueyama via Gcc-patches]

On Mon, Jul 4, 2022 at 10:17 PM Martin Liška  wrote:

> On 7/1/22 08:36, Richard Biener wrote:
> > On Thu, Jun 30, 2022 at 10:42 AM Martin Liška  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  mli...@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

[Rui Ueyama via Gcc-patches]

On Fri, Jul 8, 2022 at 8:41 PM Alexander Monakov  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

[Rui Ueyama via Gcc-patches]

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  wrote:
>
> On 7/11/22 11:55, Richard Biener wrote:
> > On Mon, Jul 11, 2022 at 11:16 AM Alexander Monakov  
> > 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 via Gcc-patches]

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  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  wrote:
> >>
> >> On 7/11/22 11:55, Richard Biener wrote:
> >>> On Mon, Jul 11, 2022 at 11:16 AM Alexander Monakov  
> >>> 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 via Gcc-patches]

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  wrote:
>
> On 7/12/22 08:28, Richard Biener wrote:
> > On Mon, Jul 11, 2022 at 6:35 PM Alexander Monakov  
> > 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

[Rui Ueyama via Gcc-patches]

On Tue, Jul 12, 2022 at 9:31 PM Martin Liška  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.

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

[Rui Ueyama via Gcc-patches]

On Fri, May 6, 2022 at 10:47 PM 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.

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

[Rui Ueyama via Gcc-patches]

On Sun, May 15, 2022 at 3:53 PM Alexander Monakov  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

[Rui Ueyama via Gcc-patches]

On Sun, May 15, 2022 at 4:51 PM Alexander Monakov  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

[Rui Ueyama via Gcc-patches]

On Sun, May 15, 2022 at 6:09 PM Alexander Monakov  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

[Rui Ueyama via Gcc-patches]

On Sun, May 15, 2022 at 7:37 PM Alexander Monakov  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

[Rui Ueyama via Gcc-patches]

On Sun, May 15, 2022 at 8:07 PM Alexander Monakov  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

[Rui Ueyama via Gcc-patches]

On Mon, May 16, 2022 at 2:38 PM Alexander Monakov  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

[Rui Ueyama via Gcc-patches]

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  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 via Gcc-patches]

On Mon, May 16, 2022 at 6:28 PM Richard Biener
 wrote:
>
> On Mon, May 16, 2022 at 11:58 AM Rui Ueyama  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  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.
> 

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

[Rui Ueyama via Gcc-patches]

On Mon, May 16, 2022 at 8:04 PM Martin Liška  wrote:
>
> On 5/16/22 12:28, Richard Biener wrote:
> > On Mon, May 16, 2022 at 11:58 AM Rui Ueyama  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  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 3/3] lto-plugin: implement LDPT_GET_API_VERSION

[Rui Ueyama via Gcc-patches]

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