jamoma / JamomaCore

Jamoma Frameworks for Audio and Control Structure
Other
36 stars 14 forks source link

Add CMake options to enable sanitization checkers #392

Closed jcelerier closed 2 years ago

jcelerier commented 8 years ago

This is not ready to be merged, I have to test on Windows and OS X before. It's relative to the exchanged mails on the jamoma list.

For instance running ./test_JamomaFoundation with a Jamoma built with :

cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DJAMOMA_LIBCXX_DEBUG:Bool=True -DJAMOMA_SANITIZE_UNDEFINED:Bool=True ../JamomaCore

yields :

/tmp/JamomaCore/Foundation/library/includes/TTAddress.h:101:52: runtime error: downcast of address 0x0000015d2150 which does not point to an object of type 'TTAddressBase'
0x0000015d2150: note: object is of type 'TTSymbolBase'
 00 00 00 00  d0 ef 3d 81 d2 7f 00 00  b0 7a 61 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'TTSymbolBase'
JamomaFoundation -- Version  - faaf91f
computedRelativePath(): /usr/local/jamoma/lib/libJamomaFoundation.so.6
Warning: no classes were loaded.nodelib.test

        Testing Address Parsing
/usr/include/c++/5.3.0/debug/safe_iterator.h:182:error: attempt to copy-
    construct an iterator from a singular iterator.

Objects involved in the operation:
iterator "this" @ 0x0x15ef730 {
type = N11__gnu_debug14_Safe_iteratorIN9__gnu_cxx17__normal_iteratorIPcNSt9__cxx19986vectorIcSaIcEEEEENSt7__debug6vectorIcS6_EEEE (mutable iterator);
  state = singular;
}
iterator "other" @ 0x0x15ee780 {
type = N11__gnu_debug14_Safe_iteratorIN9__gnu_cxx17__normal_iteratorIPcNSt9__cxx19986vectorIcSaIcEEEEENSt7__debug6vectorIcS6_EEEE (mutable iterator);
  state = singular;
  references sequence with type `NSt7__debug6vectorIcSaIcEEE' @ 0x0x7ffdbc2c01d0
}

note: there are some false positives, especially with dynamic libs.

jcelerier commented 8 years ago

http://stackoverflow.com/questions/20678801/clang-mac-os-x-maveric-not-supporting-fsanitize-undefined/21657956#21657956 :(

I only enable it for Linux then.

jcelerier commented 8 years ago

I fixed some memory leaks with the latest patches. However I didn't take care and my IDE changed all the tabs to space :( Here are the relevant changes : https://github.com/jamoma/JamomaCore/pull/392/files?w=1

tap commented 8 years ago

Is this one ready for review/merge @jcelerier ? If so I guess it should be assigned to @theod because of changes in the NodeLib...

Glad you caught the mem leaks!

I see that the TTEnvironment was upgraded to a std::unique_ptr. I guess this is so that it is deleted on exit because it was showing up in your mem leak analysis? Seems like it could ideally be updated to something like a Meyer singleton, but maybe that's too much work for too little benefit now?

jcelerier commented 8 years ago

I just have to check that it didn't break anything WRT Max and Windows and it'll be good for a review.

A Meyer singleton is definitely in the realm of the possible, it will just take a few more days but it's overall better. There are still a few things showing up in valgrind too (in NodeLib::init iirc) which I want to fix.

Happy new year :)

jcelerier commented 8 years ago

I started working on it. However I encounter a small problem :

given

TTEnvironment& ttEnvironment() 
{
    static TTEnvironment env;
    return env;
}

in TTObjectBase there are some methods that end up calling ttEnvironment(). For now it looks like this does not happen during the construction of the TTEnvironment's TTObjectBase parent, but if someone was to call registerMessage(...) or logDebugBasic(...) in the ctor of TTObjectBase it would crash with a recursive initialization fault.

There is the same problem in TTEnvironment's constructor which calls addMessageWithArguments(...). However this can be solved by adding an init() method that would be called at the place where the TTEnvironment* is currently constructed, in TTFoundationInit.

tap commented 8 years ago

Thanks for investigating @jcelerier.

Since we create an instance of TTEnvironment toward the beginning of TTFoundationInit() already, that should mean that aren't introducing any new possibilities for this recursion to take place. For that reason I think it is safe to do.

However, the issue you point out is certainly a design flaw and it seems we should maybe document or ticket that? What do you think?

tap commented 2 years ago

This pull request has gone stale.