LTO plugin: add ld_plugin_version callback.

  Hi.

This in a new plug-in function that helps identifying compiler
by a linker. Will be used in mold linker.

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

Ready to be installed?
Thanks,
Martin

include/ChangeLog:

	* plugin-api.h (enum ld_plugin_compiler): New enum.
	(enum ld_plugin_version): New typedef.
	(struct ld_plugin_tv): Add new field.

lto-plugin/ChangeLog:

	* lto-plugin.c (onload): Call ld_plugin_version.
---
 include/plugin-api.h    | 18 +++++++++++++++++-
 lto-plugin/lto-plugin.c |  4 ++++
 2 files changed, 21 insertions(+), 1 deletion(-)
  

On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
>
> Hi.
>
> This in a new plug-in function that helps identifying compiler
> by a linker. Will be used in mold linker.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

It looks a bit backward to query the compiler (and even more so to
have a fixed set) from the linker.  What is this going to be used for?
If then this should probably receive a unformatted string the linker
has to decipher itself like "gcc (SUSE Linux) 7.5.0" or something
(what we produce with gcc --version), and clang would produce
"clang version 11.0.1" or so?

Richard.

> Thanks,
> Martin
>
> include/ChangeLog:
>
>         * plugin-api.h (enum ld_plugin_compiler): New enum.
>         (enum ld_plugin_version): New typedef.
>         (struct ld_plugin_tv): Add new field.
>
> lto-plugin/ChangeLog:
>
>         * lto-plugin.c (onload): Call ld_plugin_version.
> ---
>  include/plugin-api.h    | 18 +++++++++++++++++-
>  lto-plugin/lto-plugin.c |  4 ++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/include/plugin-api.h b/include/plugin-api.h
> index 4e12c0320d6..4d0989028cc 100644
> --- a/include/plugin-api.h
> +++ b/include/plugin-api.h
> @@ -211,6 +211,13 @@ enum ld_plugin_symbol_section_kind
>    LDSSK_BSS
>  };
>
> +enum ld_plugin_compiler
> +{
> +  LDPC_UNKNOWN,
> +  LDPC_GCC,
> +  LDPC_LLVM
> +};
> +
>  /* How a symbol is resolved.  */
>
>  enum ld_plugin_symbol_resolution
> @@ -475,6 +482,13 @@ enum ld_plugin_status
>  (*ld_plugin_get_wrap_symbols) (uint64_t *num_symbols,
>                                 const char ***wrap_symbol_list);
>
> +/* The linker's interface for registering the "plugin_version" handler.
> +   This handler is called directly after onload and provides compiler
> +   and its number version (MAJOR * 1000 + MINOR).  */
> +
> +typedef
> +void (*ld_plugin_version) (enum ld_plugin_compiler compiler, int version);
> +
>  enum ld_plugin_level
>  {
>    LDPL_INFO,
> @@ -520,7 +534,8 @@ enum ld_plugin_tag
>    LDPT_GET_INPUT_SECTION_SIZE = 30,
>    LDPT_REGISTER_NEW_INPUT_HOOK = 31,
>    LDPT_GET_WRAP_SYMBOLS = 32,
> -  LDPT_ADD_SYMBOLS_V2 = 33
> +  LDPT_ADD_SYMBOLS_V2 = 33,
> +  LDPT_PLUGIN_VERSION = 34,
>  };
>
>  /* The plugin transfer vector.  */
> @@ -556,6 +571,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_version tv_plugin_version;
>    } tv_u;
>  };
>
> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> index 7a1c77812f6..698a0617ca7 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.  */
> @@ -1466,6 +1467,9 @@ 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_PLUGIN_VERSION:
> +         p->tv_u.tv_plugin_version (LDPC_GCC, GCC_VERSION);
> +         break;
>         default:
>           break;
>         }
> --
> 2.36.0
>
  

On 5/2/22 10:09, Richard Biener wrote:
> On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> Hi.
>>
>> This in a new plug-in function that helps identifying compiler
>> by a linker. Will be used in mold linker.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
> 
> It looks a bit backward to query the compiler (and even more so to
> have a fixed set) from the linker.  What is this going to be used for?

It's going to be used by mold, there are small behavior divergence
in between LLVM and GCC, where one compiler closes provided file descriptors:
https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550

> If then this should probably receive a unformatted string the linker
> has to decipher itself like "gcc (SUSE Linux) 7.5.0" or something
> (what we produce with gcc --version), and clang would produce
> "clang version 11.0.1" or so?

Well, it brings its own set of problems such string parsing. Note that the plugin API
currently provides something like:

/* The version of gold being used, or -1 if not gold.  The number is
   MAJOR * 100 + MINOR.  */
static int gold_version = -1;

So my suggestion is quite aligned with that.

Martin

> 
> Richard.
> 
>> Thanks,
>> Martin
>>
>> include/ChangeLog:
>>
>>         * plugin-api.h (enum ld_plugin_compiler): New enum.
>>         (enum ld_plugin_version): New typedef.
>>         (struct ld_plugin_tv): Add new field.
>>
>> lto-plugin/ChangeLog:
>>
>>         * lto-plugin.c (onload): Call ld_plugin_version.
>> ---
>>  include/plugin-api.h    | 18 +++++++++++++++++-
>>  lto-plugin/lto-plugin.c |  4 ++++
>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/plugin-api.h b/include/plugin-api.h
>> index 4e12c0320d6..4d0989028cc 100644
>> --- a/include/plugin-api.h
>> +++ b/include/plugin-api.h
>> @@ -211,6 +211,13 @@ enum ld_plugin_symbol_section_kind
>>    LDSSK_BSS
>>  };
>>
>> +enum ld_plugin_compiler
>> +{
>> +  LDPC_UNKNOWN,
>> +  LDPC_GCC,
>> +  LDPC_LLVM
>> +};
>> +
>>  /* How a symbol is resolved.  */
>>
>>  enum ld_plugin_symbol_resolution
>> @@ -475,6 +482,13 @@ enum ld_plugin_status
>>  (*ld_plugin_get_wrap_symbols) (uint64_t *num_symbols,
>>                                 const char ***wrap_symbol_list);
>>
>> +/* The linker's interface for registering the "plugin_version" handler.
>> +   This handler is called directly after onload and provides compiler
>> +   and its number version (MAJOR * 1000 + MINOR).  */
>> +
>> +typedef
>> +void (*ld_plugin_version) (enum ld_plugin_compiler compiler, int version);
>> +
>>  enum ld_plugin_level
>>  {
>>    LDPL_INFO,
>> @@ -520,7 +534,8 @@ enum ld_plugin_tag
>>    LDPT_GET_INPUT_SECTION_SIZE = 30,
>>    LDPT_REGISTER_NEW_INPUT_HOOK = 31,
>>    LDPT_GET_WRAP_SYMBOLS = 32,
>> -  LDPT_ADD_SYMBOLS_V2 = 33
>> +  LDPT_ADD_SYMBOLS_V2 = 33,
>> +  LDPT_PLUGIN_VERSION = 34,
>>  };
>>
>>  /* The plugin transfer vector.  */
>> @@ -556,6 +571,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_version tv_plugin_version;
>>    } tv_u;
>>  };
>>
>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
>> index 7a1c77812f6..698a0617ca7 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.  */
>> @@ -1466,6 +1467,9 @@ 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_PLUGIN_VERSION:
>> +         p->tv_u.tv_plugin_version (LDPC_GCC, GCC_VERSION);
>> +         break;
>>         default:
>>           break;
>>         }
>> --
>> 2.36.0
>>
  

On Mon, May 2, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/2/22 10:09, Richard Biener wrote:
> > On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> Hi.
> >>
> >> This in a new plug-in function that helps identifying compiler
> >> by a linker. Will be used in mold linker.
> >>
> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>
> >> Ready to be installed?
> >
> > It looks a bit backward to query the compiler (and even more so to
> > have a fixed set) from the linker.  What is this going to be used for?
>
> It's going to be used by mold, there are small behavior divergence
> in between LLVM and GCC, where one compiler closes provided file descriptors:
> https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550

But that then seems to be a defect in the plugin API specification (or
one of the two implementations).  Instead of adding a new API entry
that defect should be fixed - both require changes in the actual plugin
specification.

Richard.

> > If then this should probably receive a unformatted string the linker
> > has to decipher itself like "gcc (SUSE Linux) 7.5.0" or something
> > (what we produce with gcc --version), and clang would produce
> > "clang version 11.0.1" or so?
>
> Well, it brings its own set of problems such string parsing. Note that the plugin API
> currently provides something like:
>
> /* The version of gold being used, or -1 if not gold.  The number is
>    MAJOR * 100 + MINOR.  */
> static int gold_version = -1;
>
> So my suggestion is quite aligned with that.
>
> Martin
>
> >
> > Richard.
> >
> >> Thanks,
> >> Martin
> >>
> >> include/ChangeLog:
> >>
> >>         * plugin-api.h (enum ld_plugin_compiler): New enum.
> >>         (enum ld_plugin_version): New typedef.
> >>         (struct ld_plugin_tv): Add new field.
> >>
> >> lto-plugin/ChangeLog:
> >>
> >>         * lto-plugin.c (onload): Call ld_plugin_version.
> >> ---
> >>  include/plugin-api.h    | 18 +++++++++++++++++-
> >>  lto-plugin/lto-plugin.c |  4 ++++
> >>  2 files changed, 21 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/plugin-api.h b/include/plugin-api.h
> >> index 4e12c0320d6..4d0989028cc 100644
> >> --- a/include/plugin-api.h
> >> +++ b/include/plugin-api.h
> >> @@ -211,6 +211,13 @@ enum ld_plugin_symbol_section_kind
> >>    LDSSK_BSS
> >>  };
> >>
> >> +enum ld_plugin_compiler
> >> +{
> >> +  LDPC_UNKNOWN,
> >> +  LDPC_GCC,
> >> +  LDPC_LLVM
> >> +};
> >> +
> >>  /* How a symbol is resolved.  */
> >>
> >>  enum ld_plugin_symbol_resolution
> >> @@ -475,6 +482,13 @@ enum ld_plugin_status
> >>  (*ld_plugin_get_wrap_symbols) (uint64_t *num_symbols,
> >>                                 const char ***wrap_symbol_list);
> >>
> >> +/* The linker's interface for registering the "plugin_version" handler.
> >> +   This handler is called directly after onload and provides compiler
> >> +   and its number version (MAJOR * 1000 + MINOR).  */
> >> +
> >> +typedef
> >> +void (*ld_plugin_version) (enum ld_plugin_compiler compiler, int version);
> >> +
> >>  enum ld_plugin_level
> >>  {
> >>    LDPL_INFO,
> >> @@ -520,7 +534,8 @@ enum ld_plugin_tag
> >>    LDPT_GET_INPUT_SECTION_SIZE = 30,
> >>    LDPT_REGISTER_NEW_INPUT_HOOK = 31,
> >>    LDPT_GET_WRAP_SYMBOLS = 32,
> >> -  LDPT_ADD_SYMBOLS_V2 = 33
> >> +  LDPT_ADD_SYMBOLS_V2 = 33,
> >> +  LDPT_PLUGIN_VERSION = 34,
> >>  };
> >>
> >>  /* The plugin transfer vector.  */
> >> @@ -556,6 +571,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_version tv_plugin_version;
> >>    } tv_u;
> >>  };
> >>
> >> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> >> index 7a1c77812f6..698a0617ca7 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.  */
> >> @@ -1466,6 +1467,9 @@ 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_PLUGIN_VERSION:
> >> +         p->tv_u.tv_plugin_version (LDPC_GCC, GCC_VERSION);
> >> +         break;
> >>         default:
> >>           break;
> >>         }
> >> --
> >> 2.36.0
> >>
>
  

On Mon, May 2, 2022 at 10:51 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, May 2, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
> >
> > On 5/2/22 10:09, Richard Biener wrote:
> > > On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
> > >>
> > >> Hi.
> > >>
> > >> This in a new plug-in function that helps identifying compiler
> > >> by a linker. Will be used in mold linker.
> > >>
> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > >>
> > >> Ready to be installed?
> > >
> > > It looks a bit backward to query the compiler (and even more so to
> > > have a fixed set) from the linker.  What is this going to be used for?
> >
> > It's going to be used by mold, there are small behavior divergence
> > in between LLVM and GCC, where one compiler closes provided file descriptors:
> > https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550
>
> But that then seems to be a defect in the plugin API specification (or
> one of the two implementations).  Instead of adding a new API entry
> that defect should be fixed - both require changes in the actual plugin
> specification.

implementation of course.

Richard.

>
> Richard.
>
> > > If then this should probably receive a unformatted string the linker
> > > has to decipher itself like "gcc (SUSE Linux) 7.5.0" or something
> > > (what we produce with gcc --version), and clang would produce
> > > "clang version 11.0.1" or so?
> >
> > Well, it brings its own set of problems such string parsing. Note that the plugin API
> > currently provides something like:
> >
> > /* The version of gold being used, or -1 if not gold.  The number is
> >    MAJOR * 100 + MINOR.  */
> > static int gold_version = -1;
> >
> > So my suggestion is quite aligned with that.
> >
> > Martin
> >
> > >
> > > Richard.
> > >
> > >> Thanks,
> > >> Martin
> > >>
> > >> include/ChangeLog:
> > >>
> > >>         * plugin-api.h (enum ld_plugin_compiler): New enum.
> > >>         (enum ld_plugin_version): New typedef.
> > >>         (struct ld_plugin_tv): Add new field.
> > >>
> > >> lto-plugin/ChangeLog:
> > >>
> > >>         * lto-plugin.c (onload): Call ld_plugin_version.
> > >> ---
> > >>  include/plugin-api.h    | 18 +++++++++++++++++-
> > >>  lto-plugin/lto-plugin.c |  4 ++++
> > >>  2 files changed, 21 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/include/plugin-api.h b/include/plugin-api.h
> > >> index 4e12c0320d6..4d0989028cc 100644
> > >> --- a/include/plugin-api.h
> > >> +++ b/include/plugin-api.h
> > >> @@ -211,6 +211,13 @@ enum ld_plugin_symbol_section_kind
> > >>    LDSSK_BSS
> > >>  };
> > >>
> > >> +enum ld_plugin_compiler
> > >> +{
> > >> +  LDPC_UNKNOWN,
> > >> +  LDPC_GCC,
> > >> +  LDPC_LLVM
> > >> +};
> > >> +
> > >>  /* How a symbol is resolved.  */
> > >>
> > >>  enum ld_plugin_symbol_resolution
> > >> @@ -475,6 +482,13 @@ enum ld_plugin_status
> > >>  (*ld_plugin_get_wrap_symbols) (uint64_t *num_symbols,
> > >>                                 const char ***wrap_symbol_list);
> > >>
> > >> +/* The linker's interface for registering the "plugin_version" handler.
> > >> +   This handler is called directly after onload and provides compiler
> > >> +   and its number version (MAJOR * 1000 + MINOR).  */
> > >> +
> > >> +typedef
> > >> +void (*ld_plugin_version) (enum ld_plugin_compiler compiler, int version);
> > >> +
> > >>  enum ld_plugin_level
> > >>  {
> > >>    LDPL_INFO,
> > >> @@ -520,7 +534,8 @@ enum ld_plugin_tag
> > >>    LDPT_GET_INPUT_SECTION_SIZE = 30,
> > >>    LDPT_REGISTER_NEW_INPUT_HOOK = 31,
> > >>    LDPT_GET_WRAP_SYMBOLS = 32,
> > >> -  LDPT_ADD_SYMBOLS_V2 = 33
> > >> +  LDPT_ADD_SYMBOLS_V2 = 33,
> > >> +  LDPT_PLUGIN_VERSION = 34,
> > >>  };
> > >>
> > >>  /* The plugin transfer vector.  */
> > >> @@ -556,6 +571,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_version tv_plugin_version;
> > >>    } tv_u;
> > >>  };
> > >>
> > >> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> > >> index 7a1c77812f6..698a0617ca7 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.  */
> > >> @@ -1466,6 +1467,9 @@ 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_PLUGIN_VERSION:
> > >> +         p->tv_u.tv_plugin_version (LDPC_GCC, GCC_VERSION);
> > >> +         break;
> > >>         default:
> > >>           break;
> > >>         }
> > >> --
> > >> 2.36.0
> > >>
> >
  

On 5/2/22 10:51, Richard Biener wrote:
> On Mon, May 2, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 5/2/22 10:09, Richard Biener wrote:
>>> On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> Hi.
>>>>
>>>> This in a new plug-in function that helps identifying compiler
>>>> by a linker. Will be used in mold linker.
>>>>
>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>
>>>> Ready to be installed?
>>>
>>> It looks a bit backward to query the compiler (and even more so to
>>> have a fixed set) from the linker.  What is this going to be used for?
>>
>> It's going to be used by mold, there are small behavior divergence
>> in between LLVM and GCC, where one compiler closes provided file descriptors:
>> https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550
> 
> But that then seems to be a defect in the plugin API specification (or
> one of the two implementations).  Instead of adding a new API entry
> that defect should be fixed - both require changes in the actual plugin
> specification.

Well, the question is if it was specified somewhere or not. Plus there are released
versions of both compilers and plug-ins, so it's not easily fixable :/

Martin

> 
> Richard.
> 
>>> If then this should probably receive a unformatted string the linker
>>> has to decipher itself like "gcc (SUSE Linux) 7.5.0" or something
>>> (what we produce with gcc --version), and clang would produce
>>> "clang version 11.0.1" or so?
>>
>> Well, it brings its own set of problems such string parsing. Note that the plugin API
>> currently provides something like:
>>
>> /* The version of gold being used, or -1 if not gold.  The number is
>>    MAJOR * 100 + MINOR.  */
>> static int gold_version = -1;
>>
>> So my suggestion is quite aligned with that.
>>
>> Martin
>>
>>>
>>> Richard.
>>>
>>>> Thanks,
>>>> Martin
>>>>
>>>> include/ChangeLog:
>>>>
>>>>         * plugin-api.h (enum ld_plugin_compiler): New enum.
>>>>         (enum ld_plugin_version): New typedef.
>>>>         (struct ld_plugin_tv): Add new field.
>>>>
>>>> lto-plugin/ChangeLog:
>>>>
>>>>         * lto-plugin.c (onload): Call ld_plugin_version.
>>>> ---
>>>>  include/plugin-api.h    | 18 +++++++++++++++++-
>>>>  lto-plugin/lto-plugin.c |  4 ++++
>>>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/plugin-api.h b/include/plugin-api.h
>>>> index 4e12c0320d6..4d0989028cc 100644
>>>> --- a/include/plugin-api.h
>>>> +++ b/include/plugin-api.h
>>>> @@ -211,6 +211,13 @@ enum ld_plugin_symbol_section_kind
>>>>    LDSSK_BSS
>>>>  };
>>>>
>>>> +enum ld_plugin_compiler
>>>> +{
>>>> +  LDPC_UNKNOWN,
>>>> +  LDPC_GCC,
>>>> +  LDPC_LLVM
>>>> +};
>>>> +
>>>>  /* How a symbol is resolved.  */
>>>>
>>>>  enum ld_plugin_symbol_resolution
>>>> @@ -475,6 +482,13 @@ enum ld_plugin_status
>>>>  (*ld_plugin_get_wrap_symbols) (uint64_t *num_symbols,
>>>>                                 const char ***wrap_symbol_list);
>>>>
>>>> +/* The linker's interface for registering the "plugin_version" handler.
>>>> +   This handler is called directly after onload and provides compiler
>>>> +   and its number version (MAJOR * 1000 + MINOR).  */
>>>> +
>>>> +typedef
>>>> +void (*ld_plugin_version) (enum ld_plugin_compiler compiler, int version);
>>>> +
>>>>  enum ld_plugin_level
>>>>  {
>>>>    LDPL_INFO,
>>>> @@ -520,7 +534,8 @@ enum ld_plugin_tag
>>>>    LDPT_GET_INPUT_SECTION_SIZE = 30,
>>>>    LDPT_REGISTER_NEW_INPUT_HOOK = 31,
>>>>    LDPT_GET_WRAP_SYMBOLS = 32,
>>>> -  LDPT_ADD_SYMBOLS_V2 = 33
>>>> +  LDPT_ADD_SYMBOLS_V2 = 33,
>>>> +  LDPT_PLUGIN_VERSION = 34,
>>>>  };
>>>>
>>>>  /* The plugin transfer vector.  */
>>>> @@ -556,6 +571,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_version tv_plugin_version;
>>>>    } tv_u;
>>>>  };
>>>>
>>>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
>>>> index 7a1c77812f6..698a0617ca7 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.  */
>>>> @@ -1466,6 +1467,9 @@ 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_PLUGIN_VERSION:
>>>> +         p->tv_u.tv_plugin_version (LDPC_GCC, GCC_VERSION);
>>>> +         break;
>>>>         default:
>>>>           break;
>>>>         }
>>>> --
>>>> 2.36.0
>>>>
>>
  

On Mon, May 2, 2022 at 10:54 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, May 2, 2022 at 10:51 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Mon, May 2, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
> > >
> > > On 5/2/22 10:09, Richard Biener wrote:
> > > > On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
> > > >>
> > > >> Hi.
> > > >>
> > > >> This in a new plug-in function that helps identifying compiler
> > > >> by a linker. Will be used in mold linker.
> > > >>
> > > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > > >>
> > > >> Ready to be installed?
> > > >
> > > > It looks a bit backward to query the compiler (and even more so to
> > > > have a fixed set) from the linker.  What is this going to be used for?
> > >
> > > It's going to be used by mold, there are small behavior divergence
> > > in between LLVM and GCC, where one compiler closes provided file descriptors:
> > > https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550
> >
> > But that then seems to be a defect in the plugin API specification (or
> > one of the two implementations).  Instead of adding a new API entry
> > that defect should be fixed - both require changes in the actual plugin
> > specification.
>
> implementation of course.

So for example a claim_file_v2 entry or alternatively amending
ld_plugin_input_file to denote whether the callee closed the fd
(set it to -1?) could be possible as well if we want to support both
keeping the file open or closing it.  Btw, struct layout/meaning changes
are always more difficult of course.

Richard.

> Richard.
>
> >
> > Richard.
> >
> > > > If then this should probably receive a unformatted string the linker
> > > > has to decipher itself like "gcc (SUSE Linux) 7.5.0" or something
> > > > (what we produce with gcc --version), and clang would produce
> > > > "clang version 11.0.1" or so?
> > >
> > > Well, it brings its own set of problems such string parsing. Note that the plugin API
> > > currently provides something like:
> > >
> > > /* The version of gold being used, or -1 if not gold.  The number is
> > >    MAJOR * 100 + MINOR.  */
> > > static int gold_version = -1;
> > >
> > > So my suggestion is quite aligned with that.
> > >
> > > Martin
> > >
> > > >
> > > > Richard.
> > > >
> > > >> Thanks,
> > > >> Martin
> > > >>
> > > >> include/ChangeLog:
> > > >>
> > > >>         * plugin-api.h (enum ld_plugin_compiler): New enum.
> > > >>         (enum ld_plugin_version): New typedef.
> > > >>         (struct ld_plugin_tv): Add new field.
> > > >>
> > > >> lto-plugin/ChangeLog:
> > > >>
> > > >>         * lto-plugin.c (onload): Call ld_plugin_version.
> > > >> ---
> > > >>  include/plugin-api.h    | 18 +++++++++++++++++-
> > > >>  lto-plugin/lto-plugin.c |  4 ++++
> > > >>  2 files changed, 21 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/include/plugin-api.h b/include/plugin-api.h
> > > >> index 4e12c0320d6..4d0989028cc 100644
> > > >> --- a/include/plugin-api.h
> > > >> +++ b/include/plugin-api.h
> > > >> @@ -211,6 +211,13 @@ enum ld_plugin_symbol_section_kind
> > > >>    LDSSK_BSS
> > > >>  };
> > > >>
> > > >> +enum ld_plugin_compiler
> > > >> +{
> > > >> +  LDPC_UNKNOWN,
> > > >> +  LDPC_GCC,
> > > >> +  LDPC_LLVM
> > > >> +};
> > > >> +
> > > >>  /* How a symbol is resolved.  */
> > > >>
> > > >>  enum ld_plugin_symbol_resolution
> > > >> @@ -475,6 +482,13 @@ enum ld_plugin_status
> > > >>  (*ld_plugin_get_wrap_symbols) (uint64_t *num_symbols,
> > > >>                                 const char ***wrap_symbol_list);
> > > >>
> > > >> +/* The linker's interface for registering the "plugin_version" handler.
> > > >> +   This handler is called directly after onload and provides compiler
> > > >> +   and its number version (MAJOR * 1000 + MINOR).  */
> > > >> +
> > > >> +typedef
> > > >> +void (*ld_plugin_version) (enum ld_plugin_compiler compiler, int version);
> > > >> +
> > > >>  enum ld_plugin_level
> > > >>  {
> > > >>    LDPL_INFO,
> > > >> @@ -520,7 +534,8 @@ enum ld_plugin_tag
> > > >>    LDPT_GET_INPUT_SECTION_SIZE = 30,
> > > >>    LDPT_REGISTER_NEW_INPUT_HOOK = 31,
> > > >>    LDPT_GET_WRAP_SYMBOLS = 32,
> > > >> -  LDPT_ADD_SYMBOLS_V2 = 33
> > > >> +  LDPT_ADD_SYMBOLS_V2 = 33,
> > > >> +  LDPT_PLUGIN_VERSION = 34,
> > > >>  };
> > > >>
> > > >>  /* The plugin transfer vector.  */
> > > >> @@ -556,6 +571,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_version tv_plugin_version;
> > > >>    } tv_u;
> > > >>  };
> > > >>
> > > >> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> > > >> index 7a1c77812f6..698a0617ca7 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.  */
> > > >> @@ -1466,6 +1467,9 @@ 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_PLUGIN_VERSION:
> > > >> +         p->tv_u.tv_plugin_version (LDPC_GCC, GCC_VERSION);
> > > >> +         break;
> > > >>         default:
> > > >>           break;
> > > >>         }
> > > >> --
> > > >> 2.36.0
> > > >>
> > >
  

On Mon, May 2, 2022 at 10:57 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/2/22 10:51, Richard Biener wrote:
> > On Mon, May 2, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 5/2/22 10:09, Richard Biener wrote:
> >>> On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
> >>>>
> >>>> Hi.
> >>>>
> >>>> This in a new plug-in function that helps identifying compiler
> >>>> by a linker. Will be used in mold linker.
> >>>>
> >>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>>>
> >>>> Ready to be installed?
> >>>
> >>> It looks a bit backward to query the compiler (and even more so to
> >>> have a fixed set) from the linker.  What is this going to be used for?
> >>
> >> It's going to be used by mold, there are small behavior divergence
> >> in between LLVM and GCC, where one compiler closes provided file descriptors:
> >> https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550
> >
> > But that then seems to be a defect in the plugin API specification (or
> > one of the two implementations).  Instead of adding a new API entry
> > that defect should be fixed - both require changes in the actual plugin
> > specification.
>
> Well, the question is if it was specified somewhere or not. Plus there are released
> versions of both compilers and plug-ins, so it's not easily fixable :/

Well, without changing plug-ins the new version callback doesn't exist
either.  The
problem of existing discrepancies is not fixable without heuristics in
the linker,
the _actual_ problem can of course be fixed but the solution is of course _not_
to make if (gcc) or if (llvm) but if then something like if (plugin
closed fd) or
specify the plugin has to or has not to close fds passed in
ld_plugin_input_file.

Richard.

> Martin
>
> >
> > Richard.
> >
> >>> If then this should probably receive a unformatted string the linker
> >>> has to decipher itself like "gcc (SUSE Linux) 7.5.0" or something
> >>> (what we produce with gcc --version), and clang would produce
> >>> "clang version 11.0.1" or so?
> >>
> >> Well, it brings its own set of problems such string parsing. Note that the plugin API
> >> currently provides something like:
> >>
> >> /* The version of gold being used, or -1 if not gold.  The number is
> >>    MAJOR * 100 + MINOR.  */
> >> static int gold_version = -1;
> >>
> >> So my suggestion is quite aligned with that.
> >>
> >> Martin
> >>
> >>>
> >>> Richard.
> >>>
> >>>> Thanks,
> >>>> Martin
> >>>>
> >>>> include/ChangeLog:
> >>>>
> >>>>         * plugin-api.h (enum ld_plugin_compiler): New enum.
> >>>>         (enum ld_plugin_version): New typedef.
> >>>>         (struct ld_plugin_tv): Add new field.
> >>>>
> >>>> lto-plugin/ChangeLog:
> >>>>
> >>>>         * lto-plugin.c (onload): Call ld_plugin_version.
> >>>> ---
> >>>>  include/plugin-api.h    | 18 +++++++++++++++++-
> >>>>  lto-plugin/lto-plugin.c |  4 ++++
> >>>>  2 files changed, 21 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/include/plugin-api.h b/include/plugin-api.h
> >>>> index 4e12c0320d6..4d0989028cc 100644
> >>>> --- a/include/plugin-api.h
> >>>> +++ b/include/plugin-api.h
> >>>> @@ -211,6 +211,13 @@ enum ld_plugin_symbol_section_kind
> >>>>    LDSSK_BSS
> >>>>  };
> >>>>
> >>>> +enum ld_plugin_compiler
> >>>> +{
> >>>> +  LDPC_UNKNOWN,
> >>>> +  LDPC_GCC,
> >>>> +  LDPC_LLVM
> >>>> +};
> >>>> +
> >>>>  /* How a symbol is resolved.  */
> >>>>
> >>>>  enum ld_plugin_symbol_resolution
> >>>> @@ -475,6 +482,13 @@ enum ld_plugin_status
> >>>>  (*ld_plugin_get_wrap_symbols) (uint64_t *num_symbols,
> >>>>                                 const char ***wrap_symbol_list);
> >>>>
> >>>> +/* The linker's interface for registering the "plugin_version" handler.
> >>>> +   This handler is called directly after onload and provides compiler
> >>>> +   and its number version (MAJOR * 1000 + MINOR).  */
> >>>> +
> >>>> +typedef
> >>>> +void (*ld_plugin_version) (enum ld_plugin_compiler compiler, int version);
> >>>> +
> >>>>  enum ld_plugin_level
> >>>>  {
> >>>>    LDPL_INFO,
> >>>> @@ -520,7 +534,8 @@ enum ld_plugin_tag
> >>>>    LDPT_GET_INPUT_SECTION_SIZE = 30,
> >>>>    LDPT_REGISTER_NEW_INPUT_HOOK = 31,
> >>>>    LDPT_GET_WRAP_SYMBOLS = 32,
> >>>> -  LDPT_ADD_SYMBOLS_V2 = 33
> >>>> +  LDPT_ADD_SYMBOLS_V2 = 33,
> >>>> +  LDPT_PLUGIN_VERSION = 34,
> >>>>  };
> >>>>
> >>>>  /* The plugin transfer vector.  */
> >>>> @@ -556,6 +571,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_version tv_plugin_version;
> >>>>    } tv_u;
> >>>>  };
> >>>>
> >>>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> >>>> index 7a1c77812f6..698a0617ca7 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.  */
> >>>> @@ -1466,6 +1467,9 @@ 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_PLUGIN_VERSION:
> >>>> +         p->tv_u.tv_plugin_version (LDPC_GCC, GCC_VERSION);
> >>>> +         break;
> >>>>         default:
> >>>>           break;
> >>>>         }
> >>>> --
> >>>> 2.36.0
> >>>>
> >>
>
  

On Mon, May 2, 2022 at 11:00 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, May 2, 2022 at 10:57 AM Martin Liška <mliska@suse.cz> wrote:
> >
> > On 5/2/22 10:51, Richard Biener wrote:
> > > On Mon, May 2, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
> > >>
> > >> On 5/2/22 10:09, Richard Biener wrote:
> > >>> On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
> > >>>>
> > >>>> Hi.
> > >>>>
> > >>>> This in a new plug-in function that helps identifying compiler
> > >>>> by a linker. Will be used in mold linker.
> > >>>>
> > >>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > >>>>
> > >>>> Ready to be installed?
> > >>>
> > >>> It looks a bit backward to query the compiler (and even more so to
> > >>> have a fixed set) from the linker.  What is this going to be used for?
> > >>
> > >> It's going to be used by mold, there are small behavior divergence
> > >> in between LLVM and GCC, where one compiler closes provided file descriptors:
> > >> https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550
> > >
> > > But that then seems to be a defect in the plugin API specification (or
> > > one of the two implementations).  Instead of adding a new API entry
> > > that defect should be fixed - both require changes in the actual plugin
> > > specification.
> >
> > Well, the question is if it was specified somewhere or not. Plus there are released
> > versions of both compilers and plug-ins, so it's not easily fixable :/
>
> Well, without changing plug-ins the new version callback doesn't exist
> either.  The
> problem of existing discrepancies is not fixable without heuristics in
> the linker,
> the _actual_ problem can of course be fixed but the solution is of course _not_
> to make if (gcc) or if (llvm) but if then something like if (plugin
> closed fd) or
> specify the plugin has to or has not to close fds passed in
> ld_plugin_input_file.

In fact I think GCC is correct in its handling and LLVM would need to dup () the
file descriptor.  Was this issue raised with the LLVM folks?  I suspect the BFD
linker will also not work with LLVM due to this?

Richard.

> Richard.
>
> > Martin
> >
> > >
> > > Richard.
> > >
> > >>> If then this should probably receive a unformatted string the linker
> > >>> has to decipher itself like "gcc (SUSE Linux) 7.5.0" or something
> > >>> (what we produce with gcc --version), and clang would produce
> > >>> "clang version 11.0.1" or so?
> > >>
> > >> Well, it brings its own set of problems such string parsing. Note that the plugin API
> > >> currently provides something like:
> > >>
> > >> /* The version of gold being used, or -1 if not gold.  The number is
> > >>    MAJOR * 100 + MINOR.  */
> > >> static int gold_version = -1;
> > >>
> > >> So my suggestion is quite aligned with that.
> > >>
> > >> Martin
> > >>
> > >>>
> > >>> Richard.
> > >>>
> > >>>> Thanks,
> > >>>> Martin
> > >>>>
> > >>>> include/ChangeLog:
> > >>>>
> > >>>>         * plugin-api.h (enum ld_plugin_compiler): New enum.
> > >>>>         (enum ld_plugin_version): New typedef.
> > >>>>         (struct ld_plugin_tv): Add new field.
> > >>>>
> > >>>> lto-plugin/ChangeLog:
> > >>>>
> > >>>>         * lto-plugin.c (onload): Call ld_plugin_version.
> > >>>> ---
> > >>>>  include/plugin-api.h    | 18 +++++++++++++++++-
> > >>>>  lto-plugin/lto-plugin.c |  4 ++++
> > >>>>  2 files changed, 21 insertions(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/include/plugin-api.h b/include/plugin-api.h
> > >>>> index 4e12c0320d6..4d0989028cc 100644
> > >>>> --- a/include/plugin-api.h
> > >>>> +++ b/include/plugin-api.h
> > >>>> @@ -211,6 +211,13 @@ enum ld_plugin_symbol_section_kind
> > >>>>    LDSSK_BSS
> > >>>>  };
> > >>>>
> > >>>> +enum ld_plugin_compiler
> > >>>> +{
> > >>>> +  LDPC_UNKNOWN,
> > >>>> +  LDPC_GCC,
> > >>>> +  LDPC_LLVM
> > >>>> +};
> > >>>> +
> > >>>>  /* How a symbol is resolved.  */
> > >>>>
> > >>>>  enum ld_plugin_symbol_resolution
> > >>>> @@ -475,6 +482,13 @@ enum ld_plugin_status
> > >>>>  (*ld_plugin_get_wrap_symbols) (uint64_t *num_symbols,
> > >>>>                                 const char ***wrap_symbol_list);
> > >>>>
> > >>>> +/* The linker's interface for registering the "plugin_version" handler.
> > >>>> +   This handler is called directly after onload and provides compiler
> > >>>> +   and its number version (MAJOR * 1000 + MINOR).  */
> > >>>> +
> > >>>> +typedef
> > >>>> +void (*ld_plugin_version) (enum ld_plugin_compiler compiler, int version);
> > >>>> +
> > >>>>  enum ld_plugin_level
> > >>>>  {
> > >>>>    LDPL_INFO,
> > >>>> @@ -520,7 +534,8 @@ enum ld_plugin_tag
> > >>>>    LDPT_GET_INPUT_SECTION_SIZE = 30,
> > >>>>    LDPT_REGISTER_NEW_INPUT_HOOK = 31,
> > >>>>    LDPT_GET_WRAP_SYMBOLS = 32,
> > >>>> -  LDPT_ADD_SYMBOLS_V2 = 33
> > >>>> +  LDPT_ADD_SYMBOLS_V2 = 33,
> > >>>> +  LDPT_PLUGIN_VERSION = 34,
> > >>>>  };
> > >>>>
> > >>>>  /* The plugin transfer vector.  */
> > >>>> @@ -556,6 +571,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_version tv_plugin_version;
> > >>>>    } tv_u;
> > >>>>  };
> > >>>>
> > >>>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> > >>>> index 7a1c77812f6..698a0617ca7 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.  */
> > >>>> @@ -1466,6 +1467,9 @@ 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_PLUGIN_VERSION:
> > >>>> +         p->tv_u.tv_plugin_version (LDPC_GCC, GCC_VERSION);
> > >>>> +         break;
> > >>>>         default:
> > >>>>           break;
> > >>>>         }
> > >>>> --
> > >>>> 2.36.0
> > >>>>
> > >>
> >
  

On 5/2/22 11:00, Richard Biener wrote:
> On Mon, May 2, 2022 at 10:57 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 5/2/22 10:51, Richard Biener wrote:
>>> On Mon, May 2, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> On 5/2/22 10:09, Richard Biener wrote:
>>>>> On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
>>>>>>
>>>>>> Hi.
>>>>>>
>>>>>> This in a new plug-in function that helps identifying compiler
>>>>>> by a linker. Will be used in mold linker.
>>>>>>
>>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>>
>>>>>> Ready to be installed?
>>>>>
>>>>> It looks a bit backward to query the compiler (and even more so to
>>>>> have a fixed set) from the linker.  What is this going to be used for?
>>>>
>>>> It's going to be used by mold, there are small behavior divergence
>>>> in between LLVM and GCC, where one compiler closes provided file descriptors:
>>>> https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550
>>>
>>> But that then seems to be a defect in the plugin API specification (or
>>> one of the two implementations).  Instead of adding a new API entry
>>> that defect should be fixed - both require changes in the actual plugin
>>> specification.
>>
>> Well, the question is if it was specified somewhere or not. Plus there are released
>> versions of both compilers and plug-ins, so it's not easily fixable :/
> 
> Well, without changing plug-ins the new version callback doesn't exist
> either.  The
> problem of existing discrepancies is not fixable without heuristics in
> the linker,
> the _actual_ problem can of course be fixed but the solution is of course _not_
> to make if (gcc) or if (llvm) but if then something like if (plugin
> closed fd) or

Yes, feature querying would be much better approach.

> specify the plugin has to or has not to close fds passed in
> ld_plugin_input_file.

Anyway, I don't want to over-engineer things here and I think the mold
linker can live with some heuristics.

That said, I don't insist on this patch.

Martin

> 
> Richard.
> 
>> Martin
>>
>>>
>>> Richard.
>>>
>>>>> If then this should probably receive a unformatted string the linker
>>>>> has to decipher itself like "gcc (SUSE Linux) 7.5.0" or something
>>>>> (what we produce with gcc --version), and clang would produce
>>>>> "clang version 11.0.1" or so?
>>>>
>>>> Well, it brings its own set of problems such string parsing. Note that the plugin API
>>>> currently provides something like:
>>>>
>>>> /* The version of gold being used, or -1 if not gold.  The number is
>>>>    MAJOR * 100 + MINOR.  */
>>>> static int gold_version = -1;
>>>>
>>>> So my suggestion is quite aligned with that.
>>>>
>>>> Martin
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Thanks,
>>>>>> Martin
>>>>>>
>>>>>> include/ChangeLog:
>>>>>>
>>>>>>         * plugin-api.h (enum ld_plugin_compiler): New enum.
>>>>>>         (enum ld_plugin_version): New typedef.
>>>>>>         (struct ld_plugin_tv): Add new field.
>>>>>>
>>>>>> lto-plugin/ChangeLog:
>>>>>>
>>>>>>         * lto-plugin.c (onload): Call ld_plugin_version.
>>>>>> ---
>>>>>>  include/plugin-api.h    | 18 +++++++++++++++++-
>>>>>>  lto-plugin/lto-plugin.c |  4 ++++
>>>>>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/plugin-api.h b/include/plugin-api.h
>>>>>> index 4e12c0320d6..4d0989028cc 100644
>>>>>> --- a/include/plugin-api.h
>>>>>> +++ b/include/plugin-api.h
>>>>>> @@ -211,6 +211,13 @@ enum ld_plugin_symbol_section_kind
>>>>>>    LDSSK_BSS
>>>>>>  };
>>>>>>
>>>>>> +enum ld_plugin_compiler
>>>>>> +{
>>>>>> +  LDPC_UNKNOWN,
>>>>>> +  LDPC_GCC,
>>>>>> +  LDPC_LLVM
>>>>>> +};
>>>>>> +
>>>>>>  /* How a symbol is resolved.  */
>>>>>>
>>>>>>  enum ld_plugin_symbol_resolution
>>>>>> @@ -475,6 +482,13 @@ enum ld_plugin_status
>>>>>>  (*ld_plugin_get_wrap_symbols) (uint64_t *num_symbols,
>>>>>>                                 const char ***wrap_symbol_list);
>>>>>>
>>>>>> +/* The linker's interface for registering the "plugin_version" handler.
>>>>>> +   This handler is called directly after onload and provides compiler
>>>>>> +   and its number version (MAJOR * 1000 + MINOR).  */
>>>>>> +
>>>>>> +typedef
>>>>>> +void (*ld_plugin_version) (enum ld_plugin_compiler compiler, int version);
>>>>>> +
>>>>>>  enum ld_plugin_level
>>>>>>  {
>>>>>>    LDPL_INFO,
>>>>>> @@ -520,7 +534,8 @@ enum ld_plugin_tag
>>>>>>    LDPT_GET_INPUT_SECTION_SIZE = 30,
>>>>>>    LDPT_REGISTER_NEW_INPUT_HOOK = 31,
>>>>>>    LDPT_GET_WRAP_SYMBOLS = 32,
>>>>>> -  LDPT_ADD_SYMBOLS_V2 = 33
>>>>>> +  LDPT_ADD_SYMBOLS_V2 = 33,
>>>>>> +  LDPT_PLUGIN_VERSION = 34,
>>>>>>  };
>>>>>>
>>>>>>  /* The plugin transfer vector.  */
>>>>>> @@ -556,6 +571,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_version tv_plugin_version;
>>>>>>    } tv_u;
>>>>>>  };
>>>>>>
>>>>>> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
>>>>>> index 7a1c77812f6..698a0617ca7 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.  */
>>>>>> @@ -1466,6 +1467,9 @@ 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_PLUGIN_VERSION:
>>>>>> +         p->tv_u.tv_plugin_version (LDPC_GCC, GCC_VERSION);
>>>>>> +         break;
>>>>>>         default:
>>>>>>           break;
>>>>>>         }
>>>>>> --
>>>>>> 2.36.0
>>>>>>
>>>>
>>
  

> On Mon, May 2, 2022 at 10:51 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Mon, May 2, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
> > >
> > > On 5/2/22 10:09, Richard Biener wrote:
> > > > On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
> > > >>
> > > >> Hi.
> > > >>
> > > >> This in a new plug-in function that helps identifying compiler
> > > >> by a linker. Will be used in mold linker.
> > > >>
> > > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > > >>
> > > >> Ready to be installed?
> > > >
> > > > It looks a bit backward to query the compiler (and even more so to
> > > > have a fixed set) from the linker.  What is this going to be used for?
> > >
> > > It's going to be used by mold, there are small behavior divergence
> > > in between LLVM and GCC, where one compiler closes provided file descriptors:
> > > https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550
> >
> > But that then seems to be a defect in the plugin API specification (or
> > one of the two implementations).  Instead of adding a new API entry
> > that defect should be fixed - both require changes in the actual plugin
> > specification.
> 
> implementation of course.

I agree that implementation should be fixed.  The version query however
makes sense to me.  It makes it possible for linker to work around bugs of
older compilers while implementing features that in general rely on
corrected behaviour. 

I however also find bit odd to have fixed list gcc and llvm.  The plugin
API (when implemented without bugs) should be compler independent.  In
this case it would make more sense to have string for compiler name.
I think Martin was following existing API for querying linker type
which has same oddity and uses enum to represent known linkers
supporting the plugin API?

Honza
  

On 5/2/22 11:03, Richard Biener wrote:
> In fact I think GCC is correct in its handling and LLVM would need to dup () the
> file descriptor.  Was this issue raised with the LLVM folks?

I don't know here.

> I suspect the BFD
> linker will also not work with LLVM due to this?

I'm not sure BFD is supported by LLVM:
https://llvm.org/docs/GoldPlugin.html

They explicitly name it Gold plugin.

Martin
  

On Mon, May 2, 2022 at 11:06 AM Jan Hubicka <hubicka@kam.mff.cuni.cz> wrote:
>
> > On Mon, May 2, 2022 at 10:51 AM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Mon, May 2, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
> > > >
> > > > On 5/2/22 10:09, Richard Biener wrote:
> > > > > On Mon, May 2, 2022 at 9:52 AM Martin Liška <mliska@suse.cz> wrote:
> > > > >>
> > > > >> Hi.
> > > > >>
> > > > >> This in a new plug-in function that helps identifying compiler
> > > > >> by a linker. Will be used in mold linker.
> > > > >>
> > > > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > > > >>
> > > > >> Ready to be installed?
> > > > >
> > > > > It looks a bit backward to query the compiler (and even more so to
> > > > > have a fixed set) from the linker.  What is this going to be used for?
> > > >
> > > > It's going to be used by mold, there are small behavior divergence
> > > > in between LLVM and GCC, where one compiler closes provided file descriptors:
> > > > https://github.com/rui314/mold/blob/a15f3875269b575d024725590ae3ef87b611b8d8/elf/lto.cc#L545-L550
> > >
> > > But that then seems to be a defect in the plugin API specification (or
> > > one of the two implementations).  Instead of adding a new API entry
> > > that defect should be fixed - both require changes in the actual plugin
> > > specification.
> >
> > implementation of course.
>
> I agree that implementation should be fixed.  The version query however
> makes sense to me.  It makes it possible for linker to work around bugs of
> older compilers while implementing features that in general rely on
> corrected behaviour.
>
> I however also find bit odd to have fixed list gcc and llvm.  The plugin
> API (when implemented without bugs) should be compler independent.  In
> this case it would make more sense to have string for compiler name.

OK, though this doesn't help for bugs in the tool chain that does not yet
implement this version hook so the very reason for the hook is yet
unknown bugs.  It's then also tempting to never fix the actual bugs
but rely on an ever growing list of version/bug workarounds in linkers :/

> I think Martin was following existing API for querying linker type
> which has same oddity and uses enum to represent known linkers
> supporting the plugin API?
>
> Honza
  

On Mon, May 2, 2022 at 11:08 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/2/22 11:03, Richard Biener wrote:
> > In fact I think GCC is correct in its handling and LLVM would need to dup () the
> > file descriptor.  Was this issue raised with the LLVM folks?
>
> I don't know here.
>
> > I suspect the BFD
> > linker will also not work with LLVM due to this?
>
> I'm not sure BFD is supported by LLVM:
> https://llvm.org/docs/GoldPlugin.html
>
> They explicitly name it Gold plugin.

BFD does

  if (plugin_call_claim_file (&file, &claimed))
    einfo (_("%F%P: %s: plugin reported error claiming file\n"),
           plugin_error_plugin ());

  if (input->fd != -1
      && (!claimed || !bfd_plugin_target_p (ibfd->xvec)))
    {
      /* FIXME: fd belongs to us, not the plugin.  GCC plugin, which
         doesn't need fd after plugin_call_claim_file, doesn't use
         BFD plugin target vector.  Since GCC plugin doesn't call
         release_input_file, we close it here.  LLVM plugin, which
         needs fd after plugin_call_claim_file and calls
         release_input_file after it is done, uses BFD plugin target
         vector.  This scheme doesn't work when a plugin needs fd and
         doesn't use BFD plugin target vector neither.  */
      release_plugin_file_descriptor (input);
    }

so it knows about the problem.  The above suggests that the plugin
could set ->fd to -1 ...

Richard.

>
> Martin
>
  

On 5/2/22 11:14, Richard Biener wrote:
> On Mon, May 2, 2022 at 11:08 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 5/2/22 11:03, Richard Biener wrote:
>>> In fact I think GCC is correct in its handling and LLVM would need to dup () the
>>> file descriptor.  Was this issue raised with the LLVM folks?
>>
>> I don't know here.
>>
>>> I suspect the BFD
>>> linker will also not work with LLVM due to this?
>>
>> I'm not sure BFD is supported by LLVM:
>> https://llvm.org/docs/GoldPlugin.html
>>
>> They explicitly name it Gold plugin.
> 
> BFD does
> 
>   if (plugin_call_claim_file (&file, &claimed))
>     einfo (_("%F%P: %s: plugin reported error claiming file\n"),
>            plugin_error_plugin ());
> 
>   if (input->fd != -1
>       && (!claimed || !bfd_plugin_target_p (ibfd->xvec)))
>     {
>       /* FIXME: fd belongs to us, not the plugin.  GCC plugin, which
>          doesn't need fd after plugin_call_claim_file, doesn't use
>          BFD plugin target vector.  Since GCC plugin doesn't call
>          release_input_file, we close it here.  LLVM plugin, which
>          needs fd after plugin_call_claim_file and calls
>          release_input_file after it is done, uses BFD plugin target
>          vector.  This scheme doesn't work when a plugin needs fd and
>          doesn't use BFD plugin target vector neither.  */
>       release_plugin_file_descriptor (input);
>     }
> 
> so it knows about the problem.  The above suggests that the plugin
> could set ->fd to -1 ...

That would work. So we can add LDPT_REGISTER_CLAIM_FILE_HOOK_V2
hook that will release fd based on if claim_file_v2 resets it to -1 or not.

Does it worth implementing?

Cheers,
Martin

> 
> Richard.
> 
>>
>> Martin
>>
  

On Mon, May 2, 2022 at 11:27 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/2/22 11:14, Richard Biener wrote:
> > On Mon, May 2, 2022 at 11:08 AM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 5/2/22 11:03, Richard Biener wrote:
> >>> In fact I think GCC is correct in its handling and LLVM would need to dup () the
> >>> file descriptor.  Was this issue raised with the LLVM folks?
> >>
> >> I don't know here.
> >>
> >>> I suspect the BFD
> >>> linker will also not work with LLVM due to this?
> >>
> >> I'm not sure BFD is supported by LLVM:
> >> https://llvm.org/docs/GoldPlugin.html
> >>
> >> They explicitly name it Gold plugin.
> >
> > BFD does
> >
> >   if (plugin_call_claim_file (&file, &claimed))
> >     einfo (_("%F%P: %s: plugin reported error claiming file\n"),
> >            plugin_error_plugin ());
> >
> >   if (input->fd != -1
> >       && (!claimed || !bfd_plugin_target_p (ibfd->xvec)))
> >     {
> >       /* FIXME: fd belongs to us, not the plugin.  GCC plugin, which
> >          doesn't need fd after plugin_call_claim_file, doesn't use
> >          BFD plugin target vector.  Since GCC plugin doesn't call
> >          release_input_file, we close it here.  LLVM plugin, which
> >          needs fd after plugin_call_claim_file and calls
> >          release_input_file after it is done, uses BFD plugin target
> >          vector.  This scheme doesn't work when a plugin needs fd and
> >          doesn't use BFD plugin target vector neither.  */
> >       release_plugin_file_descriptor (input);
> >     }
> >
> > so it knows about the problem.  The above suggests that the plugin
> > could set ->fd to -1 ...
>
> That would work. So we can add LDPT_REGISTER_CLAIM_FILE_HOOK_V2
> hook that will release fd based on if claim_file_v2 resets it to -1 or not.
>
> Does it worth implementing?

I'd say the folks to ask are the binutils, mold and lld linker plus of course
LLVM which would need to adjust its handling.  It would be also possible
to add a int *claimed_fd argument or just explicitely document that
the file descriptor belongs to the linker and thus just LLVM needs to be
fixed to dup() it when it stores it away.

Linkers will need heuristics anyway for older plugins and I think
LLVM could just behave here.

Richard.

>
> Cheers,
> Martin
>
> >
> > Richard.
> >
> >>
> >> Martin
> >>
>
  

On Mon, May 2, 2022 at 11:36 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, May 2, 2022 at 11:27 AM Martin Liška <mliska@suse.cz> wrote:
> >
> > On 5/2/22 11:14, Richard Biener wrote:
> > > On Mon, May 2, 2022 at 11:08 AM Martin Liška <mliska@suse.cz> wrote:
> > >>
> > >> On 5/2/22 11:03, Richard Biener wrote:
> > >>> In fact I think GCC is correct in its handling and LLVM would need to dup () the
> > >>> file descriptor.  Was this issue raised with the LLVM folks?
> > >>
> > >> I don't know here.
> > >>
> > >>> I suspect the BFD
> > >>> linker will also not work with LLVM due to this?
> > >>
> > >> I'm not sure BFD is supported by LLVM:
> > >> https://llvm.org/docs/GoldPlugin.html
> > >>
> > >> They explicitly name it Gold plugin.
> > >
> > > BFD does
> > >
> > >   if (plugin_call_claim_file (&file, &claimed))
> > >     einfo (_("%F%P: %s: plugin reported error claiming file\n"),
> > >            plugin_error_plugin ());
> > >
> > >   if (input->fd != -1
> > >       && (!claimed || !bfd_plugin_target_p (ibfd->xvec)))
> > >     {
> > >       /* FIXME: fd belongs to us, not the plugin.  GCC plugin, which
> > >          doesn't need fd after plugin_call_claim_file, doesn't use
> > >          BFD plugin target vector.  Since GCC plugin doesn't call
> > >          release_input_file, we close it here.  LLVM plugin, which
> > >          needs fd after plugin_call_claim_file and calls
> > >          release_input_file after it is done, uses BFD plugin target
> > >          vector.  This scheme doesn't work when a plugin needs fd and
> > >          doesn't use BFD plugin target vector neither.  */
> > >       release_plugin_file_descriptor (input);
> > >     }
> > >
> > > so it knows about the problem.  The above suggests that the plugin
> > > could set ->fd to -1 ...
> >
> > That would work. So we can add LDPT_REGISTER_CLAIM_FILE_HOOK_V2
> > hook that will release fd based on if claim_file_v2 resets it to -1 or not.
> >
> > Does it worth implementing?
>
> I'd say the folks to ask are the binutils, mold and lld linker plus of course
> LLVM which would need to adjust its handling.  It would be also possible
> to add a int *claimed_fd argument or just explicitely document that
> the file descriptor belongs to the linker and thus just LLVM needs to be
> fixed to dup() it when it stores it away.
>
> Linkers will need heuristics anyway for older plugins and I think
> LLVM could just behave here.

Btw, binutils f4b78d1898203363e7f551497b6231d0f891d6f9
more accurately outlines the issue (though I admit I don't
fully understand the difference) and it hints at the linkers more
accurately need to track where the FD was obtained through.

Richard.

> Richard.
>
> >
> > Cheers,
> > Martin
> >
> > >
> > > Richard.
> > >
> > >>
> > >> Martin
> > >>
> >
  

On 5/2/22 11:36, Richard Biener wrote:
> Linkers will need heuristics anyway for older plugins and I think
> LLVM could just behave here.

Leaving that for the future, I may return to it. Right now, the benefit
is pretty low to me.

Martin