Project

General

Profile

Bug #12568

rawspeed: OpenMP with clang <7 issues

Added by Martin Straeten 6 months ago. Updated 6 months ago.

Status:
Fixed
Priority:
Medium
Assignee:
Category:
Buildsystem
Target version:
-
Start date:
01/25/2019
Due date:
% Done:

100%

Affected Version:
git master branch
System:
Mac OS X
bitness:
64-bit
hardware architecture:
amd64/x86

Description

Building current master fails due to recent rawspeed commits:

[ 8%] Building CXX object src/external/rawspeed/CMakeFiles/rawspeed.dir/src/librawspeed/common/RawImage.cpp.o
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp:409:33: error:
variable 'task' must have explicitly specified data sharing attributes
RawImageWorker worker(this, task, y_offset, y_end);
^~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp:405:23: error:
variable 'threads' must have explicitly specified data sharing attributes
for (int i = 0; i < threads; i++) {
^~~~~
~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp:406:47: error:
variable 'height' must have explicitly specified data sharing attributes
int y_offset = std::min(i * y_per_thread, height);
^~~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp:406:33: error:
variable 'y_per_thread' must have explicitly specified data sharing
attributes
int y_offset = std::min(i * y_per_thread, height);
^~~~~~~~~~
4 errors generated.
make2: *** [src/external/rawspeed/CMakeFiles/rawspeed.dir/src/librawspeed/common/RawImage.cpp.o] Error 1

OSX 10.14.2, macports (last updated End of december 2018)

build_darktable_from_scratch.txt Magnifier (24.8 KB) Martin Straeten, 01/26/2019 10:00 AM

dt_build_verbose.txt Magnifier (30.6 KB) Martin Straeten, 01/26/2019 11:15 AM

RawImage.cpp Magnifier (16.4 KB) Martin Straeten, 01/26/2019 12:28 PM

Associated revisions

Revision 4645a8b7
Added by Roman Lebedev 6 months ago

RawSpeed submodule update: Nikon Z 7 / Z 6 uncompressed raws, Kodak DCS Pro 14n

Fixes #12553.
Fixes #12568.

History

#1 Updated by Roman Lebedev 6 months ago

  • % Done changed from 0 to 20
  • Status changed from New to Incomplete

Please do make sure that you have both the darktable's git master,
and the correct rawspeed's commit (don't just randomly pick some commit,
use the one specified by darktable's rawspeed submodule), kill the build dir,
and try again.
(The latest commits should have fixed the issue, not introduced it)

If that does not work, kill the build dir,
and post all the console output starting with the removal of build dir.

#2 Updated by Martin Straeten 6 months ago

i updated the source via git pull --recurse-submodules (latest successful build i did after a git pull was 14. jan)
So i built again from scratch starting with a new git clone (see attached file of console output including cmake output) - same behaviour

#3 Updated by Roman Lebedev 6 months ago

Martin Straeten wrote:

i updated the source via git pull --recurse-submodules (latest successful build i did after a git pull was 14. jan)
So i built again from scratch starting with a new git clone (see attached file of console output including cmake output) - same behaviour

Ok, that is confusing..
Can you show `make VERBOSE=1` output?

#4 Updated by Martin Straeten 6 months ago

done

#5 Updated by Roman Lebedev 6 months ago

This makes no sense.
They do have explicitly specified data sharing attributes:
https://github.com/darktable-org/rawspeed/blob/1ca16013a07d67648fc4d66d845492db0b43f12c/src/librawspeed/common/RawImage.cpp#L400-L404

Can please you check that is what you have in /Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp on those lines ?

#6 Updated by Martin Straeten 6 months ago

#ifdef HAVE_OPENMP
#pragma omp parallel for default(none)                                         \
    firstprivate(threads, y_per_thread, height, task) num_threads(threads)     \
        schedule(static)
#endif
  for (int i = 0; i < threads; i++) {
    int y_offset = std::min(i * y_per_thread, height);
    int y_end = std::min((i + 1) * y_per_thread, height);

    RawImageWorker worker(this, task, y_offset, y_end);
  }
}

#7 Updated by Roman Lebedev 6 months ago

Yep, that makes no sense.

From https://redmine.darktable.org/attachments/4397/dt_build_verbose.txt#L192
The command it runs is:
cd /Users/martinstraeten/src/darktable/build/src/external/rawspeed && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DGDK_DISABLE_DEPRECATED -DGTK_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -DOS_OBJECT_USE_OBJC=0 -D_XOPEN_SOURCE=700 -D__GDK_KEYSYMS_COMPAT_H__ -I/Users/martinstraeten/src/darktable/build/src/external/rawspeed/src -I/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed -isystem /Users/martinstraeten/src/darktable/src/external/rawspeed/src/external -isystem /opt/local/include -stdlib=libc++ -I/opt/local/include/libomp -D_DARWIN_C_SOURCE -Wall -Wformat -Wformat-security -Wshadow -Wtype-limits -Wvla -Wthread-safety -Wno-error=varargs -Wno-error=address-of-packed-member -Wframe-larger-than=32768 -Wlarger-than=524288 -w -stdlib=libc++ -std=c++14 -flto=thin -fstrict-vtable-pointers -Wstack-usage=4096 -Wframe-larger-than=4096 -Wlarger-than=32768 -O2 -g -DNDEBUG -O2 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -mmacosx-version-min=10.7 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -mtune=generic -g3 -ggdb3 -Werror -Xclang -fopenmp -fopenmp-version=40 -std=c++14 -o CMakeFiles/rawspeed.dir/src/librawspeed/common/RawImage.cpp.o -c /Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp

Can you try manually running it, but
  • replacing "-fopenmp-version=40" with "-Xclang -fopenmp-version=40"
  • dropping "-fopenmp-version=40"

#8 Updated by Martin Straeten 6 months ago

I'm not sure about your hint:
i replaced "-Xclang -fopenmp -fopenmp-version=40" with "-Xclang -fopenmp-version=40" which is effectively a dropping of "-fopenmp" an run the command. --> no errors
then back to ~/src/darktable/build
make continues to build up to


[ 43%] Linking C shared library libdarktable.dylib
[ 43%] Built target lib_darktable
Scanning dependencies of target darktable
[ 43%] Building C object src/CMakeFiles/darktable.dir/main.c.o
[ 43%] Linking C executable darktable
[ 43%] Built target darktable
Scanning dependencies of target dependencies
[ 43%] Built target dependencies
Scanning dependencies of target tests
[ 43%] Built target tests
Scanning dependencies of target check
[ 43%] Built target check
Scanning dependencies of target darktable-rs-identify
[ 43%] Building CXX object src/external/rawspeed/src/utilities/identify/CMakeFiles/darktable-rs-identify.dir/rawspeed-identify.cpp.o
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp:266:25: error: 
      variable 'dimUncropped' must have explicitly specified data sharing
      attributes
    for (int y = 0; y < dimUncropped.y; ++y) {
                        ^~~~~~~~~~~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp:267:30: error: 
      variable 'raw' must have explicitly specified data sharing attributes
      uchar8* const data = (*raw)->getDataUncropped(0, y);
                             ^~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp:269:32: error: 
      variable 'bpp' must have explicitly specified data sharing attributes
      for (unsigned x = 0; x < bpp * dimUncropped.x; ++x)
                               ^~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp:282:27: error: 
      variable 'dimUncropped' must have explicitly specified data sharing
      attributes
      for (int y = 0; y < dimUncropped.y; ++y) {
                          ^~~~~~~~~~~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp:284:40: error: 
      variable 'raw' must have explicitly specified data sharing attributes
            reinterpret_cast<float*>((*raw)->getDataUncropped(0, y));
                                       ^~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp:286:34: error: 
      variable 'cpp' must have explicitly specified data sharing attributes
        for (unsigned x = 0; x < cpp * dimUncropped.x; ++x)
                                 ^~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp:299:27: error: 
      variable 'dimUncropped' must have explicitly specified data sharing
      attributes
      for (int y = 0; y < dimUncropped.y; ++y) {
                          ^~~~~~~~~~~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp:301:43: error: 
      variable 'raw' must have explicitly specified data sharing attributes
            reinterpret_cast<uint16_t*>((*raw)->getDataUncropped(0, y));
                                          ^~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp:303:34: error: 
      variable 'cpp' must have explicitly specified data sharing attributes
        for (unsigned x = 0; x < cpp * dimUncropped.x; ++x)
                                 ^~~
9 errors generated.
make[2]: *** [src/external/rawspeed/src/utilities/identify/CMakeFiles/darktable-rs-identify.dir/rawspeed-identify.cpp.o] Error 1
make[1]: *** [src/external/rawspeed/src/utilities/identify/CMakeFiles/darktable-rs-identify.dir/all] Error 2
make: *** [all] Error 2

seems to be the same situaion, so i removed -fopenmp there too:
cd /Users/martinstraeten/src/darktable/build/src/external/rawspeed/src/utilities/identify && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DGDK_DISABLE_DEPRECATED -DGTK_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -DOS_OBJECT_USE_OBJC=0 -DRS_CAMERAS_XML_PATH=\"/usr/local/share/darktable/rawspeed/cameras.xml\" -D_XOPEN_SOURCE=700 -D__GDK_KEYSYMS_COMPAT_H__ -I/Users/martinstraeten/src/darktable/build/src/external/rawspeed/src -I/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed -isystem /Users/martinstraeten/src/darktable/src/external/rawspeed/src/external -stdlib=libc++ -I/opt/local/include/libomp -D_DARWIN_C_SOURCE -Wall -Wformat -Wformat-security -Wshadow -Wtype-limits -Wvla -Wthread-safety -Wno-error=varargs -Wno-error=address-of-packed-member -Wframe-larger-than=32768 -Wlarger-than=524288 -w -stdlib=libc++ -std=c++14 -flto=thin -fstrict-vtable-pointers -Wstack-usage=4096 -Wframe-larger-than=4096 -Wlarger-than=32768 -O2 -g -DNDEBUG -O2 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -mmacosx-version-min=10.7 -fPIE -fvisibility=hidden -fvisibility-inlines-hidden -mtune=generic -g3 -ggdb3 -Werror -Xclang -fopenmp-version=40 -std=c++14 -o CMakeFiles/darktable-rs-identify.dir/rawspeed-identify.cpp.o -c /Users/martinstraeten/src/darktable/src/external/rawspeed/src/utilities/identify/rawspeed-identify.cpp

then back to ~/src/darktable/build
make continues to build up to 100%

#9 Updated by Roman Lebedev 6 months ago

Martin Straeten wrote:

I'm not sure about your hint:
i replaced "-Xclang -fopenmp -fopenmp-version=40" with "-Xclang -fopenmp-version=40" which is effectively a dropping of "-fopenmp" an run the command. --> no errors

Err, no, not what i suggested.
I meant: "-Xclang -fopenmp -Xclang -fopenmp-version=40", i.e. add "-Xclang" before "-fopenmp-version=40"

#10 Updated by Martin Straeten 6 months ago

ok, I'll try doing this after cleaning build directory:
after cmake ... and make up to 7% like former.
then
cd /Users/martinstraeten/src/darktable/build/src/external/rawspeed/src/utilities/identify && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DGDK_DISABLE_DEPRECATED -DGTK_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -DOS_OBJECT_USE_OBJC=0 -D_XOPEN_SOURCE=700 -D__GDK_KEYSYMS_COMPAT_H__ -I/Users/martinstraeten/src/darktable/build/src/external/rawspeed/src -I/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed -isystem /Users/martinstraeten/src/darktable/src/external/rawspeed/src/external -isystem /opt/local/include -stdlib=libc++ -I/opt/local/include/libomp -D_DARWIN_C_SOURCE -Wall -Wformat -Wformat-security -Wshadow -Wtype-limits -Wvla -Wthread-safety -Wno-error=varargs -Wno-error=address-of-packed-member -Wframe-larger-than=32768 -Wlarger-than=524288 -w -stdlib=libc++ -std=c++14 -flto=thin -fstrict-vtable-pointers -Wstack-usage=4096 -Wframe-larger-than=4096 -Wlarger-than=32768 -O2 -g -DNDEBUG -O2 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -mmacosx-version-min=10.7 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -mtune=generic -g3 -ggdb3 -Werror -Xclang -fopenmp -Xclang -fopenmp-version=40 -std=c++14 -o CMakeFiles/rawspeed.dir/src/librawspeed/common/RawImage.cpp.o -c /Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp:406:47: error:
variable 'height' must have explicitly specified data sharing attributes
int y_offset = std::min(i * y_per_thread, height);
^~~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp:406:33: error:
variable 'y_per_thread' must have explicitly specified data sharing
attributes
int y_offset = std::min(i * y_per_thread, height);
^~~~~~~~~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp:409:33: error:
variable 'task' must have explicitly specified data sharing attributes
RawImageWorker worker(this, task, y_offset, y_end);
^~~
/Users/martinstraeten/src/darktable/src/external/rawspeed/src/librawspeed/common/RawImage.cpp:405:23: error:
variable 'threads' must have explicitly specified data sharing attributes
for (int i = 0; i < threads; i++) {
^~~~~
~
4 errors generated.

#11 Updated by Roman Lebedev 6 months ago

I'm sorry, i'm unable to help here.
I don't understand what the problem is.
It seriously looks like that compiler is lobotomized.

Sadly, i can't just revert "to green" either, since those changes were a bugfix for gcc9.

#12 Updated by Martin Straeten 6 months ago

ok for me as long as my incorrect corrections from #8 works for me;
maybe with an update of some macports portsfiles this will be healed ;)
thanks for your help

#13 Updated by Igor Kuzmin 6 months ago

See https://www.mail-archive.com/llvm-bugs@lists.llvm.org/msg20164.html
So the problem seems to be that those variables are const, therefore they are shared, not private (I don't know anything about OpenMP, so I might be talking nonsense). What exactly did gcc9 complained about?

#14 Updated by Roman Lebedev 6 months ago

  • % Done changed from 20 to 10
  • Status changed from Incomplete to Confirmed

Igor Kuzmin wrote:

See https://www.mail-archive.com/llvm-bugs@lists.llvm.org/msg20164.html

Oh, i see, thanks for finding that.
So this only works with clang-7+ https://godbolt.org/z/ngI6hn

So the problem seems to be that those variables are const, therefore they are shared, not private (I don't know anything about OpenMP, so I might be talking nonsense).

What exactly did gcc9 complained about?

There was a change in the OpenMP spec after 3.1 (i.e. in openmp 4).
gcc9 has decided to finally adopt that change. Naturally, they have
no toggles to switch between the openmp versions, so we have to adapt somehow.

What changed? Compare: https://godbolt.org/z/mKFeaQ
Roughly, if you use `const` variable, and use `default(none)`,
then that was ok with openmp3.1, but is not ok with openmp4,
and you need to explicitly specify that variable as either `shared()`
(the previous implicit default), or `firstprivate()` (the better choice, actually)

The caveat is that older gcc's (<9) will complain if you specify "default(none) shared(num)".
But does not complain about "default(none) firstprivate(num)"

But if you specify "default(none) firstprivate(num)", older clang's will complain, apparently :S

I picked the path of "default(none) firstprivate(num)" both because the gcc didn't complain about it,
and because "firstprivate" makes more sense there. If i would have picked "shared" path,
i would need to to `SHARED` macro to do do stuff differently depending on gcc version.
But indeed, i failed to check what different clang versions do.
And there the situation is the opposite, "shared" is ok, "firstprivate" is not ok.
So no matter what i/we choose, it needs to be a macro.
Crap. sigh.

(and yes, building of darktable w/openmp with gcc9 is broken.)

#15 Updated by Roman Lebedev 6 months ago

  • Subject changed from OSX Build rawspeed fails to rawspeed: OpenMP with clang <7 issues
  • Assignee set to Roman Lebedev
  • Priority changed from Low to Medium

#16 Updated by Roman Lebedev 6 months ago

  • % Done changed from 10 to 50
  • Status changed from Confirmed to In Progress

#17 Updated by Roman Lebedev 6 months ago

Please try with current rawspeed git develop.

#18 Updated by Igor Kuzmin 6 months ago

works for me

#19 Updated by Martin Straeten 6 months ago

updating darktable's src/external/rawspeed with rawspeed development results in successful build of darktable.
seems to be ok, thanks

#20 Updated by Roman Lebedev 6 months ago

Igor Kuzmin wrote:

works for me

Martin Straeten wrote:

updating darktable's src/external/rawspeed with rawspeed development results in successful build of darktable.
seems to be ok, thanks

Okay, great, that is relieving to hear.
Will propagate to darktable once i'm satisfied with the fuzzing status.

#21 Updated by Roman Lebedev 6 months ago

  • Status changed from In Progress to Fixed
  • % Done changed from 50 to 100

Also available in: Atom PDF