Page MenuHome

CMake: add config for developers
ClosedPublic

Authored by Brecht Van Lommel (brecht) on Jun 28 2019, 2:18 PM.

Details

Summary

This has faster builds, error checks and tests. The number of cmake options
for this type of thing has grown over the years and it's convenient to be
able to point new developers to a single target.

A remaining issue that @LazyDodo (LazyDodo) brought up is that Release/RelWithDebInfo
builds don't have asserts. That should get enabled through this config as
well but it probably needs a new cmake option first.

There may be other useful options I forgot.

Diff Detail

Repository
rB Blender

Event Timeline

Have a mixed felling of enabling assert() for release build configuration, but if we document specific behavior in the Wiki (where we would advertise this config for developers i am imagining) think it's fine.

If we don't care much about benchmarkability, what about enabling WITH_CYCLES_DEBUG ?
Are there any downsides enabling WITH_MEM_VALGRIND ?

build_files/cmake/config/blender_develop.cmake
12 ↗(On Diff #16109)

I had issue in the past with this. Some sort of initialization fiasco when static object gets allocated using non-guarded allocator, but then is attempted to be freed using a guarded one. AFAIR, was related to Audaspace, but might be mis-remembering.

Would be cool if it works, but needs to be double-checked.

  • CMAKE_INSTALL_MESSAGE LAZY was already set in default cmake
  • Add WITH_CYCLES_DEBUG
  • Rename develop to developer
  • Remove WITH_CXX_GUARDEDALLOC for now

WITH_MEM_VALGRIND seems to require valgrind headers to build, so I don't
think we can enable that.

This revision is now accepted and ready to land.Jun 28 2019, 3:56 PM

Afaik, valgrind is not compatible with asan? So indeed we cannot include it here (or we'd need two different 'developer' settings :p ).

Otherwise, seems fine, though I have mixed feelings about the WITH_ASSERT_ABORT one, we still have quiet a few usages of asserts as 'soft errors', as in 'things that should not happen, but might still not be critical issues', "crashing" on those can be fairly annoying. That’s probably a bit of an orthogonal topic (I know e.g. I tend to use assert when I should rather use error logging), but for the time being, not sure that we should enable that one by default?

[short version]
You can safely ignore my wish for release mode assertions in regards to this diff.
[/Short version]

[Long Version]
I can give some background on the release mode asserts, this is not something meant for developers, when running the nightly tests there have been a few instances where the result is either unexpected (the long failing win32 cycles sss tests for instance) or an occasional random crash. but when you take a closer look and load up the problem in the debugger you almost instantly hit an assert checking for the issue you're experiencing. There's 4000+ asserts in our code, it be real nice if they alerted us to issues proactively rather than a debugging aid once someone reported an issue we already knew about but silently let slip by.

Ok, great just run the tests on debug builds problem solved!

Sadly it is not..

1: we have 10 Gb of space on the nightly build systems, I cannot do a debug build I just run out of space

All right fine, then just don't automate the testing and run a debug test locally, fair enough..

2: Debug builds are *slow* about 2 hours(or more) for a full ctest, and during those 2 hours the machine is having high resource usage, even using a browser while you wait is not great.

Start the tests, go for coffee, or watch a movie, run them at night or something!

3: By default, an assert pops up a dialog, you have to click away before things will continue. Even worse if you do get the tests on the azure environment, there is no screen or keyboard, so once the popup you cannot see pops up, it'll just sit there until the build time expires and your session gets killed.

So not only can I not use automated testing, I have to run them locally, It'll take 2 hours, My machine is unusable while they run, and I have to babysit it in case of random popups.

Soooo....yeahhh..... that's not great, there is a 0% chance anyone would do this voluntarily on a regular basis

I have a patch almost ready that'll address these issues, however, since the problems it will solve are so specific, I do not think any of it should be enabled for all developers hence we can safely ignore the whole concept of release mode asserts for this diff.
[/Long Version]

We indeed don't need to worry about release mode asserts for committing this diff. The reason I mentioned it here was mostly as a to do item. Since this includes asan, it should logically also include asserts.

WITH_ASSERT_ABORT is already ON by default, and asserts should be reserved for issues that are actual bugs. The main reason it's in this config is so that running this on an existing cmake cache restores all the options to what they should be for developers.

It also seems that WITH_PYTHON_SAFETY is breaking the freestyle render tests, and that WITH_COMPILER_ASAN finds some issues in Cycles tests. I'll look into fixing those.

We have all these useful tests and tools to catch issues early, but then end up disabling them for various reasons. That's what I'm trying to solve here.

It also seems that WITH_PYTHON_SAFETY is breaking the freestyle render tests, and that WITH_COMPILER_ASAN finds some issues in Cycles tests. I'll look into fixing those.
We have all these useful tests and tools to catch issues early, but then end up disabling them for various reasons. That's what I'm trying to solve here.

ASAN leak check on exit reports leaks which are out of our control, eg:

Direct leak of 1685265 byte(s) in 101 object(s) allocated from:
    #0 0x7f1ce79a0ada in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x7f1cdefd29db  (/usr/lib/libnvidia-glcore.so.430.26+0xe029db)

Direct leak of 1413382 byte(s) in 743 object(s) allocated from:
    #0 0x7f1ce79a0ada in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x7f1ce7067508 in PyObject_Malloc (/usr/lib/libpython3.7m.so.1.0+0xe7508)

This causes tests & generating docs to fail because of the nonzero exit code.

We could disable leak-check-at-exit cases when Blender's exit-code is used.

eg: ASAN_OPTIONS=leak_check_at_exit=0:$ASAN_OPTIONS (the trailing : is ignored in the case ASAN_OPTIONS isn't defined).

The Freestyle and WITH_PYTHON_SAFETY thing is actually separate, it also happens without asan. Freestyle lines are not rendered for some reason.

Agree it should not fail the tests on memory leaks now. Maybe later if we have suppressions or can issue warnings instead of failures, not sure. I'd use LSAN_OPTIONS="exitcode=0" so it still prints leaks but does not call exit().

build_files/windows/parse_arguments.cmd
45

How do you feel enabling nobuild here automatically by adding set NOBUILD=1 If you're enabling the developer options odds are you're going to work in the IDE and have no interest in doing a CLI build or waiting 20 minutes for one to finish and then open up the ide.

I'd use LSAN_OPTIONS="exitcode=0" so it still prints leaks but does not call exit().

LGTM, as long as it's not making failure cases succeed,

env LSAN_OPTIONS="exitcode=0" blender -b --python-expr "import sys; sys.exit(5)"
echo $?

Prints 5.

build_files/windows/parse_arguments.cmd
45

Personally I wouldn't do it, it's a bit unexpected if all the other configurations do build.

Once small change needed, beyond that lgtm.

build_files/windows/parse_arguments.cmd
45

Allright, fair enough, if you add

set TARGET=Developer

here so the build folder gets a proper name we'd be good to go.

LazyDodo (LazyDodo) requested changes to this revision.EditedAug 2 2019, 3:44 PM

actually i take that back,it's tagging on -fno-sanitize=vptr to the cflags, which msvc doesn't like, WITH_COMPILER_ASAN should only be on when we build with clang on windows.

This revision now requires changes to proceed.Aug 2 2019, 3:44 PM
build_files/windows/parse_arguments.cmd
45

This option can be combined with other targets. for example make lite developer. So I don't think it should change the target.

One annoying thing with asan builds is that Blender prints memory leaks in the console whenever you run it. I have my environment configured to suppress them.

export LSAN_OPTIONS="print_suppressions=false:suppressions=/home/brecht/tools/dev-tools/asan/lsan-suppressions.txt

lsan-suppressions.txt:

leak:_PyObject_Malloc
leak:_PyObject_Realloc
leak:PyThread_allocate_lock
leak:imb_exitopenexr
leak:imb_filetypes_exit
leak:<unknown module>
leak:libnvidia-glcore.so
leak:libX11.so
leak:libglib-2.0.so

But every developer would need to set that up too if they want those out of the way. I'm not sure if there is some way to suppress them from within Blender, or if that's something we even want to do.

Another issue is that WITH_DOC_MANPAGE fails, because it runs blender --help which returns a non-zero exit code due to leaks in OpenEXR. That one could be disabled for the developer config I think.

Disable manpage building in developer config

This is ready now as far as I'm concerned, along with D5404: Build: disable address sanitizer for Cycles kernels with GCC.

After the most recent fixes in master I can reliably build and pass all tests with RelWithDebInfo and Debug configurations.

Two main things to improve after this would be:

  • Improve build and test performance (big gtest binaries, use subset of cycles tests for most blender devs, ..?)
  • Enable asserts in RelWithDebInfo configuration
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.