[PATCH] Check endianess detection.

[PATCH] Check endianess detection.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

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(-)



[-- Attachment #2: 0001-Check-endianess-detection.patch --]
[-- Type: text/x-patch, Size: 681 bytes --]

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;

Re: [PATCH] Check endianess detection.

[Jakub Jelinek]

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

Re: [PATCH] Check endianess detection.

[Martin Liška]

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
> 

Re: [PATCH] Check endianess detection.

[Jakub Jelinek]

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

Re: [PATCH] Check endianess detection.

[Martin Liška]

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
> 

Re: [PATCH] Check endianess detection.

[Jakub Jelinek]

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

Re: [PATCH] Check endianess detection.

[Martin Liška]

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

Re: [PATCH] Check endianess detection.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 109 bytes --]

Hi.

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

Martin

[-- Attachment #2: 0001-Introduce-ld_plugin_symbol_v2-for-LTO-plugin.patch --]
[-- Type: text/x-patch, Size: 6230 bytes --]

From b3f88adc91c36649bec3ed125b3e36ee89143bab Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 23 Mar 2020 16:01:29 +0100
Subject: [PATCH] Introduce ld_plugin_symbol_v2 for LTO plugin.

---
 include/plugin-api.h    | 40 ++++++++++++++++++++++-----------
 lto-plugin/lto-plugin.c | 50 +++++++++++++++++++++++++++++++++--------
 2 files changed, 68 insertions(+), 22 deletions(-)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 673f136ce68..34ccae85ff8 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -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;
-  char section_kind;
-  char symbol_type;
-  char def;
-#else
-  char def;
-  char symbol_type;
-  char section_kind;
-  char unused;
-#endif
+  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;
+};
+
 /* An object's section.  */
 
 struct ld_plugin_section
@@ -237,6 +242,14 @@ enum ld_plugin_status
 (*ld_plugin_add_symbols) (void *handle, int nsyms,
                           const struct ld_plugin_symbol *syms);
 
+/* The linker's interface for adding symbols from a claimed input file
+   (version 2).  */
+
+typedef
+enum ld_plugin_status
+(*ld_plugin_add_symbols_v2) (void *handle, int nsyms,
+			     const struct ld_plugin_symbol_v2 *syms);
+
 /* The linker's interface for getting the input file information with
    an open (possibly re-opened) file descriptor.  */
 
@@ -475,6 +488,7 @@ struct ld_plugin_tv
     ld_plugin_register_all_symbols_read tv_register_all_symbols_read;
     ld_plugin_register_cleanup tv_register_cleanup;
     ld_plugin_add_symbols tv_add_symbols;
+    ld_plugin_add_symbols_v2 tv_add_symbols_v2;
     ld_plugin_get_symbols tv_get_symbols;
     ld_plugin_add_input_file tv_add_input_file;
     ld_plugin_message tv_message;
diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index ca6c84a1ffd..ea3faa89205 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -114,6 +114,7 @@ struct plugin_symtab
   int nsyms;
   struct sym_aux *aux;
   struct ld_plugin_symbol *syms;
+  struct ld_plugin_symbol_v2 *syms_v2;
   unsigned long long id;
 };
 
@@ -163,7 +164,8 @@ static ld_plugin_register_cleanup register_cleanup;
 static ld_plugin_add_input_file add_input_file;
 static ld_plugin_add_input_library add_input_library;
 static ld_plugin_message message;
-static ld_plugin_add_symbols add_symbols, add_symbols_v2;
+static ld_plugin_add_symbols add_symbols;
+static ld_plugin_add_symbols_v2 add_symbols_v2;
 
 static struct plugin_file_info *claimed_files = NULL;
 static unsigned int num_claimed_files = 0;
@@ -290,8 +292,6 @@ parse_table_entry (char *p, struct ld_plugin_symbol *entry,
   else
     entry->comdat_key = xstrdup (entry->comdat_key);
 
-  entry->unused = entry->section_kind = entry->symbol_type = 0;
-
   t = *p;
   check (t <= 4, LDPL_FATAL, "invalid symbol kind found");
   entry->def = translate_kind[t];
@@ -320,7 +320,7 @@ parse_table_entry (char *p, struct ld_plugin_symbol *entry,
    Returns the address of the next entry. */
 
 static char *
-parse_table_entry_extension (char *p, struct ld_plugin_symbol *entry)
+parse_table_entry_extension (char *p, struct ld_plugin_symbol_v2 *entry)
 {
   unsigned char t;
   enum ld_plugin_symbol_type symbol_types[] =
@@ -384,9 +384,15 @@ parse_symtab_extension (char *data, char *end, struct plugin_symtab *out)
      - section_kind
      .  */
 
+  out->syms_v2 = xrealloc (out->syms_v2,
+			   out->nsyms * sizeof (struct ld_plugin_symbol_v2));
+  memset (out->syms_v2, 0, out->nsyms * sizeof (struct ld_plugin_symbol_v2));
+
   if (version == 1)
-    for (i = 0; i < out->nsyms; i++)
-      data = parse_table_entry_extension (data, &out->syms[i]);
+    {
+      for (i = 0; i < out->nsyms; i++)
+	data = parse_table_entry_extension (data, &out->syms_v2[i]);
+    }
 }
 
 /* Free all memory that is no longer needed after writing the symbol
@@ -409,6 +415,11 @@ free_1 (struct plugin_file_info *files, unsigned num_files)
 	}
       free (symtab->syms);
       symtab->syms = NULL;
+      if (symtab->syms_v2)
+	{
+	  free (symtab->syms_v2);
+	  symtab->syms_v2 = NULL;
+	}
     }
 }
 
@@ -519,6 +530,11 @@ free_symtab (struct plugin_symtab *symtab)
 {
   free (symtab->syms);
   symtab->syms = NULL;
+  if (symtab->syms_v2)
+    {
+      free (symtab->syms_v2);
+      symtab->syms_v2 = NULL;
+    }
   free (symtab->aux);
   symtab->aux = NULL;
 }
@@ -1193,8 +1209,24 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
   if (obj.found > 0)
     {
       if (add_symbols_v2)
-	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++)
+	    {
+	      struct ld_plugin_symbol *old = &lto_file.symtab.syms[i];
+	      struct ld_plugin_symbol_v2 *new = &lto_file.symtab.syms_v2[i];
+	      new->name = old->name;
+	      new->version = old->version;
+	      new->def = old->def;
+	      new->visibility = old->visibility;
+	      new->size = old->size;
+	      new->comdat_key = old->comdat_key;
+	      new->resolution = old->resolution;
+	    }
+
+	  status = add_symbols_v2 (file->handle, lto_file.symtab.nsyms,
+				   lto_file.symtab.syms_v2);
+	}
       else
 	status = add_symbols (file->handle, lto_file.symtab.nsyms,
 			      lto_file.symtab.syms);
@@ -1359,7 +1391,7 @@ onload (struct ld_plugin_tv *tv)
 	  register_claim_file = p->tv_u.tv_register_claim_file;
 	  break;
 	case LDPT_ADD_SYMBOLS_V2:
-	  add_symbols_v2 = p->tv_u.tv_add_symbols;
+	  add_symbols_v2 = p->tv_u.tv_add_symbols_v2;
 	  break;
 	case LDPT_ADD_SYMBOLS:
 	  add_symbols = p->tv_u.tv_add_symbols;
-- 
2.25.1

Re: [PATCH] Check endianess detection.

[Richard Biener]

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

Re: [PATCH] Check endianess detection.

[Richard Biener]

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

Re: [PATCH] Check endianess detection.

[H.J. Lu]

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?


-- 
H.J.

Re: [PATCH] Check endianess detection.

[Martin Liška]

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

Re: [PATCH] Check endianess detection.

[H.J. Lu]

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



-- 
H.J.

Re: [PATCH] Check endianess detection.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 167 bytes --]

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

[-- Attachment #2: 0001-Improve-endianess-detection.patch --]
[-- Type: text/x-patch, Size: 2349 bytes --]

From be4ce2c54672e7772586c13d6e37c63124624bbc 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.
---
 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
 
+/* Detect endianess based on __BYTE_ORDER__ macro.  */
+#if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__) && defined(__ORDER_LITTLE_ENDIAN__)
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+#define PLUGIN_LITTLE_ENDIAN 1
+#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#define PLUGIN_BIG_ENDIAN 1
+#endif
+#else
+/* 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
+/* Detect endianess based on _BYTE_ORDER.  */
+#if _BYTE_ORDER == _LITTLE_ENDIAN
+#define PLUGIN_LITTLE_ENDIAN 1
+#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
+
 /* Status code returned by most API routines.  */
 
 enum ld_plugin_status
@@ -89,16 +126,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 PLUGIN_BIG_ENDIAN == 1
   char unused;
   char section_kind;
   char symbol_type;
   char def;
-#else
+#elif PLUGIN_LITTLE_ENDIAN == 1
   char def;
   char symbol_type;
   char section_kind;
   char unused;
+#else
+#error "Could not detect architecture endianess"
 #endif
   int visibility;
   uint64_t size;
-- 
2.25.1

Re: [PATCH] Check endianess detection.

[Jakub Jelinek]

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

Re: [PATCH] Check endianess detection.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

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

[-- Attachment #2: 0001-Improve-endianess-detection.patch --]
[-- Type: text/x-patch, Size: 2874 bytes --]

From 05c219e70a6928ed3fbb087090594fe2a09234a9 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.
---
 include/plugin-api.h | 61 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 2 deletions(-)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 673f136ce68..e8ba6603977 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -37,6 +37,56 @@
 #error cannot find uint64_t type
 #endif
 
+/* 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
+#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
+/* Detect endianess based on _BYTE_ORDER.  */
+#if _BYTE_ORDER == _LITTLE_ENDIAN
+#define PLUGIN_LITTLE_ENDIAN 1
+#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
+
 #ifdef __cplusplus
 extern "C"
 {
@@ -89,16 +139,23 @@ struct ld_plugin_symbol
   char *version;
   /* This is for compatibility with older ABIs.  The older ABI defined
      only 'def' field.  */
-#ifdef __BIG_ENDIAN__
+#if PLUGIN_BIG_ENDIAN == 1
   char unused;
   char section_kind;
   char symbol_type;
   char def;
-#else
+#elif PLUGIN_LITTLE_ENDIAN == 1
   char def;
   char symbol_type;
   char section_kind;
   char unused;
+#elif PLUGIN_PDP_ENDIAN == 1
+  char symbol_type;
+  char def;
+  char unused;
+  char section_kind;
+#else
+#error "Could not detect architecture endianess"
 #endif
   int visibility;
   uint64_t size;
-- 
2.25.1

Re: [PATCH] Check endianess detection.

[Jakub Jelinek]

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

Re: [PATCH] Check endianess detection.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2454 bytes --]

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
> 


[-- Attachment #2: 0001-Improve-endianess-detection.patch --]
[-- Type: text/x-patch, Size: 2934 bytes --]

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.
---
 include/plugin-api.h | 65 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/include/plugin-api.h b/include/plugin-api.h
index 673f136ce68..864d2bf91ac 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -37,6 +37,60 @@
 #error cannot find uint64_t type
 #endif
 
+/* 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>
+#ifdef _BYTE_ORDER
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+#define PLUGIN_LITTLE_ENDIAN 1
+#elif __BYTE_ORDER == __BIG_ENDIAN
+#define PLUGIN_BIG_ENDIAN 1
+#endif
+#endif
+#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
+/* Detect endianess based on _BYTE_ORDER.  */
+#ifdef _BYTE_ORDER
+#if _BYTE_ORDER == _LITTLE_ENDIAN
+#define PLUGIN_LITTLE_ENDIAN 1
+#elif _BYTE_ORDER == _BIG_ENDIAN
+#define PLUGIN_BIG_ENDIAN 1
+#endif
+#endif
+/* Detect based on _WIN32.  */
+#if defined(_WIN32)
+#define PLUGIN_LITTLE_ENDIAN 1
+#endif
+/* Detect based on __BIG_ENDIAN__ and __LITTLE_ENDIAN__ */
+#ifdef __LITTLE_ENDIAN__
+#define PLUGIN_LITTLE_ENDIAN 1
+#endif
+#ifdef __BIG_ENDIAN__
+#define PLUGIN_BIG_ENDIAN 1
+#endif
+#endif
+
 #ifdef __cplusplus
 extern "C"
 {
@@ -89,16 +143,23 @@ struct ld_plugin_symbol
   char *version;
   /* This is for compatibility with older ABIs.  The older ABI defined
      only 'def' field.  */
-#ifdef __BIG_ENDIAN__
+#if PLUGIN_BIG_ENDIAN == 1
   char unused;
   char section_kind;
   char symbol_type;
   char def;
-#else
+#elif PLUGIN_LITTLE_ENDIAN == 1
   char def;
   char symbol_type;
   char section_kind;
   char unused;
+#elif PLUGIN_PDP_ENDIAN == 1
+  char symbol_type;
+  char def;
+  char unused;
+  char section_kind;
+#else
+#error "Could not detect architecture endianess"
 #endif
   int visibility;
   uint64_t size;
-- 
2.25.1

Re: [PATCH] Check endianess detection.

[Jakub Jelinek]

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

[PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Maciej W. Rozycki]

Correct an issue with GCC commit 906b3eb9df6c ("Improve endianess 
detection.") and fix a typo in the __BYTE_ORDER fallback macro check 
that causes compilation errors like:

.../include/plugin-api.h:162:2: error: #error "Could not detect architecture endianess"

on systems that do not provide the __BYTE_ORDER__ macro.

	include/
	PR lto/94249
	* plugin-api.h: Fix a typo in the __BYTE_ORDER macro check.
---
On Tue, 24 Mar 2020, Martin Liška wrote:

> >> +/* 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.

 This change, when merged into binutils, caused BFD to fail building in 
one of my configurations:

In file included from .../bfd/elflink.c:31:
.../bfd/../include/plugin-api.h:162:2: error: #error "Could not detect architecture endianess"
make[4]: *** [elflink.lo] Error 1

This is due to a missing underscore in the:

#ifdef _BYTE_ORDER

check.

 OK to apply this hopefully obvious fix to GCC and then merge to binutils?

 NB if posting as an attachment please try matching the message subject 
with the change heading as otherwise it takes a lot of effort to track the 
patch submission corresponding to a given commit.

  Maciej
---
 include/plugin-api.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

binutils-include-plugin-api-byte-order.diff
Index: binutils/include/plugin-api.h
===================================================================
--- binutils.orig/include/plugin-api.h
+++ binutils/include/plugin-api.h
@@ -51,7 +51,7 @@
 /* Older GCC releases (<4.6.0) can make detection from glibc macros.  */
 #if defined(__GLIBC__) || defined(__GNU_LIBRARY__) || defined(__ANDROID__)
 #include <endian.h>
-#ifdef _BYTE_ORDER
+#ifdef __BYTE_ORDER
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 #define PLUGIN_LITTLE_ENDIAN 1
 #elif __BYTE_ORDER == __BIG_ENDIAN

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Hans-Peter Nilsson]

On Tue, 31 Mar 2020, Maciej W. Rozycki wrote:
> Correct an issue with GCC commit 906b3eb9df6c ("Improve endianess
> detection.") and fix a typo in the __BYTE_ORDER fallback macro check
> that causes compilation errors like:
>
> .../include/plugin-api.h:162:2: error: #error "Could not detect architecture endianess"
>
> on systems that do not provide the __BYTE_ORDER__ macro.

> Index: binutils/include/plugin-api.h
> ===================================================================
> --- binutils.orig/include/plugin-api.h
> +++ binutils/include/plugin-api.h
> @@ -51,7 +51,7 @@
>  /* Older GCC releases (<4.6.0) can make detection from glibc macros.  */
>  #if defined(__GLIBC__) || defined(__GNU_LIBRARY__) || defined(__ANDROID__)
>  #include <endian.h>
> -#ifdef _BYTE_ORDER
> +#ifdef __BYTE_ORDER
>  #if __BYTE_ORDER == __LITTLE_ENDIAN
>  #define PLUGIN_LITTLE_ENDIAN 1
>  #elif __BYTE_ORDER == __BIG_ENDIAN

FWIW, I was about to commit that as obvious, also the bignum.h
inclusion thing!

The only question being, how the typo passed any kind of testing
in the first place...  No actually, there's also the question
why the plugin-API needs to bother with host endianness.  It's
not like endians are going to be different between plugins and
gcc on host.

brgds, H-P

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Richard Biener]

On Tue, Mar 31, 2020 at 3:28 PM Maciej W. Rozycki <macro@linux-mips.org> wrote:
>
> Correct an issue with GCC commit 906b3eb9df6c ("Improve endianess
> detection.") and fix a typo in the __BYTE_ORDER fallback macro check
> that causes compilation errors like:
>
> .../include/plugin-api.h:162:2: error: #error "Could not detect architecture endianess"
>
> on systems that do not provide the __BYTE_ORDER__ macro.
>
>         include/
>         PR lto/94249
>         * plugin-api.h: Fix a typo in the __BYTE_ORDER macro check.
> ---
> On Tue, 24 Mar 2020, Martin Liška wrote:
>
> > >> +/* 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.
>
>  This change, when merged into binutils, caused BFD to fail building in
> one of my configurations:
>
> In file included from .../bfd/elflink.c:31:
> .../bfd/../include/plugin-api.h:162:2: error: #error "Could not detect architecture endianess"
> make[4]: *** [elflink.lo] Error 1
>
> This is due to a missing underscore in the:
>
> #ifdef _BYTE_ORDER
>
> check.
>
>  OK to apply this hopefully obvious fix to GCC and then merge to binutils?

OK.

>  NB if posting as an attachment please try matching the message subject
> with the change heading as otherwise it takes a lot of effort to track the
> patch submission corresponding to a given commit.
>
>   Maciej
> ---
>  include/plugin-api.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> binutils-include-plugin-api-byte-order.diff
> Index: binutils/include/plugin-api.h
> ===================================================================
> --- binutils.orig/include/plugin-api.h
> +++ binutils/include/plugin-api.h
> @@ -51,7 +51,7 @@
>  /* Older GCC releases (<4.6.0) can make detection from glibc macros.  */
>  #if defined(__GLIBC__) || defined(__GNU_LIBRARY__) || defined(__ANDROID__)
>  #include <endian.h>
> -#ifdef _BYTE_ORDER
> +#ifdef __BYTE_ORDER
>  #if __BYTE_ORDER == __LITTLE_ENDIAN
>  #define PLUGIN_LITTLE_ENDIAN 1
>  #elif __BYTE_ORDER == __BIG_ENDIAN

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Martin Liška]

On 3/31/20 3:27 PM, Maciej W. Rozycki wrote:
> Correct an issue with GCC commit 906b3eb9df6c ("Improve endianess
> detection.") and fix a typo in the __BYTE_ORDER fallback macro check
> that causes compilation errors like:
> 
> .../include/plugin-api.h:162:2: error: #error "Could not detect architecture endianess"
> 
> on systems that do not provide the __BYTE_ORDER__ macro.

Hello.

Nice catch!

> 
> 	include/
> 	PR lto/94249
> 	* plugin-api.h: Fix a typo in the __BYTE_ORDER macro check.
> ---
> On Tue, 24 Mar 2020, Martin Liška wrote:
> 
>>>> +/* 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.
> 
>   This change, when merged into binutils, caused BFD to fail building in
> one of my configurations:
> 
> In file included from .../bfd/elflink.c:31:
> .../bfd/../include/plugin-api.h:162:2: error: #error "Could not detect architecture endianess"
> make[4]: *** [elflink.lo] Error 1
> 
> This is due to a missing underscore in the:
> 
> #ifdef _BYTE_ORDER
> 
> check.
> 
>   OK to apply this hopefully obvious fix to GCC and then merge to binutils?

I've just installed the patch.
@H.J. Can you please pull it to bintuils?

> 
>   NB if posting as an attachment please try matching the message subject
> with the change heading as otherwise it takes a lot of effort to track the
> patch submission corresponding to a given commit.

I see your point, but note that sometimes a direction of a patch changes during
the mailing list discussion. And so that we end up with a commit message that
has a different name.

Anyway, thank you for your help.
Martin

> 
>    Maciej
> ---
>   include/plugin-api.h |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> binutils-include-plugin-api-byte-order.diff
> Index: binutils/include/plugin-api.h
> ===================================================================
> --- binutils.orig/include/plugin-api.h
> +++ binutils/include/plugin-api.h
> @@ -51,7 +51,7 @@
>   /* Older GCC releases (<4.6.0) can make detection from glibc macros.  */
>   #if defined(__GLIBC__) || defined(__GNU_LIBRARY__) || defined(__ANDROID__)
>   #include <endian.h>
> -#ifdef _BYTE_ORDER
> +#ifdef __BYTE_ORDER
>   #if __BYTE_ORDER == __LITTLE_ENDIAN
>   #define PLUGIN_LITTLE_ENDIAN 1
>   #elif __BYTE_ORDER == __BIG_ENDIAN
> 

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Martin Liška]

On 4/1/20 7:01 AM, Hans-Peter Nilsson wrote:
> On Tue, 31 Mar 2020, Maciej W. Rozycki wrote:
>> Correct an issue with GCC commit 906b3eb9df6c ("Improve endianess
>> detection.") and fix a typo in the __BYTE_ORDER fallback macro check
>> that causes compilation errors like:
>>
>> .../include/plugin-api.h:162:2: error: #error "Could not detect architecture endianess"
>>
>> on systems that do not provide the __BYTE_ORDER__ macro.
> 
>> Index: binutils/include/plugin-api.h
>> ===================================================================
>> --- binutils.orig/include/plugin-api.h
>> +++ binutils/include/plugin-api.h
>> @@ -51,7 +51,7 @@
>>   /* Older GCC releases (<4.6.0) can make detection from glibc macros.  */
>>   #if defined(__GLIBC__) || defined(__GNU_LIBRARY__) || defined(__ANDROID__)
>>   #include <endian.h>
>> -#ifdef _BYTE_ORDER
>> +#ifdef __BYTE_ORDER
>>   #if __BYTE_ORDER == __LITTLE_ENDIAN
>>   #define PLUGIN_LITTLE_ENDIAN 1
>>   #elif __BYTE_ORDER == __BIG_ENDIAN
> 
> FWIW, I was about to commit that as obvious, also the bignum.h
> inclusion thing!
> 
> The only question being, how the typo passed any kind of testing
> in the first place...

Because I don't have a legacy system with an ancient glibc version.
Note that testing matrix for such a change is massive, including such
exotic targets like SUN, minix, Windows, ...

>  No actually, there's also the question
> why the plugin-API needs to bother with host endianness.  It's
> not like endians are going to be different between plugins and
> gcc on host.

No, the plugin endianess matches with a host compiler endianess.
Martin

> 
> brgds, H-P
> 

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Maciej W. Rozycki]

On Wed, 1 Apr 2020, Martin Liška wrote:

> >   OK to apply this hopefully obvious fix to GCC and then merge to binutils?
> 
> I've just installed the patch.
> @H.J. Can you please pull it to bintuils?

 Why didn't you use the commit as I published it and also assumed 
authorship of my change?  I feel insulted.

  Maciej

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Martin Liška]

On 4/1/20 11:55 AM, Maciej W. Rozycki wrote:
> On Wed, 1 Apr 2020, Martin Liška wrote:
> 
>>>    OK to apply this hopefully obvious fix to GCC and then merge to binutils?
>>
>> I've just installed the patch.
>> @H.J. Can you please pull it to bintuils?
> 
>   Why didn't you use the commit as I published it

Because it didn't fit my script that takes changelog entries
and moves that to the corresponding ChangeLog files.
Next time, you will install the patch by your own.

> and also assumed
> authorship of my change?  I feel insulted.

I removed myself from the ownership of the patch here:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=d3ee88fdb4e0f718aaba050a3eb7174b8934a29d

Martin

> 
>    Maciej
> 

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Maciej W. Rozycki]

On Wed, 1 Apr 2020, Martin Liška wrote:

> >   NB if posting as an attachment please try matching the message subject
> > with the change heading as otherwise it takes a lot of effort to track the
> > patch submission corresponding to a given commit.
> 
> I see your point, but note that sometimes a direction of a patch changes
> during
> the mailing list discussion. And so that we end up with a commit message that
> has a different name.

 What's the problem with changing the message subject at the point the 
final change is posted, just as I did with the change I submitted (where 
you chose to actually ignore what I posted and gratuitously changed the 
commit heading from one I used anyway)?

  Maciej

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Martin Liška]

On 4/1/20 12:04 PM, Maciej W. Rozycki wrote:
> On Wed, 1 Apr 2020, Martin Liška wrote:
> 
>>>    NB if posting as an attachment please try matching the message subject
>>> with the change heading as otherwise it takes a lot of effort to track the
>>> patch submission corresponding to a given commit.
>>
>> I see your point, but note that sometimes a direction of a patch changes
>> during
>> the mailing list discussion. And so that we end up with a commit message that
>> has a different name.
> 
>   What's the problem with changing the message subject at the point the
> final change is posted, just as I did with the change I submitted (where
> you chose to actually ignore what I posted and gratuitously changed the
> commit heading from one I used anyway)?

Yes, that was a mistake, sorry for that.

Martin

> 
>    Maciej
> 

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Maciej W. Rozycki]

On Wed, 1 Apr 2020, Martin Liška wrote:

> >> I've just installed the patch.
> >> @H.J. Can you please pull it to bintuils?
> > 
> >   Why didn't you use the commit as I published it
> 
> Because it didn't fit my script that takes changelog entries
> and moves that to the corresponding ChangeLog files.
> Next time, you will install the patch by your own.

 That was the intent: I asked for approval to commit, which is the usual 
practice for people who have write access to the repo, and not to commit 
on my behalf.

> > and also assumed
> > authorship of my change?  I feel insulted.
> 
> I removed myself from the ownership of the patch here:
> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=d3ee88fdb4e0f718aaba050a3eb7174b8934a29d

 I don't think it has changed anything:

commit 142d68f50b48309f48e34fc1d9d6dbbeecfde684
Author:     Martin Liska <mliska@suse.cz>
AuthorDate: Wed Apr 1 09:37:37 2020 +0200
Commit:     Martin Liska <mliska@suse.cz>
CommitDate: Wed Apr 1 09:37:37 2020 +0200

You're still listed as the author of the change in question.

  Maciej

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Martin Liška]

On 4/1/20 5:59 PM, Maciej W. Rozycki wrote:
> On Wed, 1 Apr 2020, Martin Liška wrote:
> 
>>>> I've just installed the patch.
>>>> @H.J. Can you please pull it to bintuils?
>>>
>>>    Why didn't you use the commit as I published it
>>
>> Because it didn't fit my script that takes changelog entries
>> and moves that to the corresponding ChangeLog files.
>> Next time, you will install the patch by your own.
> 
>   That was the intent: I asked for approval to commit, which is the usual
> practice for people who have write access to the repo, and not to commit
> on my behalf.

I've got it.

> 
>>> and also assumed
>>> authorship of my change?  I feel insulted.
>>
>> I removed myself from the ownership of the patch here:
>> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=d3ee88fdb4e0f718aaba050a3eb7174b8934a29d
> 
>   I don't think it has changed anything:
> 
> commit 142d68f50b48309f48e34fc1d9d6dbbeecfde684
> Author:     Martin Liska <mliska@suse.cz>
> AuthorDate: Wed Apr 1 09:37:37 2020 +0200
> Commit:     Martin Liska <mliska@suse.cz>
> CommitDate: Wed Apr 1 09:37:37 2020 +0200
> 
> You're still listed as the author of the change in question.

It's the first time anybody is asking me for that. I considered the ChangeLog
entry as sufficient. Anyway, next time please send a patch with git format-patch
so that one can apply it with git am. That will preserve you as the author.
Or ideally, feel free to fulfil copyright assignment and install commits
by your own.

Martin

> 
>    Maciej
> 

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Maciej W. Rozycki]

On Wed, 1 Apr 2020, Martin Liška wrote:

> > commit 142d68f50b48309f48e34fc1d9d6dbbeecfde684
> > Author:     Martin Liska <mliska@suse.cz>
> > AuthorDate: Wed Apr 1 09:37:37 2020 +0200
> > Commit:     Martin Liska <mliska@suse.cz>
> > CommitDate: Wed Apr 1 09:37:37 2020 +0200
> > 
> > You're still listed as the author of the change in question.
> 
> It's the first time anybody is asking me for that. I considered the ChangeLog
> entry as sufficient. Anyway, next time please send a patch with git
> format-patch
> so that one can apply it with git am. That will preserve you as the author.

 Of course it was formatted for `git am', I've been doing that for many 
years now for all the pieces of the toolchain (even with GCC while it was 
still on SVN).  Had you fed the message to `git am', it would have done 
the right thing.

> Or ideally, feel free to fulfil copyright assignment and install commits
> by your own.

 I've had it for some 20 years now for GCC and other pieces of the GNU
toolchain.  Otherwise I couldn't have been listed in MAINTAINERS.

 Why did you come up with an idea that I haven't?

  Maciej

Re: [PATCH] PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro

[Hans-Peter Nilsson]

On Wed, 1 Apr 2020, Martin Li?ka wrote:

> On 4/1/20 7:01 AM, Hans-Peter Nilsson wrote:
> > On Tue, 31 Mar 2020, Maciej W. Rozycki wrote:
> > > Correct an issue with GCC commit 906b3eb9df6c ("Improve endianess
> > > detection.") and fix a typo in the __BYTE_ORDER fallback macro check
> > > that causes compilation errors like:
> > >
> > > .../include/plugin-api.h:162:2: error: #error "Could not detect
> > > architecture endianess"
> > >
> > > on systems that do not provide the __BYTE_ORDER__ macro.
> >
> > > Index: binutils/include/plugin-api.h
> > > ===================================================================
> > > --- binutils.orig/include/plugin-api.h
> > > +++ binutils/include/plugin-api.h
> > > @@ -51,7 +51,7 @@
> > >   /* Older GCC releases (<4.6.0) can make detection from glibc macros.  */
> > >   #if defined(__GLIBC__) || defined(__GNU_LIBRARY__) ||
> > > defined(__ANDROID__)
> > >   #include <endian.h>
> > > -#ifdef _BYTE_ORDER
> > > +#ifdef __BYTE_ORDER
> > >   #if __BYTE_ORDER == __LITTLE_ENDIAN
> > >   #define PLUGIN_LITTLE_ENDIAN 1
> > >   #elif __BYTE_ORDER == __BIG_ENDIAN
> >
> > FWIW, I was about to commit that as obvious, also the bignum.h
> > inclusion thing!
> >
> > The only question being, how the typo passed any kind of testing
> > in the first place...
>
> Because I don't have a legacy system with an ancient glibc version.

Before anyone starts doing this too: NO.  That's just not valid
procedure and not a valid excuse.  Please test your patches more
carefully.

*Ask* for it to be tested if you can't find a way to test it on
your own.  I see you didn't even say that you didn't test those
lines, when the patch was submitted.

> Note that testing matrix for such a change is massive, including such
> exotic targets like SUN, minix, Windows, ...

Not necessary, you just have to cover those lines for *one*
host system.

Such situations can usually even be worked around to make sure
those lines are tested at least once, something like "to test
this on my current glibc system, I temporarily edited
plugins-api.h, putting an #undef __BYTE_ORDER" just after the
'Older GCC' comment".

Another solution would be to disable plugins for such legacy
systems.

> >  No actually, there's also the question
> > why the plugin-API needs to bother with host endianness.  It's
> > not like endians are going to be different between plugins and
> > gcc on host.
>
> No, the plugin endianess matches with a host compiler endianess.

If that's true then it could be just written and read as-is,
unless you do different pickling on the way in, but if so, that
could be fixed cleaner than writing differently than reading.
...oh, I see there's a hack; there's an assumption that there
was padding with the "old" API and abusing that to add fields
for a "new" API, and using the endianness to indicate the
location of the padding.  Ew.  I'm not going to step closer than
that.

BTW, was this old/new plugin-API support tested
*cross*-versions?

brgds, H-P