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 #4442

Merged
merged 9 commits into from
Feb 29, 2024
Merged

Fixes from static analysis #4442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b5b4f82
Add a BUG_CHECK
vlstill Feb 21, 2024
ff1416a
constantFolding: Be a bit more defensive
vlstill Feb 21, 2024
819bf5a
ir-generator: Make sure we don't dereference null when exploring clas…
vlstill Feb 21, 2024
fa8970e
typeChecker: Hanlde possible nullptr
vlstill Feb 21, 2024
cc6c2df
parserUnroll: Account for SymbolicValueFactory::create returning nullptr
vlstill Feb 21, 2024
c82e440
typeChecker: Avoid dereferencing null type after getType failure
vlstill Feb 21, 2024
16b9f07
Fix comment
vlstill Feb 21, 2024
dece222
Add one more bug-check in ir-generator
vlstill Feb 27, 2024
bcf348f
Reformat irclass
vlstill Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions frontends/common/constantFolding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,7 @@ DoConstantFolding::Result DoConstantFolding::setContains(const IR::Expression *k
return Result::No;
}
auto sel = getConstant(select);
BUG_CHECK(sel, "%1%: expected a constant expression", select);
// For Enum and SerEnum instances we can just use expression equivalence.
// This assumes that type checking does not allow us to compare constants to SerEnums.
if (key->equiv(*sel)) return Result::Yes;
Expand Down
12 changes: 8 additions & 4 deletions frontends/p4/typeChecking/typeChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,7 @@ const IR::Node *TypeInference::postorder(IR::P4Action *action) {
bool foundDirectionless = false;
for (auto p : action->parameters->parameters) {
auto ptype = getType(p);
BUG_CHECK(ptype, "%1%: parameter type missing when it was found previously", p);
if (ptype->is<IR::Type_Extern>())
typeError("%1%: Action parameters cannot have extern types", p->type);
if (p->direction == IR::Direction::None)
Expand Down Expand Up @@ -3073,7 +3074,7 @@ const IR::Node *TypeInference::postorder(IR::Slice *expression) {
}

auto e1type = getType(expression->e1);
vlstill marked this conversation as resolved.
Show resolved Hide resolved
if (e1type->is<IR::Type_SerEnum>()) {
if (e1type && e1type->is<IR::Type_SerEnum>()) {
auto ei = EnumInstance::resolve(expression->e1, typeMap);
CHECK_NULL(ei);
auto sei = ei->to<SerEnumInstance>();
Expand All @@ -3084,7 +3085,7 @@ const IR::Node *TypeInference::postorder(IR::Slice *expression) {
expression->e1 = sei->value;
}
auto e2type = getType(expression->e2);
if (e2type->is<IR::Type_SerEnum>()) {
if (e2type && e2type->is<IR::Type_SerEnum>()) {
auto ei = EnumInstance::resolve(expression->e2, typeMap);
CHECK_NULL(ei);
auto sei = ei->to<SerEnumInstance>();
Expand Down Expand Up @@ -3325,8 +3326,10 @@ const IR::Node *TypeInference::postorder(IR::Member *expression) {
}

if (auto *apply = type->to<IR::IApply>(); apply && member == IR::IApply::applyMethodName) {
auto methodType = apply->getApplyMethodType();
methodType = canonicalize(methodType)->to<IR::Type_Method>();
auto *methodType = apply->getApplyMethodType();
auto *canon = canonicalize(methodType);
if (!canon) return expression;
methodType = canon->to<IR::Type_Method>();
if (methodType == nullptr) return expression;
learn(methodType, this);
setType(getOriginal(), methodType);
Expand Down Expand Up @@ -3427,6 +3430,7 @@ const IR::Expression *TypeInference::actionCall(bool inActionList,
}
auto method = actionCall->method;
auto methodType = getType(method);
if (!methodType) return actionCall; // error emitted in getType
auto baseType = methodType->to<IR::Type_Action>();
if (!baseType) {
typeError("%1%: must be an action", method);
Expand Down
2 changes: 1 addition & 1 deletion midend/parserUnroll.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ class ParserSymbolicInterpreter {
}

if (value == nullptr) value = factory->create(type, true);
if (value->is<SymbolicError>()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably throw an error if the value returned by factory->create is still a nullptr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

if (value && value->is<SymbolicError>()) {
::warning(ErrorType::ERR_EXPRESSION, "%1%: %2%", d,
value->to<SymbolicError>()->message());
return nullptr;
Expand Down
8 changes: 6 additions & 2 deletions tools/ir-generator/irclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 << " ";
auto *p = this;
for (; p && p != nodeClass(); p = p->getParent()) {
out << " ";
}
BUG_CHECK(p != nullptr, "Falled out of the class hierarchy");
out << "M(";
const char *sep = "";
for (auto p = this; p; p = p->getParent()) {
for (p = this; p; p = p->getParent()) {
out << sep << p->containedIn << p->name;
sep = *sep ? ") B(" : ", D(";
}
Expand Down
Loading