On 5/16/22 12:28, Richard Biener wrote: > On Mon, May 16, 2022 at 11:58 AM Rui Ueyama wrote: >> >> Version handshaking is doable, but it feels like we are over-designing >> an API, given that the real providers of this plugin API are only GCC >> and LLVM and the users of the API are BFD ld, gold and mold. It is >> unlikely that we'll have dozens of more compilers or linkers in the >> near future. So, I personally prefer the following style >> >> if (!strcmp(plugin_compiler_name, "gcc") && plugin_major >= 12) >> >> than versioning various API-provided functions. It'll just work and be >> easy to understand. >> >> Besides that, even if we version GCC-provided plugin API functions, we >> still need a logic similar to the above to distinguish GCC from LLVM, >> as they behave slightly differently in various corner cases. We can't >> get rid of the heuristic version detection logic from the linker >> anyways. >> >> So, from the linker's point of view, exporting a compiler name and >> version numbers is enough. > > I agree that this might be convenient enough but the different behaviors > are because of inadequate documentation of the API - that's something > we should fix. And for this I think a plugin API version might help > as we can this way also handle your case of the need of querying > whether v2 will be used or not. > > I wouldn't go to enumerate past API versions - the version handshake > hook requirement alone makes that not so useful. Instead I'd require > everybody implementing the handshare hook implementing also all > other hooks defined at that point in time and set the version to 1. > > I'd eventually keep version 2 to indicate thread safety (of a part of the API). That seems reasonable to me. > > That said, I'm not opposed to add a "GCC 12.1" provider, maybe the > version handshake should be > > int api_version (int linker, const char **identifier); > > where the linker specifies the desired API version and passes an > identifier identifying itself ("mold 1.0") and it will get back the API > version the plugin intends to use plus an identifier of the plugin > ("GCC 12.1"). I've implemented first version of the patch, please take a look. Note there's a bit name collision of api_version with: /* The version of the API specification. */ enum ld_plugin_api_version { LD_PLUGIN_API_VERSION = 1 }; @Rui: Am I correct that you're interested in thread-safe claim_file? Is there any other function being called paralely? Thoughts? > > Richard. > >> >> >> On Mon, May 16, 2022 at 5:38 PM Martin Liška wrote: >>> >>> On 5/16/22 11:25, Jan Hubicka via Gcc-patches wrote: >>>>> >>>>> Sure having a 'plugin was compiled from sources of the GCC N.M compiler' >>>>> is useful if bugs are discovered in old versions that you by definition cannot >>>>> fix but can apply workarounds to. Note the actual compiler used might still >>>>> differ. Note that still isn't clean API documentation / extension of the plugin >>>>> API itself. As of thread safety we can either add a claim_file_v2_hook >>>>> or try to do broader-level versioning of the API with a new api_version >>>>> hook which could also specify that with say version 2 the plugin will >>>>> not use get_symbols_v2 but only newer, etc. The linker would announce >>>>> the API version it likes to use and the plugin would return the >>>>> (closest) version >>>>> it can handle. A v2 could then also specify that claim_file needs to be >>>> >>>> Yep, I think having the API version handshake is quite reasonable way to >>>> go as the _v2,_v3 symbols seems already getting bit out of hand and we >>>> essentially have 3 revisions of API to think of >>>> (first adding LDPR_PREVAILING_DEF_IRONLY_EXP, second adding >>>> GET_SYMBOLS_V3 and now we plan third adding thread safety and solving >>>> the file handler problems) >>> >>> How should be design such a version handshake? >>> >>>> >>>>> thread safe, or that cleanup should allow a followup onload again with >>>>> a state identical to after dlopen, etc. >>>>> >>>>> Is there an API document besides the header itself somewhere? >>>> >>>> There is https://gcc.gnu.org/wiki/whopr/driver >>>> I wonder if this can't be moved into more official looking place since >>>> it looks like it is internal to GCC WHOPR implementation this way. >>> >>> We can likely add it here: >>> https://gcc.gnu.org/onlinedocs/gccint/LTO.html#LTO >>> >>> What do you think? I can port it. >>> >>> Martin >>> >>>> >>>> Honza >>>>> >>>>> Richard. >>>