PR 89864 - gcc fails to build/bootstrap with XCode 10.2

Fixinclude the header file <sys/ucred.h> that incorrectly uses the C-only
_Atomic keyword when compiled as C++. Apply the same work-around for two
GCC source files that transitively use this header file.

Hi Eric,

Thanks for working on this!

> On 4 Apr 2019, at 04:00, Erik Schnetter <schnetter@gmail.com> wrote:
> 
> Fixinclude the header file <sys/ucred.h> that incorrectly uses the C-only
> _Atomic keyword when compiled as C++. Apply the same work-around for two
> GCC source files that transitively use this header file.

1/
If the fix-include is doing the right thing, then there should be no need for a 
workaround in any GCC file including it.

Perhaps the wrap needs to be around the (SDK) header(s) that is/are
including <sys/ucred.h>?

(so if <sys/sysctl.h>  is including <sys/ucred.h>, then the wrap needs to be in that
file, not in the GCC files that use it).

2/
I’m somewhat unhappy that this will hack around the problem and silently do
something that can’t work properly in any case that we started to use the
field/struct.  What didn’t work about your proposed substitution of the appropriate
c++ <atomic> stuff?

thanks
Iain


> 
> Index: fixincludes/inclhack.def
> ===================================================================
> --- fixincludes/inclhack.def (revision 270127)
> +++ fixincludes/inclhack.def (working copy)
> @@ -1298,6 +1298,36 @@ fix = {
> };
> 
> /*
> + *  macOS 10.14.4 <sys/ucred.h> uses the C _Atomic keyword in C++ code.
> + */
> +fix = {
> +    hackname  = darwin_ucred__Atomic;
> +    mach      = "*-*-darwin18.5.*";
> +    files     = sys/ucred.h;
> +    select    = "_Atomic";
> +
> +    c_fix     = wrap;
> +
> +    c_fix_arg = <<- _EOArg_
> +
> + #if defined(__cplusplus) && __cplusplus >= 201103L
> + #  define _Atomic volatile
> + #endif
> +
> + _EOArg_;
> +
> +    c_fix_arg = <<- _EOArg_
> +
> + #if defined(__cplusplus) && __cplusplus >= 201103L
> + #  undef _Atomic
> + #endif
> +
> + _EOArg_;
> +
> +    test_text = "#include <sys/sysctl.h>\n";
> +};
> +
> +/*
>  *  For the AAB_darwin7_9_long_double_funcs fix to be useful,
>  *  you have to not use "" includes.
>  */
> Index: gcc/config/darwin-driver.c
> ===================================================================
> --- gcc/config/darwin-driver.c (revision 270127)
> +++ gcc/config/darwin-driver.c (working copy)
> @@ -27,7 +27,15 @@ along with GCC; see the file COPYING3.
> #include "diagnostic-core.h"
> 
> #ifndef CROSS_DIRECTORY_STRUCTURE
> +#if defined(__cplusplus) && __cplusplus >= 201103L
> +// This is safe because it applies only to struct ucred, which is
> +// not actually used by gcc.
> +#define _Atomic volatile
> +#endif
> #include <sys/sysctl.h>
> +#if defined(__cplusplus) && __cplusplus >= 201103L
> +#undef _Atomic
> +#endif
> #include "xregex.h"
> 
> static char *
> Index: libsanitizer/sanitizer_common/sanitizer_mac.cc
> ===================================================================
> --- libsanitizer/sanitizer_common/sanitizer_mac.cc (revision 270127)
> +++ libsanitizer/sanitizer_common/sanitizer_mac.cc (working copy)
> @@ -67,7 +67,13 @@ extern "C" {
> #include <sys/mman.h>
> #include <sys/resource.h>
> #include <sys/stat.h>
> +#if defined(__cplusplus) && __cplusplus >= 201103L
> +#  define _Atomic volatile
> +#endif
> #include <sys/sysctl.h>
> +#if defined(__cplusplus) && __cplusplus >= 201103L
> +#  undef _Atomic
> +#endif
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <unistd.h>
> 
> 
> -- 
> Erik Schnetter <schnetter@gmail.com>
> http://www.perimeterinstitute.ca/personal/eschnetter/

On Thu, Apr 4, 2019 at 3:11 AM Iain Sandoe <idsandoe@googlemail.com> wrote:

> Hi Eric,
>
> Thanks for working on this!
>
> > On 4 Apr 2019, at 04:00, Erik Schnetter <schnetter@gmail.com> wrote:
> >
> > Fixinclude the header file <sys/ucred.h> that incorrectly uses the C-only
> > _Atomic keyword when compiled as C++. Apply the same work-around for two
> > GCC source files that transitively use this header file.
>
> 1/
> If the fix-include is doing the right thing, then there should be no need
> for a
> workaround in any GCC file including it.
>
> Perhaps the wrap needs to be around the (SDK) header(s) that is/are
> including <sys/ucred.h>?
>
> (so if <sys/sysctl.h>  is including <sys/ucred.h>, then the wrap needs to
> be in that
> file, not in the GCC files that use it).
>

Okay, that's good to know. I will update the patch accordingly.

2/
> I’m somewhat unhappy that this will hack around the problem and silently do
> something that can’t work properly in any case that we started to use the
> field/struct.  What didn’t work about your proposed substitution of the
> appropriate
> c++ <atomic> stuff?
>

What doesn't work was that this requires changing <sys/ucred.h>. If only
<sys/sysctl.h> can be changed, then it is not possible to replace _Atomic
ulong by _Atomic(ulong) (adding parentheses). I'll try fixincluding both
files.

-erik

Hi Erik,

> On 4 Apr 2019, at 12:56, Erik Schnetter <schnetter@gmail.com> wrote:
> 
> On Thu, Apr 4, 2019 at 3:11 AM Iain Sandoe <idsandoe@googlemail.com> wrote:

> What doesn't work was that this requires changing <sys/ucred.h>. If only <sys/sysctl.h> can be changed, then it is not possible to replace _Atomic ulong by _Atomic(ulong) (adding parentheses). I'll try fixincluding both files.

That might be what’s needed (although both fixes really need to be conditional on the same guard).

Please note the additional comments in the PR (this is potentially an ABI breaking change, but then I suppose so is the SDK change).  We should at minimum test that the size and alignment of the types are the same!

It’s somewhat of an additional problem that we can’t easily check for the SDK version to base fixes on known broken SDKs - it’s not really an issue of the OS revision (we’re relying on the match case + the OS version being unique ‘enough’).

again, thanks for working on this,
Iain