static builds don't work

mirlip commented 5 years ago

On windows, static builds are very small compared to the dll and don't work with Blender at least. On Linux, when OIDN is compiled as a dynamic library it works. But when build as a static library, building Blender fails at 100%: the blender executable segfault while being called with the -h option to create the manpages. To reproduce the bug:

get Blender with OIDN from here https://github.com/skwerner/blender/tree/openimagedenoise
modify build_files/build_environment/cmake/openimagedenoise.cmake to add -DOIDN_STATIC_LIB=ON
modify build_files/build_environment/cmake/versions.cmake to download a newer version of oidn with the static option and the submodules (will require a new release on your side)
modify build_files/cmake/modules/findopenimagedenoise.cmake to find openimagedenoise, mkldnn and common
modify build_files/cmake/platform/platform_unix.cmake at "if(WITH_OPENIMAGEDENOISE)" with find_package_wrapper instead of find_package
call "make deps"
call "make full"

mirlip commented 5 years ago

Here is a log from a debug build: error_oidn_static.txt

atafra commented 5 years ago

I can't reproduce the very small static build on Windows (with VS 2017). Which compiler did you use? On Linux, based on your log for the debug build, the issue seems to be linking to a wrong (too old) TBB version. It looks like the TBB headers and the linked library are from different TBB versions. Could you please check your TBB installation? Also, could you please try the new 0.8.2 version of OIDN?

mirlip commented 5 years ago

Thanks for the update. The trace is a bit different now using 0.8.2:

tbb::interface7::internal::task_arena_base::internal_max_concurrency(tbb::interface7::task_arena const*) (Unknown Source:0)
_GLOBAL__sub_I_jit_avx512_common_conv_winograd_kernel_f32.cpp (Unknown Source:0)
__libc_csu_init (Unknown Source:0)
__libc_start_main(int (*)(int, char **, char **) main, int argc, char ** argv, int (*)(int, char **, char **) init, void (*)(void) fini, void (*)(void) rtld_fini, void * stack_end) (/build/glibc-OTsEL5/glibc-2.27/csu/libc-start.c:266)
_start (Unknown Source:0)

futex_wait_cancelable(unsigned int expected, unsigned int * futex_word) (/build/glibc-OTsEL5/glibc-2.27/sysdeps/unix/sysv/linux/futex-internal.h:88)
__pthread_cond_wait_common(pthread_mutex_t * mutex, pthread_cond_t * cond) (/build/glibc-OTsEL5/glibc-2.27/nptl/pthread_cond_wait.c:502)
__pthread_cond_wait(pthread_cond_t * cond, pthread_mutex_t * mutex) (/build/glibc-OTsEL5/glibc-2.27/nptl/pthread_cond_wait.c:655)
background_thread_sleep(uint64_t interval) (/home/mirlip/blendergit/build_linux/deps/build/jemalloc/src/external_jemalloc/src/background_thread.c:231)
background_work_sleep_once(unsigned int ind, background_thread_info_t * info) (/home/mirlip/blendergit/build_linux/deps/build/jemalloc/src/external_jemalloc/src/background_thread.c:306)
background_thread0_work(tsd_t * tsd) (/home/mirlip/blendergit/build_linux/deps/build/jemalloc/src/external_jemalloc/src/background_thread.c:450)
background_work(unsigned int ind, tsd_t * tsd) (/home/mirlip/blendergit/build_linux/deps/build/jemalloc/src/external_jemalloc/src/background_thread.c:483)
background_thread_entry(void * ind_arg) (/home/mirlip/blendergit/build_linux/deps/build/jemalloc/src/external_jemalloc/src/background_thread.c:513)
start_thread(void * arg) (/build/glibc-OTsEL5/glibc-2.27/nptl/pthread_create.c:463)
clone() (/build/glibc-OTsEL5/glibc-2.27/sysdeps/unix/sysv/linux/x86_64/clone.S:95)

I ensured tbb is 2019_U5 everywhere in system and Blender own libs. Cmake is version 3.13.4. No error during building. Just a side note, I couldn't find the static version for Linux on the release page to check against an official build. Only .so are in.

atafra commented 5 years ago

It still looks like some kind of issue with TBB. What OS and compiler are you using? After building Blender, how can I reproduce this trace?

We're not planning to include static binaries.

atafra commented 5 years ago

The problem is that I can't even compile Blender by following your instructions. It fails at "make deps" (many downloads from SourceForge fail). Could you please share you modified version of the source tree which should compile out-of-the-box?

mirlip commented 5 years ago

I use Ubuntu 18.04. The error shows up just after linking blender, when the cmake script tries to generate the help by calling blender --help. It segfaults. I can't attach diffs so here is a paste which should be applied on the openimagedenoise branch from skwerner https://github.com/skwerner/blender/tree/openimagedenoise: http://pasteall.org/1571095 building deps may require different packages like yasm/nasm, downloads all work for me, I just tested again to do a fresh make deps and it compiles all deps successfully. Let me know if you need more infos.

atafra commented 5 years ago

Thanks! What are the exact build requirements? I don't have yasm/nasm installed for example.

atafra commented 5 years ago

BTW why is this change needed?

+  /home/john/blendergit/lib/linux_x86_64/openimagedenoise
+  /home/john/blendergit/lib/linux_x86_64/

Isn't the OIDN source downloaded and build as part of the whole "make deps" process?

mirlip commented 5 years ago

It is downloaded and build during make deps indeed. I guess those 2 lines are not really needed and where only a temporary hack from a guy who helped me to get it working. But they don't hurt so I left them. for the build requirement, there is sadly no official doc and my package install history doesn't go far enough to retrieve all the stuff I installed. I just installed them following the errors that showed up. But mesa-dev and libegl1-mesa-dev are also required iirc.

LazyDodo commented 5 years ago

Hi....platform dev for blender on windows here, I have to apologize, @skwerner made an OIDN integration patch available for review on our side, while it is in an early working state, it's nowhere near ready for landing in blender.

The patch specifically mentioned that static linking doesn't work yet. If you want to play with it, that's fair go at it, enjoy, experiments are fun! But please don't go bother other projects and expect them to go build blender with all it's dependencies to solve issues we already know exist. They will be addressed before (and if) this feature lands in blender and if patches in any of our dependencies are needed we'll generally work with them to get them in place. (but I'll be honest, the problems you are seeing may as well be blender build system related, or perhaps as @atafra pointed out in tbb)

@atafra has been rather generous with his time already, but please lets be respectful of his time and good will and not ask him to build several million lines of code spread out over 40+ libraries just to reproduce an issue the author of the patch already told you existed at the time he submitted the patch.

If you can reproduce the issue with a minimal example fair enough, but not all of blender, that's just unreasonable.

atafra commented 5 years ago

Thanks for chiming in, @LazyDodo ! I’m happy to help, but debugging the issue with such a huge project is quite difficult. Do you know whether that patch mentioned this specific issue or the lack of support for static linking? The previous version of OIDN didn’t support static linking at all.

We tried static linking with some small internal projects, and we didn’t encounter any issues. What is quite puzzling about the reported problem is that it seems Blender crashes before calling any OIDN code. Is this correct?

It would certainly help a lot if the issue could be reproduced with a much smaller example. As a first step it would be useful to find out whether statically linking OIDN to a minimal application works in the same environment.

mirlip commented 5 years ago

I didn't find an easier example to reproduce the bug. that's why I tried to give the steps. But it's indeed very cumbersome to get this working. If I can help, I'm ok.

LazyDodo commented 5 years ago

When the patch got submitted there was no static linking at all in OIDN. I spoke to @skwerner this morning who said he had it working on mac but hadn't gotten around to testing on linux/windows just yet. I say just let him work on it, if there's an issue he'll bring it up with almost certainly a better repro case than "build blender"

I appreciate the offer to help but kinda feel we should get our own affairs in order before we lay such a substantial claim on time of other projects.

atafra commented 5 years ago

Thanks for the info! I'll wait for @skwerner 's feedback.

skwerner commented 5 years ago

I just finished my Blender builds with static OIDN 0.8.2 and could not reproduce the issue. Things work as expected.

I hope to publish an updated OIDN patch for Blender later today or tomorrow.

atafra commented 5 years ago

Thanks, @skwerner !

RenderKit / oidn

static builds don't work #19