On 7/26/18 10:55 AM, Mark Millard wrote:
>
>
> 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.
Yes, but the -E from above was when compiled with external GCC and it didn't
change
_Atomic(int). Here's part of the source of bar.c:
#include <sys/cdefs.h>
#include <stdatomic.h>
struct foo {
_Atomic(int) one;
_Atomic int two;
atomic_int three;
};
And here is what that became after -E:
# 4 "bar.c"
struct foo {
_Atomic(int) one;
_Atomic int two;
atomic_int three;
};
So cdefs.h did not define _Atomic.
Ah, if I add '-std=c99' then it does break. Code that wants to use
C11 atomics via <stdatomic.h> should not be using -std=c99. Try this:
Index: lib/ofed/librdmacm/Makefile
===================================================================
--- lib/ofed/librdmacm/Makefile (revision 335896)
+++ lib/ofed/librdmacm/Makefile (working copy)
@@ -8,6 +8,7 @@
SHLIB_MAJOR= 1
MK_PROFILE= no
CFLAGS+= -I${_spath}
+CSTD= gnu11
SRCS= \
acm.c \
If this works then we should probably mark OFED as a BROKEN_OPTION when
building with ancient GCC for now as well.
--
John Baldwin
_______________________________________________
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"
