| Message ID | 82f5cda2-97f4-039e-5094-c528b220eb78@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Check endianess detection. | expand |
On Mon, Mar 23, 2020 at 10:25:32AM +0100, Martin Liška wrote: > Hi. > > As seen in the PR, sparc64 LTO test-suite fails due to missing > definition of __BIG_ENDIAN__ macro. That said, I updated the > endianess detection to use __BYTE_ORDER__. > > I tested the detection on x86_64-linux-gnu, ppc64-linux-gnu and > lto.exp testsuite survives on a sparc64-linux machine. Those are GCC specific macros, are you sure plugin-api.h will be always compiled just with GCC and no other compiler? You can use them but should be prepared for some fallback (e.g. endian.h, whatever else). And there is also PDP endian... Jakub
On 3/23/20 10:43 AM, Jakub Jelinek wrote: > On Mon, Mar 23, 2020 at 10:25:32AM +0100, Martin Liška wrote: >> Hi. >> >> As seen in the PR, sparc64 LTO test-suite fails due to missing >> definition of __BIG_ENDIAN__ macro. That said, I updated the >> endianess detection to use __BYTE_ORDER__. >> >> I tested the detection on x86_64-linux-gnu, ppc64-linux-gnu and >> lto.exp testsuite survives on a sparc64-linux machine. > > Those are GCC specific macros, are you sure plugin-api.h will be always > compiled just with GCC and no other compiler? And Clang supports that. The header file is used for GCC LTO plug-in (which is like a run-time library) and then it's consumed by binutils. So I don't how much portable it should be? > You can use them but should be prepared for some fallback (e.g. endian.h, > whatever else). > And there is also PDP endian... Huh, are we talking about something so complex like: https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/int_endianness.h Btw. do we force our run-time libraries to be built with GCC? Thanks, Martin > > Jakub >
On Mon, Mar 23, 2020 at 11:00:21AM +0100, Martin Liška wrote: > On 3/23/20 10:43 AM, Jakub Jelinek wrote: > > On Mon, Mar 23, 2020 at 10:25:32AM +0100, Martin Liška wrote: > > > Hi. > > > > > > As seen in the PR, sparc64 LTO test-suite fails due to missing > > > definition of __BIG_ENDIAN__ macro. That said, I updated the > > > endianess detection to use __BYTE_ORDER__. > > > > > > I tested the detection on x86_64-linux-gnu, ppc64-linux-gnu and > > > lto.exp testsuite survives on a sparc64-linux machine. > > > > Those are GCC specific macros, are you sure plugin-api.h will be always > > compiled just with GCC and no other compiler? > > And Clang supports that. The header file is used for GCC LTO plug-in > (which is like a run-time library) and then it's consumed by binutils. > So I don't how much portable it should be? GCC only supports that since GCC 4.6 and Clang copied that from that. If it is only used in the LTO plugin and nothing else, I guess you can rely in it being compiled by GCC only, but if it is e.g. used by binutils itself too, then no. > > You can use them but should be prepared for some fallback (e.g. endian.h, > > whatever else). > > And there is also PDP endian... > > Huh, are we talking about something so complex like: > https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/int_endianness.h I'd say even that is very simplified. E.g. on glibc there is <endian.h> with its macros, etc. > Btw. do we force our run-time libraries to be built with GCC? Some of our run-time libraries rely on being built by GCC, sure. But I thought include/ is shared with binutils and there we don't really say which compiler must be used to compile it. Jakub
On 3/23/20 11:10 AM, Jakub Jelinek wrote: > On Mon, Mar 23, 2020 at 11:00:21AM +0100, Martin Liška wrote: >> On 3/23/20 10:43 AM, Jakub Jelinek wrote: >>> On Mon, Mar 23, 2020 at 10:25:32AM +0100, Martin Liška wrote: >>>> Hi. >>>> >>>> As seen in the PR, sparc64 LTO test-suite fails due to missing >>>> definition of __BIG_ENDIAN__ macro. That said, I updated the >>>> endianess detection to use __BYTE_ORDER__. >>>> >>>> I tested the detection on x86_64-linux-gnu, ppc64-linux-gnu and >>>> lto.exp testsuite survives on a sparc64-linux machine. >>> >>> Those are GCC specific macros, are you sure plugin-api.h will be always >>> compiled just with GCC and no other compiler? >> >> And Clang supports that. The header file is used for GCC LTO plug-in >> (which is like a run-time library) and then it's consumed by binutils. >> So I don't how much portable it should be? > > GCC only supports that since GCC 4.6 and Clang copied that from that. > If it is only used in the LTO plugin and nothing else, I guess you can rely > in it being compiled by GCC only, but if it is e.g. used by binutils itself > too, then no. All right... > >>> You can use them but should be prepared for some fallback (e.g. endian.h, >>> whatever else). >>> And there is also PDP endian... >> >> Huh, are we talking about something so complex like: >> https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/int_endianness.h > > I'd say even that is very simplified. E.g. on glibc there is <endian.h> > with its macros, etc. > >> Btw. do we force our run-time libraries to be built with GCC? > > Some of our run-time libraries rely on being built by GCC, sure. > But I thought include/ is shared with binutils and there we don't really say > which compiler must be used to compile it. ... and can we then rely on configure detection of the endianess done by bfd and gold: gold/config.h:#define GOLD_DEFAULT_BIG_ENDIAN false bfd/PORTING: TARGET_IS_BIG_ENDIAN_P Should be defined if <target> is big-endian. ? Martin > > Jakub >
On Mon, Mar 23, 2020 at 11:28:00AM +0100, Martin Liška wrote: > > > > You can use them but should be prepared for some fallback (e.g. endian.h, > > > > whatever else). > > > > And there is also PDP endian... > > > > > > Huh, are we talking about something so complex like: > > > https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/int_endianness.h > > > > I'd say even that is very simplified. E.g. on glibc there is <endian.h> > > with its macros, etc. > > > > > Btw. do we force our run-time libraries to be built with GCC? > > > > Some of our run-time libraries rely on being built by GCC, sure. > > But I thought include/ is shared with binutils and there we don't really say > > which compiler must be used to compile it. > > ... and can we then rely on configure detection of the endianess done by bfd and gold: > > gold/config.h:#define GOLD_DEFAULT_BIG_ENDIAN false > > bfd/PORTING: > TARGET_IS_BIG_ENDIAN_P > Should be defined if <target> is big-endian. I don't think so. That is about the target, but you care about the host. Wouldn't it be much easier not to do this and simply use macros for bits from the full 32-bit value (and use shifts)? Jakub
On 3/23/20 11:35 AM, Jakub Jelinek wrote: > I don't think so. That is about the target, but you care about the host. I see. > Wouldn't it be much easier not to do this and simply use macros for bits > from the full 32-bit value (and use shifts)? That would make the current small hack even bigger. Note that plugin-api.h provides reasonable ways how to extend the API. That said, I incline to implement lto_plugin_symbols_v2 and use it in the corresponding function add_symbols_v2. @Richi: Can you please express your opinion? Thanks, Martin
Hi. There's a patch draft that will do the proper versioning of API. It's a subject for discussion. Martin
On March 23, 2020 1:43:30 PM GMT+01:00, "Martin Liška" <mliska@suse.cz> wrote: >On 3/23/20 11:35 AM, Jakub Jelinek wrote: >> I don't think so. That is about the target, but you care about the >host. > >I see. > >> Wouldn't it be much easier not to do this and simply use macros for >bits >> from the full 32-bit value (and use shifts)? > >That would make the current small hack even bigger. Note that >plugin-api.h >provides reasonable ways how to extend the API. That said, I incline >to implement lto_plugin_symbols_v2 and use it in the corresponding >function >add_symbols_v2. > >@Richi: Can you please express your opinion? Since lto-plugin is a host tool it is compiled by the system compiler which means we either have to properly detect host endianess or do shifting, leaving the layout unchanged. Richard. >Thanks, >Martin
On Mon, Mar 23, 2020 at 4:07 PM Martin Liška <mliska@suse.cz> wrote: > > Hi. > > There's a patch draft that will do the proper versioning of API. Would sth like this be preferred on the binutils side or would splitting up the 'def' field via shift&masking better? @@ -87,25 +87,30 @@ struct ld_plugin_symbol { char *name; char *version; - /* This is for compatibility with older ABIs. The older ABI defined - only 'def' field. */ -#ifdef __BIG_ENDIAN__ - char unused; ... + int def; int visibility; uint64_t size; char *comdat_key; int resolution; }; +/* A symbol belonging to an input file managed by the plugin library + (version 2). */ + +struct ld_plugin_symbol_v2 +{ + char *name; + char *version; + int def; + int visibility; + uint64_t size; + char *comdat_key; + int resolution; + char symbol_type; + char section_kind; + uint16_t unused; +}; I think for ease of use you should do struct ld_plugin_symbol_v2 { ld_plugin_symbol v1_data; char symbol_type; char section_kind; uint16_t unused; } also please avoid 'char', either use uint8_t or at least unsigned char. I guess since you're extending the type anyway using two plain 'int' would better follow the rest of the data structure. > It's a subject for discussion. - status = add_symbols_v2 (file->handle, lto_file.symtab.nsyms, - lto_file.symtab.syms); + { + /* Merge symtab::syms and symtab::syms_v2 data. */ + for (unsigned int i = 0; i < lto_file.symtab.nsyms; i++) + { I think lto-plugin should internally always parse into the "full" format, using defaults for entries not in IL files. Only the interfacing with the linker should then decide. > > Martin
On Mon, Mar 23, 2020 at 8:39 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Mon, Mar 23, 2020 at 4:07 PM Martin Liška <mliska@suse.cz> wrote: > > > > Hi. > > > > There's a patch draft that will do the proper versioning of API. > > Would sth like this be preferred on the binutils side or would > splitting up the 'def' field via shift&masking better? > > @@ -87,25 +87,30 @@ struct ld_plugin_symbol > { > char *name; > char *version; > - /* This is for compatibility with older ABIs. The older ABI defined > - only 'def' field. */ > -#ifdef __BIG_ENDIAN__ > - char unused; > ... > + int def; > int visibility; > uint64_t size; > char *comdat_key; > int resolution; > }; > > +/* A symbol belonging to an input file managed by the plugin library > + (version 2). */ > + > +struct ld_plugin_symbol_v2 > +{ > + char *name; > + char *version; > + int def; > + int visibility; > + uint64_t size; > + char *comdat_key; > + int resolution; > + char symbol_type; > + char section_kind; > + uint16_t unused; > +}; > > I think for ease of use you should do > > struct ld_plugin_symbol_v2 > { > ld_plugin_symbol v1_data; > char symbol_type; > char section_kind; > uint16_t unused; > } > > also please avoid 'char', either use uint8_t or > at least unsigned char. I guess since you're extending > the type anyway using two plain 'int' would better follow > the rest of the data structure. > > > It's a subject for discussion. > > - status = add_symbols_v2 (file->handle, lto_file.symtab.nsyms, > - lto_file.symtab.syms); > + { > + /* Merge symtab::syms and symtab::syms_v2 data. */ > + for (unsigned int i = 0; i < lto_file.symtab.nsyms; i++) > + { > > I think lto-plugin should internally always parse into the "full" format, > using defaults for entries not in IL files. Only the interfacing with > the linker should then decide. Can we use #if __has_include (<endian.h>) ... #ifdef __BYTE_ORDER #if __BYTE_ORDER == __BIG_ENDIAN ... We support V2 interface only if <endian.h> defines __BYTE_ORDER?
On 3/23/20 5:06 PM, H.J. Lu wrote: > #if __has_include (<endian.h>) > ... > #ifdef __BYTE_ORDER > #if __BYTE_ORDER == __BIG_ENDIAN > ... > > We support V2 interface only if <endian.h> defines __BYTE_ORDER? Right now we rely on __BIG_ENDIAN__ which is a weak endianess detection. So we either need a very robust endianess detection (as mention in this thread): https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/int_endianness.h or we'll come up with ld_plugin_symbol_v2 or we will shift values in 'int def' in order to support symbol_type and section_kind. What do you prefer H.J.? Thanks, Martin
On Mon, Mar 23, 2020 at 10:17 AM Martin Liška <mliska@suse.cz> wrote: > > On 3/23/20 5:06 PM, H.J. Lu wrote: > > #if __has_include (<endian.h>) > > ... > > #ifdef __BYTE_ORDER > > #if __BYTE_ORDER == __BIG_ENDIAN > > ... > > > > We support V2 interface only if <endian.h> defines __BYTE_ORDER? > > Right now we rely on __BIG_ENDIAN__ which is a weak endianess detection. > So we either need a very robust endianess detection (as mention in this thread): > https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/int_endianness.h I prefer this one. > or > > we'll come up with ld_plugin_symbol_v2 > > or > > we will shift values in 'int def' in order to support symbol_type and section_kind. > > What do you prefer H.J.? > Thanks, > Martin
All right, there's update endianess detection that should be robust enough. I've tested that on x86_64-linux-gnu and sparc64-linux. Ready for master? Thanks, Martin
On Tue, Mar 24, 2020 at 09:19:12AM +0100, Martin Liška wrote: > 2020-03-24 Martin Liska <mliska@suse.cz> > > PR lto/94249 > * plugin-api.h: Add more robust endianess detection. > --- > include/plugin-api.h | 43 +++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 41 insertions(+), 2 deletions(-) > > diff --git a/include/plugin-api.h b/include/plugin-api.h > index 673f136ce68..4a211d51f43 100644 > --- a/include/plugin-api.h > +++ b/include/plugin-api.h > @@ -42,6 +42,43 @@ extern "C" > { > #endif The location is incorrect, you don't want to include system headers inside explicit extern "C", so please move it a few lines above it. Furthermore, you don't have the glibc case with GCC < 4.6 handled, that needs something like: #if defined(__GLIBC__) || defined(__GNU_LIBRARY__) \ || defined(__ANDROID__) #include <endian.h> #if __BYTE_ORDER == __LITTLE_ENDIAN #define PLUGIN_LITTLE_ENDIAN 1 #elif __BYTE_ORDER == __BIG_ENDIAN #define PLUGIN_BIG_ENDIAN 1 ... (of course done only if __BYTE_ORDER__ and __ORDER_*_ENDIAN__ isn't defined). And, you don't handle PDP endian, while GCC does support pdp11-*, so IMNSHO you also need to detect PDP endian and use: #elif PLUGIN_PDP_ENDIAN == 1 char symbol_type; char def; char unused; char section_kind; Jakub
On 3/24/20 9:31 AM, Jakub Jelinek wrote: > On Tue, Mar 24, 2020 at 09:19:12AM +0100, Martin Liška wrote: >> 2020-03-24 Martin Liska <mliska@suse.cz> >> >> PR lto/94249 >> * plugin-api.h: Add more robust endianess detection. >> --- >> include/plugin-api.h | 43 +++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 41 insertions(+), 2 deletions(-) >> >> diff --git a/include/plugin-api.h b/include/plugin-api.h >> index 673f136ce68..4a211d51f43 100644 >> --- a/include/plugin-api.h >> +++ b/include/plugin-api.h >> @@ -42,6 +42,43 @@ extern "C" >> { >> #endif > > The location is incorrect, you don't want to include system headers > inside explicit extern "C", so please move it a few lines above it. > > Furthermore, you don't have the glibc case with GCC < 4.6 handled, that > needs something like: > #if defined(__GLIBC__) || defined(__GNU_LIBRARY__) \ > || defined(__ANDROID__) > #include <endian.h> > #if __BYTE_ORDER == __LITTLE_ENDIAN > #define PLUGIN_LITTLE_ENDIAN 1 > #elif __BYTE_ORDER == __BIG_ENDIAN > #define PLUGIN_BIG_ENDIAN 1 > ... > (of course done only if __BYTE_ORDER__ and __ORDER_*_ENDIAN__ isn't > defined). > > And, you don't handle PDP endian, while GCC does support pdp11-*, > so IMNSHO you also need to detect PDP endian and use: > > #elif PLUGIN_PDP_ENDIAN == 1 > char symbol_type; > char def; > char unused; > char section_kind; > > Jakub > Thank you Jakub for the review! There's updated patch that reflects that. Martin
Hi! So, assuming I'm on glibc with GCC 4.5 on powerpc64-linux. On Tue, Mar 24, 2020 at 09:49:42AM +0100, Martin Liška wrote: > +/* Detect endianess based on __BYTE_ORDER__ macro. */ > +#if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__) && \ > + defined(__ORDER_LITTLE_ENDIAN__) && defined(__ORDER_PDP_ENDIAN__) > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > +#define PLUGIN_LITTLE_ENDIAN 1 > +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > +#define PLUGIN_BIG_ENDIAN 1 > +#elif __BYTE_ORDER__ == __ORDER_PDP_ENDIAN__ > +#define PLUGIN_PDP_ENDIAN 1 > +#endif > +#else > +/* Older GCC releases (<4.6.0) can make detection from glibc macros. */ > +#if defined(__GLIBC__) || defined(__GNU_LIBRARY__) || defined(__ANDROID__) > +#include <endian.h> > +#if __BYTE_ORDER == __LITTLE_ENDIAN > +#define PLUGIN_LITTLE_ENDIAN 1 > +#elif __BYTE_ORDER == __BIG_ENDIAN > +#define PLUGIN_BIG_ENDIAN 1 > +#endif This will definePLUGIN_BIG_ENDIAN. > +#endif > +/* Include all necessary header files based on target. */ > +#if defined(__SVR4) && defined(__sun) > +#include <sys/byteorder.h> > +#endif > +#if defined(__FreeBSD__) || defined(__NetBSD__) || \ > + defined(__DragonFly__) || defined(__minix) > +#include <sys/endian.h> > +#endif > +#if defined(__OpenBSD__) > +#include <machine/endian.h> > +#endif The above headers will not be included. > +/* Detect endianess based on _BYTE_ORDER. */ > +#if _BYTE_ORDER == _LITTLE_ENDIAN So most likely _BYTE_ORDER and _LITTLE_ENDIAN macros will not be defined. Which means this will be handled as #if 0 == 0 and override the > +#define PLUGIN_LITTLE_ENDIAN 1 will define also PLUGIN_LITTLE_ENDIAN. > +#elif _BYTE_ORDER == _BIG_ENDIAN > +#define PLUGIN_BIG_ENDIAN 1 > +#endif > +/* Detect based on _WIN32. */ > +#if defined(_WIN32) > +#define PLUGIN_LITTLE_ENDIAN 1 > +#endif > +/* Fallback to __BIG_ENDIAN__ and __LITTLE_ENDIAN__ */ > +#ifdef __LITTLE_ENDIAN__ > +#define PLUGIN_LITTLE_ENDIAN 1 > +#endif > +#ifdef __BIG_ENDIAN__ > +#define PLUGIN_BIG_ENDIAN 1 > +#endif > +#endif and the above isn't really a fallback, because it isn't guarded with PLUGIN_*_ENDIAN not being defined yet. Jakub
On 3/24/20 10:18 AM, Jakub Jelinek wrote: > Hi! > > So, assuming I'm on glibc with GCC 4.5 on powerpc64-linux. > > On Tue, Mar 24, 2020 at 09:49:42AM +0100, Martin Liška wrote: >> +/* Detect endianess based on __BYTE_ORDER__ macro. */ >> +#if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__) && \ >> + defined(__ORDER_LITTLE_ENDIAN__) && defined(__ORDER_PDP_ENDIAN__) >> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ >> +#define PLUGIN_LITTLE_ENDIAN 1 >> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ >> +#define PLUGIN_BIG_ENDIAN 1 >> +#elif __BYTE_ORDER__ == __ORDER_PDP_ENDIAN__ >> +#define PLUGIN_PDP_ENDIAN 1 >> +#endif >> +#else >> +/* Older GCC releases (<4.6.0) can make detection from glibc macros. */ >> +#if defined(__GLIBC__) || defined(__GNU_LIBRARY__) || defined(__ANDROID__) >> +#include <endian.h> >> +#if __BYTE_ORDER == __LITTLE_ENDIAN >> +#define PLUGIN_LITTLE_ENDIAN 1 >> +#elif __BYTE_ORDER == __BIG_ENDIAN >> +#define PLUGIN_BIG_ENDIAN 1 >> +#endif > > This will definePLUGIN_BIG_ENDIAN. > >> +#endif >> +/* Include all necessary header files based on target. */ >> +#if defined(__SVR4) && defined(__sun) >> +#include <sys/byteorder.h> >> +#endif >> +#if defined(__FreeBSD__) || defined(__NetBSD__) || \ >> + defined(__DragonFly__) || defined(__minix) >> +#include <sys/endian.h> >> +#endif >> +#if defined(__OpenBSD__) >> +#include <machine/endian.h> >> +#endif > > The above headers will not be included. > >> +/* Detect endianess based on _BYTE_ORDER. */ >> +#if _BYTE_ORDER == _LITTLE_ENDIAN > > So most likely _BYTE_ORDER and _LITTLE_ENDIAN macros will not be defined. > Which means this will be handled as #if 0 == 0 and override the >> +#define PLUGIN_LITTLE_ENDIAN 1 > > will define also PLUGIN_LITTLE_ENDIAN. Ok, for being sure, I've wrapped all equality comparison with corresponding check of the LHS is defined. > >> +#elif _BYTE_ORDER == _BIG_ENDIAN >> +#define PLUGIN_BIG_ENDIAN 1 >> +#endif >> +/* Detect based on _WIN32. */ >> +#if defined(_WIN32) >> +#define PLUGIN_LITTLE_ENDIAN 1 >> +#endif >> +/* Fallback to __BIG_ENDIAN__ and __LITTLE_ENDIAN__ */ >> +#ifdef __LITTLE_ENDIAN__ >> +#define PLUGIN_LITTLE_ENDIAN 1 >> +#endif >> +#ifdef __BIG_ENDIAN__ >> +#define PLUGIN_BIG_ENDIAN 1 >> +#endif >> +#endif > > and the above isn't really a fallback, because it isn't guarded with > PLUGIN_*_ENDIAN not being defined yet. This comment has been also updated. Martin > > Jakub >
On Tue, Mar 24, 2020 at 11:32:32AM +0100, Martin Liška wrote: > >From 82b8731f304c734353c34ddaf1b1265341a90882 Mon Sep 17 00:00:00 2001 > From: Martin Liska <mliska@suse.cz> > Date: Tue, 24 Mar 2020 09:12:50 +0100 > Subject: [PATCH] Improve endianess detection. > > include/ChangeLog: > > 2020-03-24 Martin Liska <mliska@suse.cz> > > PR lto/94249 > * plugin-api.h: Add more robust endianess detection. Ok. Jakub
diff --git a/include/plugin-api.h b/include/plugin-api.h index 673f136ce68..5a45ce773f7 100644 --- a/include/plugin-api.h +++ b/include/plugin-api.h @@ -89,16 +89,18 @@ struct ld_plugin_symbol char *version; /* This is for compatibility with older ABIs. The older ABI defined only 'def' field. */ -#ifdef __BIG_ENDIAN__ +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ char unused; char section_kind; char symbol_type; char def; -#else +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ char def; char symbol_type; char section_kind; char unused; +#else +#error "Could not detect architecture endianess" #endif int visibility; uint64_t size;