diff mbox series

Check endianess detection.

Message ID 82f5cda2-97f4-039e-5094-c528b220eb78@suse.cz
State New
Headers show
Series Check endianess detection. | expand

Commit Message

Martin Liška March 23, 2020, 9:25 a.m. UTC
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.

Ready to be installed?
Thanks,
Martin

include/ChangeLog:

2020-03-23  Martin Liska  <mliska@suse.cz>

	PR lto/94249
	* plugin-api.h: Use __BYTE_ORDER__ instead of __BIG_ENDIAN__
	which is not defined on sparc64 target.
---
  include/plugin-api.h | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Jeff Law via Gcc-patches March 23, 2020, 9:43 a.m. UTC | #1
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
Martin Liška March 23, 2020, 10 a.m. UTC | #2
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
>
Jeff Law via Gcc-patches March 23, 2020, 10:10 a.m. UTC | #3
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
Martin Liška March 23, 2020, 10:28 a.m. UTC | #4
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
>
Jeff Law via Gcc-patches March 23, 2020, 10:35 a.m. UTC | #5
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
Martin Liška March 23, 2020, 12:43 p.m. UTC | #6
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
Martin Liška March 23, 2020, 3:06 p.m. UTC | #7
Hi.

There's a patch draft that will do the proper versioning of API.
It's a subject for discussion.

Martin
Jeff Law via Gcc-patches March 23, 2020, 3:16 p.m. UTC | #8
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
Jeff Law via Gcc-patches March 23, 2020, 3:39 p.m. UTC | #9
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
Jeff Law via Gcc-patches March 23, 2020, 4:06 p.m. UTC | #10
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?
Martin Liška March 23, 2020, 5:17 p.m. UTC | #11
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
Jeff Law via Gcc-patches March 23, 2020, 5:40 p.m. UTC | #12
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
Martin Liška March 24, 2020, 8:19 a.m. UTC | #13
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
Jeff Law via Gcc-patches March 24, 2020, 8:31 a.m. UTC | #14
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
Martin Liška March 24, 2020, 8:49 a.m. UTC | #15
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
Jeff Law via Gcc-patches March 24, 2020, 9:18 a.m. UTC | #16
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
Martin Liška March 24, 2020, 10:32 a.m. UTC | #17
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
>
Jeff Law via Gcc-patches March 24, 2020, 10:39 a.m. UTC | #18
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 mbox series

Patch

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;