Could we help static analyzers know a single SECTION is executed?

[nidoizo]

We're running Coverity to analyze our code base, and if we do std::move of the same thing in multiple SECTION, Coverity will complain that if, for example, both catch_internal_Section258->operator bool() and catch_internal_Section259->operator bool() are true, then we end touching which was moved. I understand it's much simpler to generate a if for each SECTION, but if it would be great if we could find a way to tell the static analyzer that a single section is active at the same time. For example, maybe all the booleans are in a bitfield and we assert that a single bit is set. Generating else if/switch-case would work, but I cannot think of a way to make it work with just SECTION. We discussed the idea of adding DECLARE_SECTIONS(name1, name2, ...) to add an assert to help the static analyzer. I'm creating this issue without any clear solution in mind, but more to brainstorm if we have any idea for a solution.

[horenmar]

clang-tidy has the same issue.

I am not aware of any reasonable way of solving this for clang-tidy either.

[nidoizo]

Well, clang-tidy is analyzing functions and translation units individually, but Coverity is doing a global analysis, so it would be possible to add an assert at a higher level that a single section is active per iteration.

[vberlier]

It's kind of a hack but for clang-tidy we can check if __clang_analyzer__ is defined and override the macros to turn sections into if statements that are clearly mutually exclusive to the analyzer.

#include <string>
#include <catch2/catch_test_macros.hpp>

#ifdef __clang_analyzer__
#undef TEST_CASE
#define TEST_CASE(...) void INTERNAL_CATCH_UNIQUE_NAME(TestCase)([[maybe_unused]] int sectionHint) 
#undef SECTION
#define SECTION(...) if (sectionHint == __LINE__)
#endif

TEST_CASE("Example")
{
    std::string foo;
    
    SECTION("A")
    {
        std::string a = std::move(foo);
    }

    SECTION("B")
    {
        std::string b = std::move(foo);
    }
}

[vberlier]

Tested in compiler explorer https://godbolt.org/z/3eTxTvch7

I also made a few changes to make it work with nested sections.

[nidoizo]

Wow, great idea, didn't think of using __LINE__ to make a "if" exclusive. I'll test that, it sounds really simple for static analyzer to figure out. That would prevent nested sections however, as a nested section cannot be reached.

[vberlier]

Yeah the godbolt link shows how to make it work with nested sections. The trick is to shadow the sectionHint from the parent scope with a fresh value:

int GetNewSectionHint();

#define SECTION(...) if ([[maybe_unused]] int tmpHint = sectionHint, sectionHint = GetNewSectionHint(); tmpHint == __LINE__)

[nidoizo]

This is fixing the issue for me, by I guess now the question is if this is built-in for Coverity, clang-tidy, etc. so that people don't search for a solution again.

[horenmar]

@vberlier I like the idea, and the change should not be too annoying to maintain. One question though: does clang-tidy promise to always understand C++17 attributes? Catch2 still targets C++14 baseline which does not include [[maybe_unused]]

[nidoizo]

@horenmar clang-tidy will understand anything the Clang it is based on understands, so the question is more if people are using C++14-based clang-tidy. I tend to think for tools like clang-tidy and Coverity you use something recent to get the best value out of it, but I cannot speak for others. If you prefer, you can just provide a define we use for static analyzers. If you want to support some static analyzers built-in, you can use __COVERITY__ for Coverity BTW.

[vberlier]

For [[maybe_unused]] clang has the non-standard __attribute__((unused)) that does the same thing in C++14 and below. However I can't think of an immediate alternative to the if initialization statement for nested sections.

Personally I think it would be fine to implement this as a progressive enhancement. Catch would simply work a bit better with static analyzers when C++17 or higher is detected.

#if __cplusplus >= 201703L && (defined(__clang_analyzer__) || defined(__COVERITY__))

[horenmar]

Okay, so I implemented this and ran into an issue:

I used this as my test case

TEST_CASE( "Example" ) {
    std::string foo = "hello";

    SECTION( "A" ) {
        std::string a = std::move(foo);
        CHECK( a == "hello" );
    }

    SECTION( "Group" ) {
        SECTION( "B" ) {
            std::string b = std::move( foo );
            CHECK( b == "hello" );
        }
        SECTION( "C" ) {
            std::string c = std::move( foo );
            CHECK(c == "hello");

            //// should only emit warning here
            //std::string d = std::move( foo );
            //CHECK( d == "hello" );
        }
    }
}

When I run clang-tidy-17 through CMake, with this

CXX=clang++-17 cmake -B debug-v3-clang-tidy -S /mnt/c/ubuntu/Catch2/ --preset basic-tests -DCMAKE_CXX_CLANG_TIDY='clang-tidy;-checks=bugprone-use-after-move' -G Ninja

I still get warnings about use-after-move

/mnt/c/ubuntu/Catch2/tests/SelfTest/UsageTests/Misc.tests.cpp:565:40: warning: 'foo' used after it was moved [bugprone-use-after-move]
            std::string b = std::move( foo );
                                       ^
/mnt/c/ubuntu/Catch2/tests/SelfTest/UsageTests/Misc.tests.cpp:559:25: note: move occurred here
        std::string a = std::move(foo);
                        ^
/mnt/c/ubuntu/Catch2/tests/SelfTest/UsageTests/Misc.tests.cpp:569:40: warning: 'foo' used after it was moved [bugprone-use-after-move]
            std::string c = std::move( foo );
                                       ^
/mnt/c/ubuntu/Catch2/tests/SelfTest/UsageTests/Misc.tests.cpp:565:29: note: move occurred here
            std::string b = std::move( foo );
                            ^

It's all in the branch devel-better-static-analysis-support-in-sections if anyone wants to play with this.

[vberlier]

The bugprone-* checks aren't enabled by default. The one that we care about is clang-analyzer-cplusplus.Move. It looks like the bugprone-use-after-move check isn't as smart as the clang-analyzer-cplusplus.Move check. The documentation says it can occasionally fail to recognize mutually exclusive branches https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html

I tested it and with -checks="clang-analyzer-cplusplus.Move" and I only get a warning when uncommenting the last assignment.

[horenmar]

Whyyyy are there differently named checks for using moved-from objects with different quality of checking?

[vberlier]

Honestly I'm not completely sure, it's kind of confusing. Most projects run clang-tidy with the default config though, so I think it's fine to ignore the bugprone-use-after-move check since it's not enabled by default.

By the way I'm thinking that the sectionHint should probably be const, and maybe also prefixed with catch to avoid collisions with user-defined variables.