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.

Sign up for GitHub

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

Jump to bottom

Fixes from static analysis #4391

Merged
merged 12 commits into from
Feb 6, 2024
Merged

Fixes from static analysis #4391

merged 12 commits into from
Feb 6, 2024

Conversation

vlstill
Copy link
Contributor

@vlstill vlstill commented Feb 5, 2024 •
edited
Loading

Fixes for issues found by coverity.

@vlstill vlstill added the run-sanitizer Use this tag to run a Clang+Sanitzers CI run. label Feb 5, 2024
@vlstill vlstill changed the title Fixes from static analysis [WIP] Fixes from static analysis Feb 5, 2024
@vlstill vlstill marked this pull request as ready for review February 5, 2024 13:21
vlstill
vlstill commented Feb 5, 2024
View reviewed changes
backends/p4tools/modules/testgen/targets/bmv2/p4_asserts_parser.cpp Outdated
@@ -530,7 +530,7 @@ const IR::Node *AssertsParser::postorder(IR::P4Table *node) {
auto *cloneProperties = node->properties->clone();
IR::IndexedVector<IR::Property> properties;
for (const auto *property : cloneProperties->properties) {
if (property->name.name != "key" || property->name.name != "entries") {
if (property->name.name != "key" && property->name.name != "entries") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fruffy, note that the original condition was a tautology. I've changed it to match the comment above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. Hope I can get rid of this pass soon.

vlstill
vlstill commented Feb 5, 2024
View reviewed changes
midend/removeComplexExpressions.cpp
@@ -104,7 +104,7 @@ const IR::Vector<IR::Expression> *RemoveComplexExpressions::simplifyExpressions(
// of the list. Otherwise we simplify the argument itself.
// This is mostly for functions that take FieldLists - these
// should still take a list as argument.
bool changes = true;
bool changes = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the for cycle below.

@vlstill
Copy link
Contributor Author

vlstill commented Feb 5, 2024

It might make more sense to go over the PR commit-by-commit as each commit is a separate problem fix.

fruffy
fruffy reviewed Feb 5, 2024
View reviewed changes
lib/options.cpp Outdated
@@ -15,7 +15,8 @@ limitations under the License.
*/

#include "options.h"
#include <lib/null.h>

#include "null.h"
Copy link
Collaborator

@fruffy fruffy Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"lib/null.h" should work. When something like #include <lib/null.h> happens it usually means some tool (clang-tidy or IWYU) adds system includes instead of local includes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was mainly surprised this failed in bazel, but not everywhere else. This seems like some sort of inconsistency in the build system, but since I don't know bazel I didn't want to investigate it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the quotes in my comment got swallowed.

The lack of support for angled brackets is by design: https://bazel.build/docs/bazel-and-cpp#include-paths

fruffy
fruffy reviewed Feb 5, 2024
View reviewed changes
frontends/p4/alias.h
@@ -123,6 +123,8 @@ class ReadsWrites : public Inspector {
void postorder(const IR::Operation_Binary *expression) override {
auto left = ::get(rw, expression->left);
auto right = ::get(rw, expression->right);
CHECK_NULL(left);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use this a lot. Maybe we should implement some form of checked ::get?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe... or just use .at() it does produce a standard exception instead of p4c one, but that is not a big problem in my opinion. Maybe we should have some sort of guidelines. I'd guess many new programmers don't even know there is ::get until they find some in the code.

@vlstill
Copy link
Contributor Author

vlstill commented Feb 6, 2024

@fruffy, I've fixed the include. The ::get seems to me like something that should be done separately (or I could use at in this file now, instead of the added CHECK_NULL, but I don't want to go over all the ::get uses now as that seems like quite a lot of work).

fruffy
fruffy approved these changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a form of these static analysis checks to the open-source infrastructure. We do have the Github marketplace available but never used it.

@vlstill vlstill enabled auto-merge (squash) February 6, 2024 16:21
@vlstill
Copy link
Contributor Author

vlstill commented Feb 6, 2024

Maybe we can add a form of these static analysis checks to the open-source infrastructure. We do have the Github marketplace available but never used it.

Yeah, that would be great if there is a good free analyzer. It seems that clang-tidy is quite a bit more limited then coverity, and it is pretty slow as you have pointed out. Maybe there would be a way to run in in scheduled runs (nightly, weekly), or in some sort of differential mode though. I don't know about other options sadly.

@fruffy
Copy link
Collaborator

fruffy commented Feb 6, 2024

Maybe we can add a form of these static analysis checks to the open-source infrastructure. We do have the Github marketplace available but never used it.

Yeah, that would be great if there is a good free analyzer. It seems that clang-tidy is quite a bit more limited then coverity, and it is pretty slow as you have pointed out. Maybe there would be a way to run in in scheduled runs (nightly, weekly), or in some sort of differential mode though. I don't know about other options sadly.

I will take a look.

P4Testgen will fail because of a failing test that slipped past CI. #4397 should fix it, feel free to merge that one, patch the fix, or just ignore the failure.

@vlstill vlstill merged commit 12dafee into p4lang:main Feb 6, 2024
15 of 16 checks passed
@vlstill
Copy link
Contributor Author

vlstill commented Feb 7, 2024

It looks like the auto merger is willing to auto-merge with failed optional tests... Is this deliberate?

@fruffy
Copy link
Collaborator

fruffy commented Feb 7, 2024

It looks like the auto merger is willing to auto-merge with failed optional tests... Is this deliberate?

Well, I also did not expect that. But it makes sense, there might be optional tests that fail frequently and you do not want to make the auto-merge useless because of that. The tools tests are still optional.

@vlstill vlstill deleted the static-analysis-fixes branch February 21, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fruffy fruffy fruffy approved these changes

@mihaibudiu mihaibudiu Awaiting requested review from mihaibudiu

Assignees
No one assigned
Labels
run-sanitizer Use this tag to run a Clang+Sanitzers CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vlstill @fruffy