mosra / corrade

C++11 multiplatform utility library
https://magnum.graphics/corrade/
Other
482 stars 105 forks source link

Add clang-static analyzer to linux CI builds #122

Open jonesmz opened 3 years ago

jonesmz commented 3 years ago

This is trivial to add to a project, just add "scan-build" before any call to cmake. E.g.

scan-build --exclude someFolderToExclude -o folderToOutputLogs cmake -B build -DCMAKE_BUILD_TYPE=Release
scan-build --exclude someFolderToExclude -o folderToOutputLogs cmake --build build --parallel --config Release --target myTarget

I see nags from scan-build coming from Corrade in several places in my own project. Note that I'm not claiming that the nags from scan-build are 100% correct. Just that they're nagging.

In file included from /home/jonesmz/osp-magnum/3rdparty/magnum/src/Magnum/GL/Implementation/driverSpecific.cpp:27:
In file included from /home/jonesmz/osp-magnum/3rdparty/corrade/src/Corrade/Containers/GrowableArray.h:39:
In file included from /home/jonesmz/osp-magnum/3rdparty/corrade/src/Corrade/Containers/Array.h:40:
/home/jonesmz/osp-magnum/3rdparty/corrade/src/Corrade/Containers/constructHelpers.h:67:5: warning: Storage provided to placement new is only -2305843009213693936 bytes, whereas the allocated type requires 16 bytes [cplusplus.PlacementNew]
    new(&value) T{std::forward<First>(first), std::forward<Next>(next)...};
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jonesmz/osp-magnum/3rdparty/corrade/src/Corrade/Containers/constructHelpers.h:67:5: warning: Storage provided to placement new is only 0 bytes, whereas the allocated type requires 24 bytes [cplusplus.PlacementNew]
    new(&value) T{std::forward<First>(first), std::forward<Next>(next)...};
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/jonesmz/osp-magnum/3rdparty/magnum/src/Magnum/GL/Implementation/driverSpecific.cpp:27:
/home/jonesmz/osp-magnum/3rdparty/corrade/src/Corrade/Containers/GrowableArray.h:923:9: warning: Storage provided to placement new is only 0 bytes, whereas the allocated type requires 24 bytes [cplusplus.PlacementNew]
        new(dst) T{std::move(*src)};
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jonesmz/osp-magnum/3rdparty/corrade/src/Corrade/Containers/GrowableArray.h:1024:65: warning: Method called on moved-from object of type 'std::pair' [cplusplus.Move]
    for(T *it = array, *end = array + prevSize; it < end; ++it) it->~T();
                                                                ^~~~~~~~
4 warnings generated.
mosra commented 2 years ago

I used to run Clang Analyzer on the codebase in the past, but (compared to ASan & TSan) it didn't catch much for me, so enabling it was not a priority when I was balancing the tradeoffs for CI build time. Same is for Clang Tidy, it is able to catch certain issues, but rarely something truly important, so I'm relying on 3rd party projects reporting such issues to me.

Storage provided to placement new is only -2305843009213693936 bytes

Hmm, but looking at the output snippet, the cplusplus.PlacementNew and cplusplus.Move checks certainly need some work :smile: None of the sizes make sense, calling a destructor on a moved-from object is also not a wrong thing to do in low-level container implementations.

Leaving this open (together with #29 for UBSan) as a reminder to look into this again in the future.

jonesmz commented 2 years ago

I totally get where you are coming from. The clang analyzer does have false positives sometimes.

In my opinion, it would be good enough to just add the annotation comments to the code that will make clang analyzer shut up.

The only reason I opened this issue is because its kind of difficult to convince clang analyzer to ignore 3rd party headers when analyzing a codebase, so whether the warnings get suppressed or the code gets changed, it doesn't matter a whole lot to me :)

jonesmz commented 2 years ago

I did manage to cut most 3rd party code out of my analysts by only generating a compiler-commands.json file for my first-party source files.

Here's how I did it: https://github.com/TheOpenSpaceProgram/osp-magnum/blob/master/.github/workflows/analyzers.yml