Date: Thu, 26 Jul 2018 10:55:58 -0700 From: Mark Millard <marklmi@yahoo.com> To: John Baldwin <jhb@FreeBSD.org> Cc: Konstantin Belousov <kib@freebsd.org>, FreeBSD Current <freebsd-current@freebsd.org> Subject: Re: head -r336568 and -r336570 appears to have made ci.freebsg.org's FreeBSD-head-amd64-gcc fail either than it had been (error: operand type 'struct <anonymous> *' is incompatible with argument 1 of '__atomic_fetch_add') Message-ID: <0103123A-2D77-4D64-8FF6-97CD521CA7A8@yahoo.com> In-Reply-To: <1ea2a8d0-b27a-503a-0a8b-48d7fbcd8fcb@FreeBSD.org> References: <AED126D8-AFB9-4BF6-81AF-A3CE5F16D2AB@yahoo.com> <EDDB87CC-3CC6-4A71-AF6D-B193F26BB692@yahoo.com> <95fdbf29-6c11-77a6-27a3-2d0dc30f1668@FreeBSD.org> <788B1EE7-EFC9-4AD4-9FD1-9876D0121189@yahoo.com> <9D40F38E-F1DC-4A3F-8792-09AD30D8802B@yahoo.com> <D06CD69A-F0E5-4935-8B64-D1ADB7B6D90A@yahoo.com> <1ea2a8d0-b27a-503a-0a8b-48d7fbcd8fcb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2018-Jul-26, at 10:21 AM, John Baldwin <jhb at FreeBSD.org> wrote: > On 7/25/18 6:52 PM, Mark Millard wrote: >>=20 >>=20 >> On 2018-Jul-25, at 2:10 PM, Mark Millard <marklmi at yahoo.com> = wrote: >>=20 >>=20 >>=20 >>> On 2018-Jul-25, at 10:09 AM, Mark Millard <marklmi at yahoo.com> = wrote: >>>=20 >>>> On 2018-Jul-25, at 8:39 AM, John Baldwin <jhb at freebsd.org> = wrote: >>>>=20 >>>>> On 7/24/18 11:39 PM, Mark Millard wrote: >>>>>> On 2018-Jul-24, at 10:32 PM, Mark Millard <marklmi at yahoo.com> = wrote: >>>>>>=20 >>>>>>> = https://ci.freebsd.org/job/FreeBSD-head-amd64-gcc/6597/consoleText >>>>>>> (head -r336573 after the prior 6596's -r336565 ): >>>>>>>=20 >>>>>>> --- all_subdir_lib/ofed --- >>>>>>> In file included from = /workspace/src/contrib/ofed/librdmacm/cma.h:43:0, >>>>>>> from /workspace/src/contrib/ofed/librdmacm/acm.c:42: >>>>>>> /workspace/src/contrib/ofed/librdmacm/cma.h: In function = 'fastlock_init': >>>>>>> /workspace/src/contrib/ofed/librdmacm/cma.h:60:2: error: invalid = initializer >>>>>>> atomic_store(&lock->cnt, 0); >>>>>>> ^ >>>>>>> In file included from = /workspace/src/contrib/ofed/librdmacm/acm.c:42:0: >>>>>>> /workspace/src/contrib/ofed/librdmacm/cma.h: In function = 'fastlock_acquire': >>>>>>> /workspace/src/contrib/ofed/librdmacm/cma.h:68:2: error: operand = type 'struct <anonymous> *' is incompatible with argument 1 of = '__atomic_fetch_add' >>>>>>> if (atomic_fetch_add(&lock->cnt, 1) > 0) >>>>>>> ^~ >>>>>>> /workspace/src/contrib/ofed/librdmacm/cma.h: In function = 'fastlock_release': >>>>>>> /workspace/src/contrib/ofed/librdmacm/cma.h:73:2: error: operand = type 'struct <anonymous> *' is incompatible with argument 1 of = '__atomic_fetch_sub' >>>>>>> if (atomic_fetch_sub(&lock->cnt, 1) > 1) >>>>>>> ^~ >>>>>>> . . . >>>>>>> --- all_subdir_lib/ofed --- >>>>>>> *** [acm.o] Error code 1 >>>>>>>=20 >>>>>>>=20 >>>>>>> = https://ci.freebsd.org/job/FreeBSD-head-amd64-gcc/6621/consoleText ( for >>>>>>> -r336700 ) still shows this type of error. >>>>>>=20 >>>>>>=20 >>>>>> [I should have a subject with "head -r336568 through -r336570 . . = .".] >>>>>>=20 >>>>>> =46rom what I can tell looking around having something like: >>>>>>=20 >>>>>> if (atomic_fetch_add(&lock->cnt, 1) > 0) >>>>>>=20 >>>>>> involve a __atomic_fetch_add indicates that: >>>>>>=20 >>>>>> = /usr/local/lib/gcc/x86_64-unknown-freebsd12.0/6.4.0/include/stdatomic.h >>>>>>=20 >>>>>> was in use instead of FreeBSD's stdatomic.h file. >>>>>>=20 >>>>>> If this is right, then the issue may be tied to head -r335782 >>>>>> implicitly changing the order of the include file directory >>>>>> searching for builds via the devel/*-gcc . >>>>>>=20 >>>>>> (I reverted -r335782 in my environment some time ago and have >>>>>> not run into this problem in my context so far.) >>>>>=20 >>>>> C11 atomics should work fine with compiler-provided headers since = they >>>>> are a part of the language (and the system stdatomic.h simply = attempts >>>>> to mimic the compiler-provided header in case it is missing). >>>>>=20 >>>>> Simple standalone tests of _Atomic(int) with GCC don't trigger = those >>>>> failures when using its stdatomic.h, so there is probably = something else >>>>> going on with kernel includes being used while building the = library, >>>>> etc. The last time we had this issue with stdarg.h it was because = a >>>>> header shared between the kernel and userland always used = '<machine/stdarg.h>' >>>>> which is correct for the kernel but not for userland. >>>>=20 >>>> I did misread the headers. FreeBSD has the likes of: >>>>=20 >>>> #if defined(__CLANG_ATOMICS) >>>> . . . >>>> #define atomic_fetch_add_explicit(object, operand, order) = \ >>>> __c11_atomic_fetch_add(object, operand, order) >>>> . . . >>>> #elif defined(__GNUC_ATOMICS) >>>> . . . >>>> #define atomic_fetch_add_explicit(object, operand, order) = \ >>>> __atomic_fetch_add(&(object)->__val, operand, order) >>>> . . . >>>> #endif >>>> . . . >>>> #define atomic_fetch_add(object, operand) = \ >>>> atomic_fetch_add_explicit(object, operand, memory_order_seq_cst) >>>>=20 >>>> so __atomic_fetch_add would occur. >>>>=20 >>>> But so far I do not see the problem with -r335782 reverted. I last = built >>>> -r336693 last night via devel/amd64-gcc (via xtoolchain). >>>>=20 >>>> =46rom what I can tell FreeBSD defines: >>>>=20 >>>> #if !defined(__CLANG_ATOMICS) >>>> #define _Atomic(T) struct { volatile T = __val; } >>>> #endif >>>>=20 >>>> and that struct is being used in &(object)->__val is what the >>>> error reports are about. So that would be, for example, >>>>=20 >>>> &(&lock->cnt)->__val >>>>=20 >>>> This would appear to suggest that __val itself had a type meeting: >>>>=20 >>>> operand type struct <anonymous> >>>>=20 >>>> for T in _Atomic(T) . >>>>=20 >>>> (This is independent of just what the issue traces back to: just >>>> the net result on ci.freebsd.org . No claim that you are right >>>> or wrong here. I'll not be looking any more until this afternoon >>>> or night.) >>>=20 >>> Going in a somewhat different direction . . . >>>=20 >>> Looking around I found https://bugs.llvm.org/show_bug.cgi?id=3D26462 >>> which is titled: >>>=20 >>> 26462 =E2=80=93 GCC/clang C11 _Atomic incompatibility >>>=20 >>> It appears that the normal source of platform ABI definitions are >>> not explicit/detailed in the area and allow for incompatibilities >>> in this area. clang and gcc made differing choices absent being >>> constrained to match. >>>=20 >>> An example (a powerpc64 context was indicated): >>>=20 >>> struct A16 { char val[16]; };=20 >>> _Atomic struct A16 a16;=20 >>> // GCC: >>> _Static_assert(_Alignof(a16) =3D=3D 16, "");=20 >>> // Clang: >>> _Static_assert(_Alignof(a16) =3D=3D 1, "");=20 >>>=20 >>>=20 >>> Non-power-of-2 is a general problem >>> (not a powerpc64 context from what I can >>> tell): >>>=20 >>> struct A3 { char val[3]; }; >>> _Atomic struct A3 a3; >>> // GCC: >>> _Static_assert(sizeof(a3) =3D=3D 3, ""); >>> _Static_assert(_Alignof(a3) =3D=3D 1, ""); >>> // Clang: >>> _Static_assert(sizeof(a3) =3D=3D 4, ""); >>> _Static_assert(_Alignof(a3) =3D=3D 4, ""); >>>=20 >>>=20 >>>=20 >>> Comment 6 (by John McCall) is relevant: >>>=20 >>> QUOTE >>> Anyway, while I prefer the Clang rule, the GCC rule is defensible, = as are any number of other rules. The important point, however, is that = having this discussion is not the right approach to solving this = problem. The layout of _Atomic(T) is ABI. ABI rules are not generally = determined by compiler implementors making things up as they go along, = or at least they shouldn't be. The Darwin ABI for _Atomic is the rule = implemented in Clang, which we actually did think about carefully when = we adopted it. Other platforms need to make their own call, and it = probably shouldn't just be "whatever's implemented in GCC", especially = on other platforms where GCC is not the system compiler. >>> END QUOTE >>>=20 >>>=20 >>> (I do nto claim to have proivided all the material that should >>> be read in https://bugs.llvm.org/show_bug.cgi?id=3D26462 .) >>>=20 >>> It may be that FreeBSD needs to be the source of the ABI definitions >>> involved if clang and gcc freeBSD builds are to be interoperable in >>> this area. But this could mean avoiding builtins? >>>=20 >>> If any of this is inlined and so not behind a more stable interface, >>> it looks like clang and gcc can not be mixed for the same instances >>> of various _Atomic possibilities. >>>=20 >>>=20 >>=20 >>=20 >> Going back to the code being compiled and=20 >> confirming your note: >> ( /usr/src/contrib/ofed/librdmacm/cma.h ) >>=20 >> typedef struct { >> sem_t sem; >> _Atomic(int) cnt; >> } fastlock_t; >> . . . >> static inline void fastlock_acquire(fastlock_t *lock) >> { >> if (atomic_fetch_add(&lock->cnt, 1) > 0) >> sem_wait(&lock->sem); >> } >>=20 >> So lock->cnt is an _Atomic(int) , i.e., >>=20 >> struct { volatile int __val; } >>=20 >> so overall: >>=20 >> typedef struct { >> sem_t sem; >> struct { volatile int __val; } cnt; >> } fastlock_t; >>=20 >>=20 >> for which: atomic_fetch_add(&lock->cnt, 1) has >> for A filled-in in the C11 language official: >>=20 >> atomic_fetch_add (volatile A* obj, M arg) >>=20 >> (generic function) being: >>=20 >> atomic_fetch_add (volatile struct { volatile int __val; }* obj, M = arg) >>=20 >> and a direct type-check of that notation for obj would find: >>=20 >> operand type 'struct <anonymous> *' >>=20 >> and that would propagate to GCC's translation to __atomic_fetch_add >> via gcc's stdatomic.h : >>=20 >> #define atomic_fetch_add(PTR, VAL) __atomic_fetch_add ((PTR), (VAL), = \ >> __ATOMIC_SEQ_CST) >>=20 >> So, yes, it looks like the: >>=20 >> #if !defined(__CLANG_ATOMICS) >> #define _Atomic(T) struct { volatile T = __val; } >> #endif >=20 > So where are you seeing this btw? In head the conditional is = different: >=20 > #if !defined(__cplusplus) && !__has_extension(c_atomic) && \ > !__has_extension(cxx_atomic) > /* > * No native support for _Atomic(). Place object in structure to = prevent > * most forms of direct non-atomic access. > */ > #define _Atomic(T) struct { T volatile __val; } > #endif >=20 > And external GCC does support those extensions so should end up not = defining > an _Atomic macro, and in fact when I use -E with external GCC it = leaves > _Atomic(int) alone: >=20 > % x86_64-unknown-freebsd11.2-gcc -E bar.c > ... > # 1 "bar.c" > # 1 "/usr/include/sys/cdefs.h" 1 3 4 > # 2 "bar.c" 2 > # 1 = "/usr/local/lib/gcc/x86_64-unknown-freebsd11.2/6.4.0/include/stdatomic.h" = 1 3 4 > # 29 = "/usr/local/lib/gcc/x86_64-unknown-freebsd11.2/6.4.0/include/stdatomic.h" = 3 4 >=20 > # 29 = "/usr/local/lib/gcc/x86_64-unknown-freebsd11.2/6.4.0/include/stdatomic.h" = 3 4 > typedef enum > { > memory_order_relaxed =3D 0, > memory_order_consume =3D 1, > memory_order_acquire =3D 2, > memory_order_release =3D 3, > memory_order_acq_rel =3D 4, > memory_order_seq_cst =3D 5 > } memory_order; >=20 >=20 > typedef _Atomic _Bool atomic_bool; > typedef _Atomic char atomic_char; > ... >=20 > So cdefs.h isn't overriding _Atomic and should be using the = CLANG_ATOMICS case > for external GCC. Some of my research was when I did not have FreeBSD available and was = over the web and I did not cross check all of it later. I apparently looked at some older source at some point. I now see what you report for the #if test. Sorry for the confusion. However, there is in /usr/src/sys/sys/cdefs.h : /* * Testing against Clang-specific extensions. */ #ifndef __has_attribute #define __has_attribute(x) 0 #endif #ifndef __has_extension #define __has_extension __has_feature #endif #ifndef __has_feature #define __has_feature(x) 0 #endif so if those are clang specific then: #if !defined(__cplusplus) && !__has_extension(c_atomic) && \ !__has_extension(cxx_atomic) will test for C code: !0 && !0 && !0 and then the code will select to: #define _Atomic(T) struct { T volatile __val; } https://clang.llvm.org/docs/LanguageExtensions.html (for clang 7) lists: QUOTE __has_feature and __has_extension These function-like macros take a single identifier argument that is the = name of a feature. __has_feature evaluates to 1 if the feature is both = supported by Clang and standardized in the current language standard or = 0 if not (but see below), while __has_extension evaluates to 1 if the = feature is supported by Clang in the current language (either as a = language extension or a standard language feature) or 0 if not. They can = be used like this: #ifndef __has_feature // Optional of course. =20 #define __has_feature(x) 0 // Compatibility with non-clang compilers. #endif #ifndef __has_extension =20 #define __has_extension __has_feature // Compatibility with pre-3.0 = compilers. #endif END QUOTE so I think that: #define _Atomic(T) struct { T volatile __val; } is being done. =3D=3D=3D Mark Millard marklmi at yahoo.com ( dsl-only.net went away in early 2018-Mar)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0103123A-2D77-4D64-8FF6-97CD521CA7A8>