Fix some issues found by static analysis #4213
Conversation
Coverity, so no. My feeling is a lot of this could actually be found by clang-tidy too though, but I had not checked that yet. |
vlstill
changed the title
| @@ -66,12 +66,14 @@ TestBackEnd::TestInfo EBPFTestBackend::produceTestInfo( | |||
| IR::getConstant(IR::getBitType(outPktSize), EBPFTestBackend::ZERO_PKT_VAL); | |||
| testInfo.packetTaintMask = | |||
| IR::getConstant(IR::getBitType(outPktSize), EBPFTestBackend::ZERO_PKT_MAX); | |||
| } else { | |||
There was a problem hiding this comment.
@fruffy I'm not really sure about this change. The original code seems suspicious, since the outputPacket set in the "short" path was always rewritten. But I don't know if it is correct now.
There was a problem hiding this comment.
This seems good. I am not even sure whether the condition can ever be true. The eBPF back end can not reduce packet size.
| @@ -66,12 +66,14 @@ TestBackEnd::TestInfo EBPFTestBackend::produceTestInfo( | |||
| IR::getConstant(IR::getBitType(outPktSize), EBPFTestBackend::ZERO_PKT_VAL); | |||
| testInfo.packetTaintMask = | |||
| IR::getConstant(IR::getBitType(outPktSize), EBPFTestBackend::ZERO_PKT_MAX); | |||
| } else { | |||
There was a problem hiding this comment.
This seems good. I am not even sure whether the condition can ever be true. The eBPF back end can not reduce packet size.
ir/v1.def
Outdated
| @@ -230,7 +230,7 @@ class CalculatedField : IAnnotated { | |||
| bool update; | |||
| ID name; | |||
| Expression cond; | |||
| update_or_verify() {} // FIXME -- needed by umpack_json(safe_vector) -- should not be | |||
| update_or_verify() { update = false; } // FIXME -- needed by umpack_json(safe_vector) -- should not be | |||
There was a problem hiding this comment.
Should this be in the function call or in the initializer of update?
There was a problem hiding this comment.
I would like to put it into the initializer, but for some reason that breaks the generated code -- the parametric constructor has just 3 arguments. I don't know why that is, it seems quite weird to me. Therefore I've added it to the constructor. I'm quite surprised nested classes can be generated by IRGen.
There was a problem hiding this comment.
optional bool update = false;
maybe?
There was a problem hiding this comment.
That worked, thanks.
|
@fruffy is this test working normally? |
It is failing about 30% of the time. We are unsure what causes this failure but are trying to fix it. It might be caused by a particular Github runner being chosen. |
…ntext found by clang-tidy
No description provided.