Fixes from static analysis #4442
Conversation
vlstill
commented
Feb 21, 2024
•
vlstill
commented
- Add a BUG_CHECK
- constantFolding: Be a bit more defensive
- ir-generator: Make sure we don't dereference null when exploring class hierarchy
- Permit AutoCompileContext to throw in destructor if context is inconsistent
- typeChecker: Hanlde possible nullptr
- parserUnroll: Account for SymbolicValueFactory::create returning nullptr
- typeChecker: Avoid dereferencing null type after getType failure
vlstill
force-pushed
the
coverity-fixes-2024-02
branch
3 times, most recently
from
ea598f1
to
e0191c0
Compare
fruffy
added
the
core
vlstill
force-pushed
the
coverity-fixes-2024-02
branch
from
e0191c0
to
de6a344
Compare
vlstill
force-pushed
the
coverity-fixes-2024-02
branch
from
de6a344
to
16b9f07
Compare
| @@ -391,7 +391,7 @@ class ParserSymbolicInterpreter { | |||
| } | |||
|
|
|||
| if (value == nullptr) value = factory->create(type, true); | |||
| if (value->is<SymbolicError>()) { | |||
There was a problem hiding this comment.
Should probably throw an error if the value returned by factory->create is still a nullptr.
There was a problem hiding this comment.
Well the code below still checks if it is nullptr only only then sets it. That said, if it was nullptr it would crash until now, so presumably it never is :-). It seems to me based on reading the code that the factory could return nullptr if this pass was applied to a parser with non-inlined subparser. So that would suggest the pass probably should not be called.
Sadly, this is a complex code that is not well documented. If there was a documented prerequisite that the parsers has to be inlined I'd be happy adding an assert there. But it seems to that at least for the factory, someone was thinking about the case where they are not. I believe this modification is safe, adding the error would be a little bit more risky.
There was a problem hiding this comment.
Makes sense, the parser unroll pass unfortunately does not have good error checking. I think this error should be handled correctly but this is not the point of this PR. The changes here just make this erroneous behavior explicit.
tools/ir-generator/irclass.cpp
Outdated
| @@ -251,7 +251,7 @@ void IrDefinitions::generate(std::ostream &t, std::ostream &out, std::ostream &i | |||
| } | |||
|
|
|||
| void IrClass::generateTreeMacro(std::ostream &out) const { | |||
| for (auto p = this; p != nodeClass(); p = p->getParent()) out << " "; | |||
| for (auto p = this; p && p != nodeClass(); p = p->getParent()) out << " "; | |||
There was a problem hiding this comment.
This is a bit odd, I am guessing it doesn't like the nullptr comparison.
There was a problem hiding this comment.
I don't follow. nodeClass is a function that defines a representation of IR::Node class. So this code's correctness counts on IR::Node being root of the hierarchy. Since this is an implicit root, it would probably be a bug if it was not there. I'll add a bug check after the loop that IR::Node was found.
p4c/tools/ir-generator/irclass.cpp
Lines 36 to 39 in 16b9f07
There was a problem hiding this comment.
Comment was truncated. I meant to say this piece of code is a little funky in that it assumes that when p is nullptr it can terminate. The static analysis is right to add this check here but the code itself seems prone to errors.
There was a problem hiding this comment.
Yep, you are right in that, if it comes to nullptr it is a bug (elsewhere), which will now be reported.
tools/ir-generator/irclass.cpp
Outdated
| @@ -251,10 +251,14 @@ void IrDefinitions::generate(std::ostream &t, std::ostream &out, std::ostream &i | |||
| } | |||
|
|
|||
| void IrClass::generateTreeMacro(std::ostream &out) const { | |||
| for (auto p = this; p != nodeClass(); p = p->getParent()) out << " "; | |||
| { | |||
There was a problem hiding this comment.
Any particular reason for the bracketing? You could just set p in the second loop again, no? Other than that approving early.
There was a problem hiding this comment.
Also would add brackets to the for loop, I completely missed the out << " "; on the first read.
There was a problem hiding this comment.
No particular reason, just limiting scope of the p. But I guess it looks too weird, changed.
There was a problem hiding this comment.
Also would add brackets to the for loop, I completely missed the
out << " "; on the first read.
Yes, this is one of my least favorite features of the code format we use :-(. Personally I tend to use brackets around single line blocks in p4c because of it, even though I never used them in other codebases.
But in general I tend to not change formatting of existing code that I just bug-fix.
|
Little tangential, but: @fruffy, I am quite confused from how the merge queues work. I thought it was this PR in which the macOS test failed (ac6d9d5 / https://github.com/p4lang/p4c/actions/runs/8100632186) but apparently it got still merged, without that test ever running. And your #4487 does not seem like it is using the queue at all. The merge queue surely does look very interesting, and I hope we can continue using it, but this is a bit weird. |
|
Actually, this is working as intended. The merge queue will only block on required tests. We can make more tests required, but we have kept stuff like P4Testgen and ptf-linux optional. |
|
The M1 test is not required yet, for example. |
|
Interesting, it seems the merge queue is just running the required checks. That kind of makes sense, although I am not super keen of it hiding the rest. Do you know why some of the PRs are just in the "auto-merge" state, not in a queue? |
