Patchwork New failure on aarch64 in Fedora Rawhide.

login
register
mail settings
Submitter Carlos O'Donell
Date April 9, 2019, 9:26 p.m.
Message ID <bf1c5fb5-510f-8cc6-89d9-d3ca9e03ae8f@redhat.com>
Download mbox | patch
Permalink /patch/32232/
State New
Headers show

Comments

Carlos O'Donell - April 9, 2019, 9:26 p.m.
Szabolcs,

https://kojipkgs.fedoraproject.org//work/tasks/2880/34082880/build.log

BUILDSTDERR: In file included from ../sysdeps/aarch64/sysdep.h:22,
BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:26,
BUILDSTDERR:                  from ../sysdeps/aarch64/nptl/tls.h:37,
BUILDSTDERR:                  from ../include/errno.h:25,
BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/netlink_assert_response.c:19:
BUILDSTDERR: ../sysdeps/generic/sysdep.h:81: error: "CFI_RESTORE" redefined [-Werror]
BUILDSTDERR:    81 | # define CFI_RESTORE(reg) \
BUILDSTDERR:       |
BUILDSTDERR: In file included from ../sysdeps/unix/sysdep.h:18,
BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:25,
BUILDSTDERR:                  from ../sysdeps/aarch64/nptl/tls.h:37,
BUILDSTDERR:                  from ../include/errno.h:25,
BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/netlink_assert_response.c:19:
BUILDSTDERR: ../sysdeps/generic/sysdep.h:81: note: this is the location of the previous definition
BUILDSTDERR:    81 | # define CFI_RESTORE(reg) \
BUILDSTDERR:       |
BUILDSTDERR: cc1: all warnings being treated as errors

This just came up in Fedora Rawhide.

We have no guards on systeps/generic/sysdep.h, I assume we want to include it multiple
times with different macro API settings.

Note the difference in include path is:

sysdeps/aarch64/sysdep.h vs. sysdeps/unix/sysdep.h

In sysdeps/unix/sysv/linux/aarch64/sysdep.h we include both:

  25 #include <sysdeps/unix/sysdep.h>
  26 #include <sysdeps/aarch64/sysdep.h>

Then in sysdeps/aarch64/sysdep.h we include:

  22 #include <sysdeps/generic/sysdep.h>

Then in sysdeps/unix/sysdeps.h we include:

  18 #include <sysdeps/generic/sysdep.h>

So we get two copies.

In general the project rule has been "Include the headers you need."

Guarding the macros with ifndef could lead to defaults being used incorrectly
(macro API issues), and was the reason we enabled -Wundef, so doing that makes
things less robust.

It would be really nice if we could just include what we needed once.

I would like to get to something like this:
---
We include the aarch64 sysdep.h, and then the Linux version, and we're done.
This avoids the double inclusion, and appears to work just fine, matching
what other arches do.

Thoughts?
Szabolcs Nagy - April 10, 2019, 10:31 a.m.
On 09/04/2019 22:26, Carlos O'Donell wrote:
> Szabolcs,

> 

> https://kojipkgs.fedoraproject.org//work/tasks/2880/34082880/build.log

> 

> BUILDSTDERR: In file included from ../sysdeps/aarch64/sysdep.h:22,

> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:26,

> BUILDSTDERR:                  from ../sysdeps/aarch64/nptl/tls.h:37,

> BUILDSTDERR:                  from ../include/errno.h:25,

> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/netlink_assert_response.c:19:

> BUILDSTDERR: ../sysdeps/generic/sysdep.h:81: error: "CFI_RESTORE" redefined [-Werror]

> BUILDSTDERR:    81 | # define CFI_RESTORE(reg) \

> BUILDSTDERR:       |

> BUILDSTDERR: In file included from ../sysdeps/unix/sysdep.h:18,

> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:25,

> BUILDSTDERR:                  from ../sysdeps/aarch64/nptl/tls.h:37,

> BUILDSTDERR:                  from ../include/errno.h:25,

> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/netlink_assert_response.c:19:

> BUILDSTDERR: ../sysdeps/generic/sysdep.h:81: note: this is the location of the previous definition

> BUILDSTDERR:    81 | # define CFI_RESTORE(reg) \

> BUILDSTDERR:       |

> BUILDSTDERR: cc1: all warnings being treated as errors

> 

> This just came up in Fedora Rawhide.

> 

> We have no guards on systeps/generic/sysdep.h, I assume we want to include it multiple

> times with different macro API settings.

> 

> Note the difference in include path is:

> 

> sysdeps/aarch64/sysdep.h vs. sysdeps/unix/sysdep.h

> 

> In sysdeps/unix/sysv/linux/aarch64/sysdep.h we include both:

> 

>  25 #include <sysdeps/unix/sysdep.h>

>  26 #include <sysdeps/aarch64/sysdep.h>

> 

> Then in sysdeps/aarch64/sysdep.h we include:

> 

>  22 #include <sysdeps/generic/sysdep.h>

> 

> Then in sysdeps/unix/sysdeps.h we include:

> 

>  18 #include <sysdeps/generic/sysdep.h>

> 

> So we get two copies.

> 

> In general the project rule has been "Include the headers you need."

> 

> Guarding the macros with ifndef could lead to defaults being used incorrectly

> (macro API issues), and was the reason we enabled -Wundef, so doing that makes

> things less robust.

> 

> It would be really nice if we could just include what we needed once.

> 

> I would like to get to something like this:

> diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h

> index 935c507f8cb36b2a..35f3dd65b3397001 100644

> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h

> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h

> @@ -22,9 +22,8 @@

>  /* Always enable vsyscalls on aarch64 */

>  #define ALWAYS_USE_VSYSCALL 1

>  

> -#include <sysdeps/unix/sysdep.h>

> -#include <sysdeps/aarch64/sysdep.h>

>  #include <sysdeps/unix/sysv/linux/generic/sysdep.h>

> +#include <sysdeps/aarch64/sysdep.h>

>  

>  /* Defines RTLD_PRIVATE_ERRNO and USE_DL_SYSINFO.  */

>  #include <dl-sysdep.h>

> ---

> We include the aarch64 sysdep.h, and then the Linux version, and we're done.

> This avoids the double inclusion, and appears to work just fine, matching

> what other arches do.

> 

> Thoughts?


This patch looks OK, but i don't yet understand the
failure: including sysdeps/generic/sysdep.h multiple
times should work, redefining a macro is valid if the
exact same pp token sequence is used.

Is there something that may change the definition of
CFI_RESTORE(reg) in sysdeps/generic/sysdep.h?
isn't inconsistent macro definition problematic
elsewhere?

I cannot reproduce this issue, would be nice to know
what causes it, it may be a gcc bug.
Andreas Schwab - April 10, 2019, 11:04 a.m.
On Apr 09 2019, Carlos O'Donell <codonell@redhat.com> wrote:

> https://kojipkgs.fedoraproject.org//work/tasks/2880/34082880/build.log
>
> BUILDSTDERR: In file included from ../sysdeps/aarch64/sysdep.h:22,
> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:26,
> BUILDSTDERR:                  from ../sysdeps/aarch64/nptl/tls.h:37,
> BUILDSTDERR:                  from ../include/errno.h:25,
> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/netlink_assert_response.c:19:
> BUILDSTDERR: ../sysdeps/generic/sysdep.h:81: error: "CFI_RESTORE" redefined [-Werror]
> BUILDSTDERR:    81 | # define CFI_RESTORE(reg) \
> BUILDSTDERR:       |
> BUILDSTDERR: In file included from ../sysdeps/unix/sysdep.h:18,
> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:25,
> BUILDSTDERR:                  from ../sysdeps/aarch64/nptl/tls.h:37,
> BUILDSTDERR:                  from ../include/errno.h:25,
> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/netlink_assert_response.c:19:
> BUILDSTDERR: ../sysdeps/generic/sysdep.h:81: note: this is the location of the previous definition
> BUILDSTDERR:    81 | # define CFI_RESTORE(reg) \
> BUILDSTDERR:       |
> BUILDSTDERR: cc1: all warnings being treated as errors
>
> This just came up in Fedora Rawhide.

I don't see that failure, and I don't understand why the compiler
complains.  The definitions should be 100% identical.  Why doesn't the
compiler complain about the other macro redefinitions?

Andreas.
Carlos O'Donell - April 10, 2019, 2:03 p.m.
On 4/10/19 7:04 AM, Andreas Schwab wrote:
> On Apr 09 2019, Carlos O'Donell <codonell@redhat.com> wrote:
> 
>> https://kojipkgs.fedoraproject.org//work/tasks/2880/34082880/build.log
>>
>> BUILDSTDERR: In file included from ../sysdeps/aarch64/sysdep.h:22,
>> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:26,
>> BUILDSTDERR:                  from ../sysdeps/aarch64/nptl/tls.h:37,
>> BUILDSTDERR:                  from ../include/errno.h:25,
>> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/netlink_assert_response.c:19:
>> BUILDSTDERR: ../sysdeps/generic/sysdep.h:81: error: "CFI_RESTORE" redefined [-Werror]
>> BUILDSTDERR:    81 | # define CFI_RESTORE(reg) \
>> BUILDSTDERR:       |
>> BUILDSTDERR: In file included from ../sysdeps/unix/sysdep.h:18,
>> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:25,
>> BUILDSTDERR:                  from ../sysdeps/aarch64/nptl/tls.h:37,
>> BUILDSTDERR:                  from ../include/errno.h:25,
>> BUILDSTDERR:                  from ../sysdeps/unix/sysv/linux/netlink_assert_response.c:19:
>> BUILDSTDERR: ../sysdeps/generic/sysdep.h:81: note: this is the location of the previous definition
>> BUILDSTDERR:    81 | # define CFI_RESTORE(reg) \
>> BUILDSTDERR:       |
>> BUILDSTDERR: cc1: all warnings being treated as errors
>>
>> This just came up in Fedora Rawhide.
> 
> I don't see that failure, and I don't understand why the compiler
> complains.  The definitions should be 100% identical.  Why doesn't the
> compiler complain about the other macro redefinitions?

And the weirder thing is that a rebuild with the same compiler version worked.

Maybe flaky hardware?

Patch

diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
index 935c507f8cb36b2a..35f3dd65b3397001 100644
--- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
@@ -22,9 +22,8 @@ 
  /* Always enable vsyscalls on aarch64 */
  #define ALWAYS_USE_VSYSCALL 1
  
-#include <sysdeps/unix/sysdep.h>
-#include <sysdeps/aarch64/sysdep.h>
  #include <sysdeps/unix/sysv/linux/generic/sysdep.h>
+#include <sysdeps/aarch64/sysdep.h>
  
  /* Defines RTLD_PRIVATE_ERRNO and USE_DL_SYSINFO.  */
  #include <dl-sysdep.h>