__TBB_machine_fetchadd4 undefined on s390x and aarch64

[jamesjer]

I attempted to build 2019 update 9 for Fedora. The build failed on s390x and aarch64, both with this error message:

In file included from ../../src/tbb/tools_api/ittnotify_static.c:17,
                 from ../../src/tbb/itt_notify.cpp:43:
../../src/tbb/tools_api/ittnotify_config.h: In function 'long int __itt_interlocked_increment(volatile long int*)':
../../src/tbb/tools_api/ittnotify_config.h:334:12: error: '__TBB_machine_fetchadd4' was not declared in this scope
  334 |     return __TBB_machine_fetchadd4(ptr, 1) + 1L;
      |            ^~~~~~~~~~~~~~~~~~~~~~~
make[1]: *** [../../build/common_rules.inc:80: itt_notify.o] Error 1

This is a regression over update 8; both architectures built successfully in that version.

[jamesjer]

It appears that src/tbb/tools_api/ittnotify_config.h tries to define its own __TBB_machine_fetchadd4. It does so incompletely, since there are missing architectures, and it uses the old __sync interface on arm and ppc64. Why is it not using the more modern and more complete definitions in include/tbb/machine?

[alexey-katranov]

../../src/tbb/itt_notify.cpp:43 is compiled only when DO_ITT_NOTIFY is defined. I would expect that DO_ITT_NOTIFY is not defined on s390x and aarch64. What arguments are passed to make?

[nerijus]

Fedora package uses such patch for this issue:

diff -up tbb-2019_U9/src/tbb/tools_api/ittnotify_config.h.orig tbb-2019_U9/src/tbb/tools_api/ittnotify_config.h
--- tbb-2020.1/src/tbb/tools_api/ittnotify_config.h.orig        2020-01-21 04:26:46.000000000 -0700
+++ tbb-2020.1/src/tbb/tools_api/ittnotify_config.h     2020-01-22 20:59:51.911673011 -0700
@@ -335,7 +335,7 @@ ITT_INLINE long
 __itt_interlocked_increment(volatile long* ptr) ITT_INLINE_ATTRIBUTE;
 ITT_INLINE long __itt_interlocked_increment(volatile long* ptr)
 {
-    return __TBB_machine_fetchadd4(ptr, 1) + 1L;
+    return __atomic_fetch_add(ptr, 1L, __ATOMIC_SEQ_CST) + 1L;
 }
 #endif /* ITT_SIMPLE_INIT */

[alexey-katranov]

Have you tried to disable the tools api with -DDO_ITT_NOTIFY=0 ?

[nerijus]

No, I don't have aarch64 or s390. From the spec file:

# Fix compilation on aarch64 and s390x.  See
# https://github.com/intel/tbb/issues/186
Patch4: tbb-2019-fetchadd4.patch

[nerijus]

But if the 2nd comment is correct, why not to fix it correctly, instead of undefining DO_ITT_NOTIFY?

[alexey-katranov]

I am afraid that nobody really tests tools api for non Intel architectures. Overall, it makes sense to apply the fix. Does the issue present in the current version (oneTBB 2021)?

As for the fix, I think it is better to improve the conditions near lines 330-332. The main concern is whether all compilers support __atomic_fetch_add.

[nerijus]

I am afraid that nobody really tests tools api for non Intel architectures. Overall, it makes sense to apply the fix. Does the issue present in the current version (oneTBB 2021)?

Yes, and patch applies cleanly.

[pkubaj]

Same issue happens on ppc:

/usr/bin/c++ -D__TBB_BUILD -D__TBB_USE_ITT_NOTIFY -I/wrkdirs/usr/ports/devel/onetbb/work/oneTBB-2021.2.0/src/tbb/../../include -O2 -pipe -fstack-protector-strong -fno-strict-aliasing -O2 -pipe -fstack-protector-strong -fno-strict-aliasing -fPIC -MMD -Wall -Wextra -pthread -std=c++11 -MD -MT src/tbb/CMakeFiles/tbb.dir/itt_notify.cpp.o -MF src/tbb/CMakeFiles/tbb.dir/itt_notify.cpp.o.d -o src/tbb/CMakeFiles/tbb.dir/itt_notify.cpp.o -c /wrkdirs/usr/ports/devel/onetbb/work/oneTBB-2021.2.0/src/tbb/itt_notify.cpp
c++: warning: argument unused during compilation: '-MD' [-Wunused-command-line-argument]
In file included from /wrkdirs/usr/ports/devel/onetbb/work/oneTBB-2021.2.0/src/tbb/itt_notify.cpp:43:
In file included from /wrkdirs/usr/ports/devel/onetbb/work/oneTBB-2021.2.0/src/tbb/tools_api/ittnotify_static.c:17:
/wrkdirs/usr/ports/devel/onetbb/work/oneTBB-2021.2.0/src/tbb/tools_api/ittnotify_config.h:338:12: error: use of undeclared identifier '__TBB_machine_fetchadd4'
    return __TBB_machine_fetchadd4(ptr, 1) + 1L;

[alexey-katranov]

@pkubaj ,
does the fix work?

-    return __TBB_machine_fetchadd4(ptr, 1) + 1L;
+    return __atomic_fetch_add(ptr, 1L, __ATOMIC_SEQ_CST) + 1L;

[pkubaj]

@pkubaj ,
does the fix work?

-    return __TBB_machine_fetchadd4(ptr, 1) + 1L;
+    return __atomic_fetch_add(ptr, 1L, __ATOMIC_SEQ_CST) + 1L;

Yes. Can you commit that patch?

[felixonmars]

I have opened #550 to fix the define checks so the old __sync interface would be used correctly, instead of failing with an error.

The right fix seems to be defining __TBB_machine_fetchadd4 to use __atomic_fetch_add for these supported architectures, if I'm not mistaken?

[anton-potapov]

closing as #550 landed