Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clang-static analyzer to linux CI builds #122

Open
jonesmz opened this issue Aug 23, 2021 · 3 comments
Open

Add clang-static analyzer to linux CI builds #122

jonesmz opened this issue Aug 23, 2021 · 3 comments

Comments

@jonesmz
Copy link

jonesmz commented Aug 23, 2021

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
Copy link
Owner

mosra commented Sep 12, 2021

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 😄 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
Copy link
Author

jonesmz commented Sep 13, 2021

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
Copy link
Author

jonesmz commented Sep 13, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants