Switch From Clang To GCC

[garethellis0]

Please fill out the following before requesting review on this PR

Description

This is basically #1177 with some extra changes to fix test segfaults, build issues, and code coverage.

Testing Done

Builds and passes in CI, coverage is properly generated.

Resolved Issues

resolves #1124

Length Justification

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

• Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
• Remove all commented out code
• Remove extra print statements: for example, those just used for testing
• Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue
• Justify drops in code coverage: If this PR results in a non-trivial drop in code coverage (a bot should post a coverage diagram as a comment), please justify why we can't test the code that's not covered.

[MathewMacDougall]

@jonathanlew segfaults were due to variables in some of my physics tests being destroyed in a different order due to change in compilers. In some low-level tests certain variables must always be destroyed first due to how Box2D manages its pointers.

[garethellis0]

Looks a lot cleaner. I'm not sure how you fixed the segfaults but I'm glad you did. Can you also comment on why we need a build switch?

Sorry, not sure what a "build switch" is?

[MathewMacDougall]

another question for my sanity

[MathewMacDougall]

just want to double check. Bazel's CLI-Reference says the coverage command "Generates code coverage report for specified test targets". It seems it's roughly the same as test but with coverage. Does this mean we're not building all the code, ie. missing the top-level code not associated with tests?

I know we need to build code with covarege enabled to get the coverage data (so doesn't make sense to necessarily call build here).

[garethellis0]

Correct, I just thought of this while I was cleaning this up..........

We could run bazel build //software:full_system_main as well, but the different command line options would probably trigger a re-compile of basically everything.

[MathewMacDougall]

Maybe worth trying locally. Or perhaps we can somehow pass the coverage flags to build first? My understanding is that the main difference is the extra symbols / tracking built in for coverage to consume

[garethellis0]

Just made some changes to full_system_main.cpp and it failed. No test depends on that target, so looks like bazel builds everything. So this is actually a non-issue! 🎉

[jonathanlew]

Looks a lot cleaner. I'm not sure how you fixed the segfaults but I'm glad you did. Can you also comment on why we need a build switch?

Sorry, not sure what a "build switch" is?

Commented in line

[akhilveeraghanta]

LGTM

[jonathanlew]

LGTM pending CI 👍

[MathewMacDougall]

Ok so after some investigation it turns out you can't pass matrices as float** in c++. This is the cause of the CI failure.

Eg.

void matrix_test(int rows, int cols, float** matrix){
    for (int i = 0; i < rows; i++){
        for (int j = 0; j < cols; j++){
            volatile float a = matrix[i][j];
        }
    }
}
int main(int argc, char ** argv) {
    int num_rows = 10;
    int num_cols = 10;
    float matrix[num_rows][num_cols];
    for (int i = 0; i < num_rows; i++) {
        for (int j = 0; j < num_cols; j++){
            matrix[i][j] = i*j;
        }
    }
    matrix_test(num_rows, num_cols, matrix);
}

This is valid C code, but invalid C code. (although C does throw a warning).

The easiest solution was to move the offending matrix / linear algebra code to a new file and build target, so it can be purely built as C and use Variable Length Arrays. We should consider proper "Shareable" matrix and vector types moving forwards, but that's out of scope for this PR.

The commits I pushed move the matrix functions to a new file, and also removed any unused functions.

[MathewMacDougall]

[jonathanlew]

👍