Opened 2 months ago

Closed 6 weeks ago

#26779 closed defect (implemented)

atomic_init() and friends failing on clang

Reported by: mikeperry Owned by: nickm
Priority: High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: jenkins, regression, 032-backport, 033-backport, 034-backport, 035-must
Cc: Actual Points:
Parent ID: Points:
Reviewer: mikeperry Sponsor:

Description

Clang does not like how we're using atomic_init() and friends in src/lib/thread/threads.h. It looks like these changes were introduced by nickm. I'm not finding a lot of great documentation on these functions, though.

https://jenkins.torproject.org/job/tor-ci-linux-master-clang/3164/ARCHITECTURE=i386,SUITE=sid/consoleFulll
https://jenkins.torproject.org/view/Failed+Unstable/job/tor-ci-linux-0.3.3-clang/lastFailedBuild/ARCHITECTURE=i386,SUITE=sid/console

0.3.4 also, but lost the url in the jenkins maze.

Child Tickets

Change History (11)

comment:1 Changed 2 months ago by weasel

e.g.

23:02:37 In file included from src/lib/compress/compress.c:36:
23:02:37 ./src/lib/thread/threads.h:128:3: error: address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(unsigned int) *' invalid)
23:02:37   atomic_init(&counter->val, 0);
23:02:37   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:02:37 /usr/bin/../lib/gcc/i686-linux-gnu/7.3.0/include/stdatomic.h:83:3: note: expanded from macro 'atomic_init'
23:02:37   atomic_store_explicit (PTR, VAL, __ATOMIC_RELAXED)
23:02:37   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:02:37 /usr/bin/../lib/gcc/i686-linux-gnu/7.3.0/include/stdatomic.h:126:5: note: expanded from macro 'atomic_store_explicit'
23:02:37     __atomic_store (__atomic_store_ptr, &__atomic_store_tmp, (MO));     \
23:02:37     ^               ~~~~~~~~~~~~~~~~~~
23:02:37 In file included from src/lib/compress/compress.c:36:
23:02:37 ./src/lib/thread/threads.h:140:10: error: address argument to atomic operation must be a pointer to integer or pointer ('atomic_size_t *' (aka '_Atomic(unsigned int) *') invalid)
23:02:37   (void) atomic_fetch_add(&counter->val, add);
23:02:37          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:02:37 /usr/bin/../lib/gcc/i686-linux-gnu/7.3.0/include/stdatomic.h:192:36: note: expanded from macro 'atomic_fetch_add'
23:02:37 #define atomic_fetch_add(PTR, VAL) __atomic_fetch_add ((PTR), (VAL),    \
23:02:37                                    ^                   ~~~~~
23:02:37 In file included from src/lib/compress/compress.c:36:
23:02:37 ./src/lib/thread/threads.h:146:10: error: address argument to atomic operation must be a pointer to integer or pointer ('atomic_size_t *' (aka '_Atomic(unsigned int) *') invalid)
23:02:37   (void) atomic_fetch_sub(&counter->val, sub);
23:02:37          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:02:37 /usr/bin/../lib/gcc/i686-linux-gnu/7.3.0/include/stdatomic.h:197:36: note: expanded from macro 'atomic_fetch_sub'
23:02:37 #define atomic_fetch_sub(PTR, VAL) __atomic_fetch_sub ((PTR), (VAL),    \
23:02:37                                    ^                   ~~~~~
23:02:37 In file included from src/lib/compress/compress.c:36:
23:02:37 ./src/lib/thread/threads.h:152:10: error: address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(unsigned int) *' invalid)
23:02:37   return atomic_load(&counter->val);
23:02:37          ^~~~~~~~~~~~~~~~~~~~~~~~~~
23:02:37 /usr/bin/../lib/gcc/i686-linux-gnu/7.3.0/include/stdatomic.h:142:27: note: expanded from macro 'atomic_load'
23:02:37 #define atomic_load(PTR)  atomic_load_explicit (PTR, __ATOMIC_SEQ_CST)
23:02:37                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:02:37 /usr/bin/../lib/gcc/i686-linux-gnu/7.3.0/include/stdatomic.h:138:5: note: expanded from macro 'atomic_load_explicit'
23:02:37     __atomic_load (__atomic_load_ptr, &__atomic_load_tmp, (MO));        \
23:02:37     ^              ~~~~~~~~~~~~~~~~~
23:02:37 In file included from src/lib/compress/compress.c:36:
23:02:37 ./src/lib/thread/threads.h:158:10: error: address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(unsigned int) *' invalid)
23:02:37   return atomic_exchange(&counter->val, newval);
23:02:37          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:02:37 /usr/bin/../lib/gcc/i686-linux-gnu/7.3.0/include/stdatomic.h:157:3: note: expanded from macro 'atomic_exchange'
23:02:37   atomic_exchange_explicit (PTR, VAL, __ATOMIC_SEQ_CST)
23:02:37   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:02:37 /usr/bin/../lib/gcc/i686-linux-gnu/7.3.0/include/stdatomic.h:151:5: note: expanded from macro 'atomic_exchange_explicit'
23:02:37     __atomic_exchange (__atomic_exchange_ptr, &__atomic_exchange_val,   \
23:02:37     ^                  ~~~~~~~~~~~~~~~~~~~~~
23:02:37 5 errors generated.
23:02:37 Makefile:8039: recipe for target 'src/lib/compress/compress.o' failed
23:02:37 make[1]: *** [src/lib/compress/compress.o] Error 1

comment:2 Changed 2 months ago by nickm

atomic_init() and friends are part of C11; they're declared in stdatomic.h.

It looks like this is maybe 32-bit clang only? Or maybe only some versions of clang? I ask because clang on Travis doesn't object to this code.

And for that matter, I think 32-bit clang didn't object to this code a while ago: these tests used to pass on Jenkins, until a couple of weeks ago: https://jenkins.torproject.org/job/tor-ci-linux-0.3.3-clang/ . We've been using stdatomic.h for much longer than that.

comment:3 Changed 2 months ago by nickm

Looking at the debian packages involved, it seems that sid got some new clang packages around Jul 8 maybe, which fits with the timeline.

comment:4 Changed 2 months ago by nickm

Keywords: regression 032-backport 033-backport 034-backport 035-must added
Milestone: Tor: 0.3.3.x-finalTor: 0.3.5.x-final
Priority: MediumHigh

comment:5 Changed 2 months ago by nickm

Here is a minimal C file that demonstrates the issue:

#include <stdatomic.h>

void increment(atomic_size_t *arg) {
	atomic_fetch_add(arg, 1);
}

comment:6 Changed 2 months ago by nickm

Leading theory after more investigation with weasel: clang is looking at the version of stdatomic.h in /usr/lib/gcc (see error messages above), but it shouldn't be. It wants the one in /usr/lib/llvm-6.0.

Last edited 2 months ago by weasel (previous) (diff)

comment:7 Changed 2 months ago by weasel

Last edited 2 months ago by weasel (previous) (diff)

comment:8 Changed 7 weeks ago by nickm

Status: assignedneeds_review

I don't see much action on that ticket, so it looks like we should add a workaround.

Branch bug26779_033 should merge into 0.3.3 and 0.3.4 cleanly; branch bug26779_035 merges it into 0.3.5.

See PR at https://github.com/torproject/tor/pull/269 for the 033 version.

comment:9 Changed 6 weeks ago by asn

Reviewer: mikeperry

comment:10 Changed 6 weeks ago by mikeperry

Status: needs_reviewmerge_ready

Ok this seems plausible to me that it detects a working stdatomic, and if not, falls back to our previous mutex-based functions.

I'm setting this to merge ready, but with the caveat that I'm not the best person to review this to ensure that all the stdatomic stuff is sane. But the ifdef changes check out. The 033 branch does appear to replace all usage of HAVE_STDATOMIC_H with HAVE_WORKING_STDATOMIC.

comment:11 Changed 6 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to 0.2.9 and forward; let's see what CI thinks of it.

Note: See TracTickets for help on using tickets.