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: >> >> >> On 2018-Jul-25, at 2:10 PM, Mark Millard <marklmi at yahoo.com> wrote: >> >> >> >>> On 2018-Jul-25, at 10:09 AM, Mark Millard <marklmi at yahoo.com> wrote: >>> >>>> On 2018-Jul-25, at 8:39 AM, John Baldwin <jhb at freebsd.org> wrote: >>>> >>>>> 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: >>>>>> >>>>>>> https://ci.freebsd.org/job/FreeBSD-head-amd64-gcc/6597/consoleText >>>>>>> (head -r336573 after the prior 6596's -r336565 ): >>>>>>> >>>>>>> --- 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 >>>>>>> >>>>>>> >>>>>>> https://ci.freebsd.org/job/FreeBSD-head-amd64-gcc/6621/consoleText ( for >>>>>>> -r336700 ) still shows this type of error. >>>>>> >>>>>> >>>>>> [I should have a subject with "head -r336568 through -r336570 . . .".] >>>>>> >>>>>> From what I can tell looking around having something like: >>>>>> >>>>>> if (atomic_fetch_add(&lock->cnt, 1) > 0) >>>>>> >>>>>> involve a __atomic_fetch_add indicates that: >>>>>> >>>>>> /usr/local/lib/gcc/x86_64-unknown-freebsd12.0/6.4.0/include/stdatomic.h >>>>>> >>>>>> was in use instead of FreeBSD's stdatomic.h file. >>>>>> >>>>>> 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 . >>>>>> >>>>>> (I reverted -r335782 in my environment some time ago and have >>>>>> not run into this problem in my context so far.) >>>>> >>>>> 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). >>>>> >>>>> 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. >>>> >>>> I did misread the headers. FreeBSD has the likes of: >>>> >>>> #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) >>>> >>>> so __atomic_fetch_add would occur. >>>> >>>> But so far I do not see the problem with -r335782 reverted. I last built >>>> -r336693 last night via devel/amd64-gcc (via xtoolchain). >>>> >>>> From what I can tell FreeBSD defines: >>>> >>>> #if !defined(__CLANG_ATOMICS) >>>> #define _Atomic(T) struct { volatile T __val; } >>>> #endif >>>> >>>> and that struct is being used in &(object)->__val is what the >>>> error reports are about. So that would be, for example, >>>> >>>> &(&lock->cnt)->__val >>>> >>>> This would appear to suggest that __val itself had a type meeting: >>>> >>>> operand type struct <anonymous> >>>> >>>> for T in _Atomic(T) . >>>> >>>> (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.) >>> >>> Going in a somewhat different direction . . . >>> >>> Looking around I found https://bugs.llvm.org/show_bug.cgi?id=26462 >>> which is titled: >>> >>> 26462 – GCC/clang C11 _Atomic incompatibility >>> >>> 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. >>> >>> An example (a powerpc64 context was indicated): >>> >>> struct A16 { char val[16]; }; >>> _Atomic struct A16 a16; >>> // GCC: >>> _Static_assert(_Alignof(a16) == 16, ""); >>> // Clang: >>> _Static_assert(_Alignof(a16) == 1, ""); >>> >>> >>> Non-power-of-2 is a general problem >>> (not a powerpc64 context from what I can >>> tell): >>> >>> struct A3 { char val[3]; }; >>> _Atomic struct A3 a3; >>> // GCC: >>> _Static_assert(sizeof(a3) == 3, ""); >>> _Static_assert(_Alignof(a3) == 1, ""); >>> // Clang: >>> _Static_assert(sizeof(a3) == 4, ""); >>> _Static_assert(_Alignof(a3) == 4, ""); >>> >>> >>> >>> Comment 6 (by John McCall) is relevant: >>> >>> 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 >>> >>> >>> (I do nto claim to have proivided all the material that should >>> be read in https://bugs.llvm.org/show_bug.cgi?id=26462 .) >>> >>> 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? >>> >>> 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. >>> >>> >> >> >> Going back to the code being compiled and >> confirming your note: >> ( /usr/src/contrib/ofed/librdmacm/cma.h ) >> >> 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); >> } >> >> So lock->cnt is an _Atomic(int) , i.e., >> >> struct { volatile int __val; } >> >> so overall: >> >> typedef struct { >> sem_t sem; >> struct { volatile int __val; } cnt; >> } fastlock_t; >> >> >> for which: atomic_fetch_add(&lock->cnt, 1) has >> for A filled-in in the C11 language official: >> >> atomic_fetch_add (volatile A* obj, M arg) >> >> (generic function) being: >> >> atomic_fetch_add (volatile struct { volatile int __val; }* obj, M arg) >> >> and a direct type-check of that notation for obj would find: >> >> operand type 'struct <anonymous> *' >> >> and that would propagate to GCC's translation to __atomic_fetch_add >> via gcc's stdatomic.h : >> >> #define atomic_fetch_add(PTR, VAL) __atomic_fetch_add ((PTR), (VAL), >> \ >> __ATOMIC_SEQ_CST) >> >> So, yes, it looks like the: >> >> #if !defined(__CLANG_ATOMICS) >> #define _Atomic(T) struct { volatile T __val; } >> #endif > > So where are you seeing this btw? In head the conditional is different: > > #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 > > 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: > > % 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 > > # 29 > "/usr/local/lib/gcc/x86_64-unknown-freebsd11.2/6.4.0/include/stdatomic.h" 3 4 > typedef enum > { > memory_order_relaxed = 0, > memory_order_consume = 1, > memory_order_acquire = 2, > memory_order_release = 3, > memory_order_acq_rel = 4, > memory_order_seq_cst = 5 > } memory_order; > > > typedef _Atomic _Bool atomic_bool; > typedef _Atomic char atomic_char; > ... > > 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. #define __has_feature(x) 0 // Compatibility with non-clang compilers. #endif #ifndef __has_extension #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. === Mark Millard marklmi at yahoo.com ( dsl-only.net went away in early 2018-Mar) _______________________________________________ freebsd-current@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
