Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 9:52 AM Martin Liška 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 >
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On 5/2/22 10:09, Richard Biener wrote: > On Mon, May 2, 2022 at 9:52 AM Martin Liška 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 >>
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 10:19 AM Martin Liška wrote: > > On 5/2/22 10:09, Richard Biener wrote: > > On Mon, May 2, 2022 at 9:52 AM Martin Liška 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 > >> >
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 10:51 AM Richard Biener wrote: > > On Mon, May 2, 2022 at 10:19 AM Martin Liška wrote: > > > > On 5/2/22 10:09, Richard Biener wrote: > > > On Mon, May 2, 2022 at 9:52 AM Martin Liška 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_ver
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On 5/2/22 10:51, Richard Biener wrote: > On Mon, May 2, 2022 at 10:19 AM Martin Liška wrote: >> >> On 5/2/22 10:09, Richard Biener wrote: >>> On Mon, May 2, 2022 at 9:52 AM Martin Liška 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 >>
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 10:54 AM Richard Biener wrote: > > On Mon, May 2, 2022 at 10:51 AM Richard Biener > wrote: > > > > On Mon, May 2, 2022 at 10:19 AM Martin Liška wrote: > > > > > > On 5/2/22 10:09, Richard Biener wrote: > > > > On Mon, May 2, 2022 at 9:52 AM Martin Liška 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 COP
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 10:57 AM Martin Liška wrote: > > On 5/2/22 10:51, Richard Biener wrote: > > On Mon, May 2, 2022 at 10:19 AM Martin Liška wrote: > >> > >> On 5/2/22 10:09, Richard Biener wrote: > >>> On Mon, May 2, 2022 at 9:52 AM Martin Liška 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 COPYIN
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 11:00 AM Richard Biener wrote: > > On Mon, May 2, 2022 at 10:57 AM Martin Liška wrote: > > > > On 5/2/22 10:51, Richard Biener wrote: > > > On Mon, May 2, 2022 at 10:19 AM Martin Liška wrote: > > >> > > >> On 5/2/22 10:09, Richard Biener wrote: > > >>> On Mon, May 2, 2022 at 9:52 AM Martin Liška 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 l
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On 5/2/22 11:00, Richard Biener wrote: > On Mon, May 2, 2022 at 10:57 AM Martin Liška wrote: >> >> On 5/2/22 10:51, Richard Biener wrote: >>> On Mon, May 2, 2022 at 10:19 AM Martin Liška wrote: On 5/2/22 10:09, Richard Biener wrote: > On Mon, May 2, 2022 at 9:52 AM Martin Liška 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;
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
> On Mon, May 2, 2022 at 10:51 AM Richard Biener > wrote: > > > > On Mon, May 2, 2022 at 10:19 AM Martin Liška wrote: > > > > > > On 5/2/22 10:09, Richard Biener wrote: > > > > On Mon, May 2, 2022 at 9:52 AM Martin Liška 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
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
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
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 11:06 AM Jan Hubicka wrote: > > > On Mon, May 2, 2022 at 10:51 AM Richard Biener > > wrote: > > > > > > On Mon, May 2, 2022 at 10:19 AM Martin Liška wrote: > > > > > > > > On 5/2/22 10:09, Richard Biener wrote: > > > > > On Mon, May 2, 2022 at 9:52 AM Martin Liška 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
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 11:08 AM Martin Liška 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 >
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On 5/2/22 11:14, Richard Biener wrote: > On Mon, May 2, 2022 at 11:08 AM Martin Liška 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 >>
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 11:27 AM Martin Liška wrote: > > On 5/2/22 11:14, Richard Biener wrote: > > On Mon, May 2, 2022 at 11:08 AM Martin Liška 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 > >> >
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
On Mon, May 2, 2022 at 11:36 AM Richard Biener wrote: > > On Mon, May 2, 2022 at 11:27 AM Martin Liška wrote: > > > > On 5/2/22 11:14, Richard Biener wrote: > > > On Mon, May 2, 2022 at 11:08 AM Martin Liška 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 > > >> > >
Re: [PATCH] LTO plugin: add ld_plugin_version callback.
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