diff mbox series

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

Message ID alpine.LFD.2.21.2003301906260.2689954@eddie.linux-mips.org
State Accepted
Headers show
Series PR lto/94249: Correct endianness detection with the __BYTE_ORDER macro | expand

Commit Message

Maciej W. Rozycki March 31, 2020, 1:27 p.m. UTC 
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

Comments

Hans-Peter Nilsson April 1, 2020, 5:01 a.m. UTC | #1 
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
Li, Pan2 via Gcc-patches April 1, 2020, 7:17 a.m. UTC | #2 
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
Martin Liška April 1, 2020, 7:41 a.m. UTC | #3 
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
>
Martin Liška April 1, 2020, 7:43 a.m. UTC | #4 
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
>
Maciej W. Rozycki April 1, 2020, 9:55 a.m. UTC | #5 
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
Martin Liška April 1, 2020, 10:01 a.m. UTC | #6 
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
>
Maciej W. Rozycki April 1, 2020, 10:04 a.m. UTC | #7 
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
Martin Liška April 1, 2020, 10:09 a.m. UTC | #8 
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
>
Maciej W. Rozycki April 1, 2020, 3:59 p.m. UTC | #9 
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
Martin Liška April 1, 2020, 4:54 p.m. UTC | #10 
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
>
Maciej W. Rozycki April 1, 2020, 5:28 p.m. UTC | #11 
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
Hans-Peter Nilsson April 1, 2020, 11:57 p.m. UTC | #12 
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
diff mbox series

Patch

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