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.

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

Fixes from static analysis #4391

Merged
merged 12 commits into from
Feb 6, 2024
2 changes: 1 addition & 1 deletion frontends/common/constantParsing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ static IR::Constant *parseConstantWithWidth(Util::SourceInfo srcInfo, const char
char *sep;
auto size = strtol(text, &sep, 10);
sep += strspn(sep, " \t\r\n");
if (sep == nullptr || !*sep) BUG("Expected to find separator %1%", text);
if (!*sep) BUG("Expected to find separator %1%", text);
if (size < 0) {
::error(ErrorType::ERR_INVALID, "%1%: invalid width; %2% must be positive", srcInfo, size);
return nullptr;
Expand Down
6 changes: 5 additions & 1 deletion frontends/common/parseInput.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ const IR::P4Program *parseP4File(ParserOptions &options) {
auto result = options.isv1()
? parseV1Program<FILE *, C>(in, options.file, 1, options.getDebugHook())
: P4ParserDriver::parse(in, options.file);
options.closeInput(in);
if (options.doNotPreprocess) {
fclose(in);
} else {
options.closePreprocessedInput(in);
}

if (::errorCount() > 0) {
::error(ErrorType::ERR_OVERLIMIT, "%1% errors encountered, aborting compilation",
Expand Down
4 changes: 2 additions & 2 deletions frontends/common/parser_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -425,13 +425,13 @@ FILE *ParserOptions::preprocess() {
ssize_t read;

while ((read = getline(&line, &len, in)) != -1) printf("%s", line);
closeInput(in);
closePreprocessedInput(in);
return nullptr;
}
return in;
}

void ParserOptions::closeInput(FILE *inputStream) const {
void ParserOptions::closePreprocessedInput(FILE *inputStream) const {
if (close_input) {
int exitCode = pclose(inputStream);
if (WIFEXITED(exitCode) && WEXITSTATUS(exitCode) == 4)
Expand Down
2 changes: 1 addition & 1 deletion frontends/common/parser_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class ParserOptions : public Util::Options {
// Returns the output of the preprocessor.
FILE *preprocess();
// Closes the input stream returned by preprocess.
void closeInput(FILE *input) const;
void closePreprocessedInput(FILE *input) const;
// True if we are compiling a P4 v1.0 or v1.1 program
bool isv1() const;
// Get a debug hook function suitable for insertion
Expand Down
14 changes: 14 additions & 0 deletions frontends/p4/alias.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.

CHECK_NULL(right);
rw.emplace(expression, left->join(right));
}

Expand All @@ -135,11 +137,13 @@ class ReadsWrites : public Inspector {

void postorder(const IR::Operation_Unary *expression) override {
auto e = ::get(rw, expression->expr);
CHECK_NULL(e);
rw.emplace(expression, e);
}

void postorder(const IR::Member *expression) override {
auto e = ::get(rw, expression->expr);
CHECK_NULL(e);
auto result = e->append(expression->member);
rw.emplace(expression, result);
}
Expand Down Expand Up @@ -182,18 +186,23 @@ class ReadsWrites : public Inspector {
auto e0 = ::get(rw, expression->e0);
auto e1 = ::get(rw, expression->e1);
auto e2 = ::get(rw, expression->e2);
CHECK_NULL(e0);
CHECK_NULL(e1);
CHECK_NULL(e2);
rw.emplace(expression, e0->join(e1)->join(e2));
}

void postorder(const IR::Slice *expression) override {
auto e = ::get(rw, expression->e0);
CHECK_NULL(e);
rw.emplace(expression, e);
}

void postorder(const IR::MethodCallExpression *expression) override {
auto e = ::get(rw, expression->method);
for (auto a : *expression->arguments) {
auto s = ::get(rw, a->expression);
CHECK_NULL(s);
e = e->join(s);
}
rw.emplace(expression, e);
Expand All @@ -203,6 +212,7 @@ class ReadsWrites : public Inspector {
const SetOfLocations *result = new SetOfLocations();
for (auto e : *expression->arguments) {
auto s = ::get(rw, e->expression);
CHECK_NULL(s);
result = result->join(s);
}
rw.emplace(expression, result);
Expand All @@ -212,6 +222,7 @@ class ReadsWrites : public Inspector {
const SetOfLocations *result = new SetOfLocations();
for (auto e : expression->components) {
auto s = ::get(rw, e->expression);
CHECK_NULL(s);
result = result->join(s);
}
rw.emplace(expression, result);
Expand All @@ -221,6 +232,7 @@ class ReadsWrites : public Inspector {
const SetOfLocations *result = new SetOfLocations();
for (auto e : expression->components) {
auto s = ::get(rw, e);
CHECK_NULL(s);
result = result->join(s);
}
rw.emplace(expression, result);
Expand All @@ -237,6 +249,8 @@ class ReadsWrites : public Inspector {
bool mayAlias(const IR::Expression *left, const IR::Expression *right) {
auto llocs = get(left);
auto rlocs = get(right);
CHECK_NULL(llocs);
CHECK_NULL(rlocs);
LOG3("Checking overlap between " << llocs << " and " << rlocs);
return llocs->overlaps(rlocs);
}
Expand Down
2 changes: 1 addition & 1 deletion frontends/p4/fromv1.0/programStructure.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ void ProgramStructure::include(cstring filename, cstring ppoptions) {
auto code = P4::P4ParserDriver::parse(file, options.file);
if (code && !::errorCount())
for (auto decl : code->objects) declarations->push_back(decl);
options.closeInput(file);
options.closePreprocessedInput(file);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions frontends/p4/reassociation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ limitations under the License.
namespace P4 {

const IR::Node *Reassociation::reassociate(IR::Operation_Binary *root) {
auto right = root->right->to<IR::Constant>();
const auto *right = root->right->to<IR::Constant>();
if (!right) return root;
auto leftBin = root->left->to<IR::Operation_Binary>();
if (!leftBin) return root;
if (leftBin->getStringOp() != root->getStringOp()) return root;
if (!leftBin->right->is<IR::Constant>()) return root;
auto c = root->checkedTo<IR::Operation_Binary>()->clone();
auto *c = root->clone();
c->left = leftBin->right;
c->right = root->right;
root->left = leftBin->left;
Expand Down
13 changes: 10 additions & 3 deletions frontends/p4/typeChecking/typeChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3081,17 +3081,21 @@ const IR::Node *TypeInference::postorder(IR::Slice *expression) {
auto ei = EnumInstance::resolve(expression->e1, typeMap);
CHECK_NULL(ei);
auto sei = ei->to<SerEnumInstance>();
if (sei == nullptr)
if (sei == nullptr) {
typeError("%1%: slice bit index values must be constants", expression->e1);
return expression;
}
expression->e1 = sei->value;
}
auto e2type = getType(expression->e2);
if (e2type->is<IR::Type_SerEnum>()) {
auto ei = EnumInstance::resolve(expression->e2, typeMap);
CHECK_NULL(ei);
auto sei = ei->to<SerEnumInstance>();
if (sei == nullptr)
if (sei == nullptr) {
typeError("%1%: slice bit index values must be constants", expression->e2);
return expression;
}
expression->e2 = sei->value;
}

Expand Down Expand Up @@ -3430,8 +3434,11 @@ const IR::Expression *TypeInference::actionCall(bool inActionList,
}
auto method = actionCall->method;
auto methodType = getType(method);
if (!methodType->is<IR::Type_Action>()) typeError("%1%: must be an action", method);
auto baseType = methodType->to<IR::Type_Action>();
if (!baseType) {
typeError("%1%: must be an action", method);
return actionCall;
}
LOG2("Action type " << baseType);
BUG_CHECK(method->is<IR::PathExpression>(), "%1%: unexpected call", method);
BUG_CHECK(baseType->returnType == nullptr, "%1%: action with return type?",
Expand Down
2 changes: 1 addition & 1 deletion lib/error_reporter.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class ErrorReporter {
typename... Args>
void diagnose(DiagnosticAction action, const int errorCode, const char *format,
const char *suffix, const T *node, Args... args) {
if (!error_reported(errorCode, node->getSourceInfo())) {
if (node && !error_reported(errorCode, node->getSourceInfo())) {
const char *name = get_error_name(errorCode);
auto da = getDiagnosticAction(name, action);
if (name)
Expand Down
8 changes: 6 additions & 2 deletions lib/options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ limitations under the License.

#include "options.h"

#include "lib/null.h"

void Util::Options::registerOption(const char *option, const char *argName,
OptionProcessor processor, const char *description,
OptionFlags flags /* = OptionFlags::Default */) {
Expand Down Expand Up @@ -114,16 +116,18 @@ void Util::Options::usage() {
*outStream << binaryName << ": " << message << std::endl;

size_t labelLen = 0;
for (auto o : optionOrder) {
for (const auto &o : optionOrder) {
size_t len = o.size();
auto option = get(options, o);
CHECK_NULL(option);
if (option->argName != nullptr) len += 1 + strlen(option->argName);
if (labelLen < len) labelLen = len;
}

labelLen += 3;
for (auto o : optionOrder) {
for (const auto &o : optionOrder) {
auto option = get(options, o);
CHECK_NULL(option);
size_t len = strlen(o);
if (option->flags & OptionFlags::Hide) continue;
*outStream << option->option;
Expand Down
9 changes: 4 additions & 5 deletions midend/convertErrors.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,12 @@ const IR::Node *DoConvertErrors::postorder(IR::Type_Name *type) {
}

const IR::Node *DoConvertErrors::postorder(IR::Member *member) {
if (!member->type->is<IR::Type_Error>()) {
const auto *typeErr = member->type->to<IR::Type_Error>();
if (!typeErr) {
return member;
}
if (!member->type->is<IR::Type_Error>()) {
return member;
}
auto *r = ::get(repr, member->type->to<IR::Type_Error>()->name);
auto *r = ::get(repr, typeErr->name);
CHECK_NULL(r);
if (!member->expr->is<IR::TypeNameExpression>()) {
// variable
auto *newMember = member->clone();
Expand Down
2 changes: 1 addition & 1 deletion midend/removeComplexExpressions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.

auto result = new IR::Vector<IR::Expression>();
for (auto e : *vec) {
auto r = simplifyExpression(e, force);
Expand Down
Loading