Skip to content

Commit

Permalink
Format-related string fixes and refactorings (#4704)
Browse files Browse the repository at this point in the history 
* Use abseil StrFormat in SourceFile

* Simplify source file: do not create cstrings for something transient

* Switch to cord for source code builder

* Switch to statically-checked format string printing. Uncovered and fixed few bugs here and there.

* Switch from Utils::printf_format to abseil formatting. Overall cstring's are
misused everywhere in this code. But this is a subject of different task.

* Get rid of unsafe and error-prone printf_format

* Add missed conversion behind #ifdef
  • Loading branch information
@asl
asl authored Jun 8, 2024
1 parent 6359549 commit 7124b4e
Show file tree
Hide file tree
Showing 28 changed files with 208 additions and 318 deletions.
8 changes: 4 additions & 4 deletions backends/ebpf/ebpfControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ bool ControlBodyTranslator::preorder(const IR::MethodCallExpression *expression)
BUG_CHECK(expression->arguments->size() == 0, "%1%: unexpected arguments for action call",
expression);
cstring msg =
Util::printf_format("Control: explicit calling action %s()", ac->action->name.name);
absl::StrFormat("Control: explicit calling action %s()", ac->action->name.name);
builder->target->emitTraceMessage(builder, msg.c_str());
visit(ac->action->body);
return false;
Expand Down Expand Up @@ -271,7 +271,7 @@ void ControlBodyTranslator::compileEmit(const IR::Vector<IR::Argument> *args) {

// Increment header pointer
builder->emitIndent();
builder->appendFormat("%s += BYTES(%s);", program->headerStartVar.c_str(), width);
builder->appendFormat("%s += BYTES(%d);", program->headerStartVar.c_str(), width);
builder->newline();

builder->blockEnd(true);
Expand Down Expand Up @@ -304,7 +304,7 @@ void ControlBodyTranslator::processApply(const P4::ApplyMethod *method) {
auto table = control->getTable(method->object->getName().name);
BUG_CHECK(table != nullptr, "No table for %1%", method->expr);

msgStr = Util::printf_format("Control: applying %s", method->object->getName().name);
msgStr = absl::StrFormat("Control: applying %s", method->object->getName().name);
builder->target->emitTraceMessage(builder, msgStr.c_str());

builder->emitIndent();
Expand Down Expand Up @@ -393,7 +393,7 @@ void ControlBodyTranslator::processApply(const P4::ApplyMethod *method) {
builder->blockEnd(true);
builder->blockEnd(true);

msgStr = Util::printf_format("Control: %s applied", method->object->getName().name);
msgStr = absl::StrFormat("Control: %s applied", method->object->getName().name);
builder->target->emitTraceMessage(builder, msgStr.c_str());
}

Expand Down
8 changes: 4 additions & 4 deletions backends/ebpf/ebpfDeparser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ void DeparserHdrEmitTranslator::processMethod(const P4::ExternMethod *method) {
"Header %1% size %2% is not a multiple of 8 bits.", expr, width);
return;
}
msgStr = Util::printf_format("Deparser: emitting header %s", expr->toString().c_str());
msgStr = absl::StrFormat("Deparser: emitting header %s", expr->toString().c_str());
builder->target->emitTraceMessage(builder, msgStr.c_str());

builder->emitIndent();
Expand Down Expand Up @@ -201,13 +201,13 @@ void DeparserHdrEmitTranslator::emitField(CodeBuilder *builder, cstring field,
visit(hdrExpr);
builder->appendFormat(".%s", field.c_str());
builder->endOfStatement(true);
msgStr = Util::printf_format("Deparser: emitting field %s=0x%%llx (%u bits)", field,
widthToEmit);
msgStr = absl::StrFormat("Deparser: emitting field %s=0x%%llx (%u bits)", field,
widthToEmit);
builder->target->emitTraceMessage(builder, msgStr.c_str(), 1, "tmp");
builder->blockEnd(true);
}
} else {
msgStr = Util::printf_format("Deparser: emitting field %s (%u bits)", field, widthToEmit);
msgStr = absl::StrFormat("Deparser: emitting field %s (%u bits)", field, widthToEmit);
builder->target->emitTraceMessage(builder, msgStr.c_str());
}

Expand Down
39 changes: 19 additions & 20 deletions backends/ebpf/ebpfParser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ limitations under the License.
namespace EBPF {

void StateTranslationVisitor::compileLookahead(const IR::Expression *destination) {
cstring msgStr = Util::printf_format("Parser: lookahead for %s %s",
state->parser->typeMap->getType(destination)->toString(),
destination->toString());
cstring msgStr = absl::StrFormat("Parser: lookahead for %s %s",
state->parser->typeMap->getType(destination)->toString(),
destination->toString());
builder->target->emitTraceMessage(builder, msgStr.c_str());

builder->emitIndent();
Expand All @@ -49,8 +49,8 @@ void StateTranslationVisitor::compileAdvance(const P4::ExternMethod *extMethod)
if (cnst) {
cstring argStr = cstring::to_cstring(cnst->asUnsigned());
cstring offsetStr =
Util::printf_format("%s - (u8*)%s + BYTES(%s)", state->parser->program->headerStartVar,
state->parser->program->packetStartVar, argStr);
absl::StrFormat("%s - (u8*)%s + BYTES(%s)", state->parser->program->headerStartVar,
state->parser->program->packetStartVar, argStr);
builder->target->emitTraceMessage(builder,
"Parser (advance): check pkt_len=%%d < "
"last_read_byte=%%d",
Expand Down Expand Up @@ -115,8 +115,8 @@ void StateTranslationVisitor::compileVerify(const IR::MethodCallExpression *expr
errorMember->member.name);
builder->endOfStatement(true);

cstring msg = Util::printf_format("Verify: condition failed, parser_error=%%u (%s)",
errorMember->member.name);
cstring msg = absl::StrFormat("Verify: condition failed, parser_error=%%u (%s)",
errorMember->member.name);
builder->target->emitTraceMessage(builder, msg.c_str(), 1,
state->parser->program->errorVar.c_str());

Expand Down Expand Up @@ -156,10 +156,9 @@ bool StateTranslationVisitor::preorder(const IR::ParserState *parserState) {
builder->spc();
builder->blockStart();

cstring msgStr =
Util::printf_format("Parser: state %s (curr_offset=%%d)", parserState->name.name);
cstring offsetStr = Util::printf_format("%s - (u8*)%s", state->parser->program->headerStartVar,
state->parser->program->packetStartVar);
cstring msgStr = absl::StrFormat("Parser: state %s (curr_offset=%%d)", parserState->name.name);
cstring offsetStr = absl::StrFormat("%s - (u8*)%s", state->parser->program->headerStartVar,
state->parser->program->packetStartVar);
builder->target->emitTraceMessage(builder, msgStr.c_str(), 1, offsetStr.c_str());

visit(parserState->components, "components");
Expand Down Expand Up @@ -283,7 +282,7 @@ void StateTranslationVisitor::compileExtractField(const IR::Expression *expr,
cstring msgStr;
cstring fieldName = field->name.name;

msgStr = Util::printf_format("Parser: extracting field %s", fieldName);
msgStr = absl::StrFormat("Parser: extracting field %s", fieldName);
builder->target->emitTraceMessage(builder, msgStr.c_str());

if (widthToExtract <= 64) {
Expand Down Expand Up @@ -390,13 +389,13 @@ void StateTranslationVisitor::compileExtractField(const IR::Expression *expr,
expr->to<IR::Member>()->expr->to<IR::PathExpression>()->path->name.name)) {
exprStr = exprStr.replace("."_cs, "->"_cs);
}
cstring tmp = Util::printf_format("(unsigned long long) %s.%s", exprStr, fieldName);
cstring tmp = absl::StrFormat("(unsigned long long) %s.%s", exprStr, fieldName);

msgStr = Util::printf_format("Parser: extracted %s=0x%%llx (%u bits)", fieldName,
widthToExtract);
msgStr =
absl::StrFormat("Parser: extracted %s=0x%%llx (%u bits)", fieldName, widthToExtract);
builder->target->emitTraceMessage(builder, msgStr.c_str(), 1, tmp.c_str());
} else {
msgStr = Util::printf_format("Parser: extracted %s (%u bits)", fieldName, widthToExtract);
msgStr = absl::StrFormat("Parser: extracted %s (%u bits)", fieldName, widthToExtract);
builder->target->emitTraceMessage(builder, msgStr.c_str());
}
}
Expand All @@ -421,8 +420,8 @@ void StateTranslationVisitor::compileExtract(const IR::Expression *destination)

auto program = state->parser->program;

cstring offsetStr = Util::printf_format("(%s - (u8*)%s) + BYTES(%s)", program->headerStartVar,
program->packetStartVar, cstring::to_cstring(width));
cstring offsetStr = absl::StrFormat("(%s - (u8*)%s) + BYTES(%s)", program->headerStartVar,
program->packetStartVar, cstring::to_cstring(width));

builder->target->emitTraceMessage(builder, "Parser: check pkt_len=%d >= last_read_byte=%d", 2,
program->lengthVar.c_str(), offsetStr.c_str());
Expand Down Expand Up @@ -465,7 +464,7 @@ void StateTranslationVisitor::compileExtract(const IR::Expression *destination)
builder->newline();
builder->blockEnd(true);

msgStr = Util::printf_format("Parser: extracting header %s", destination->toString());
msgStr = absl::StrFormat("Parser: extracting header %s", destination->toString());
builder->target->emitTraceMessage(builder, msgStr.c_str());
builder->newline();

Expand Down Expand Up @@ -495,7 +494,7 @@ void StateTranslationVisitor::compileExtract(const IR::Expression *destination)
builder->appendFormat("%s += BYTES(%u);", program->headerStartVar.c_str(), width);
builder->newline();

msgStr = Util::printf_format("Parser: extracted %s", destination->toString());
msgStr = absl::StrFormat("Parser: extracted %s", destination->toString());
builder->target->emitTraceMessage(builder, msgStr.c_str());

builder->newline();
Expand Down
38 changes: 18 additions & 20 deletions backends/ebpf/ebpfTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ bool ActionTranslationVisitor::isActionParameter(const IR::PathExpression *expre

cstring ActionTranslationVisitor::getParamInstanceName(const IR::Expression *expression) const {
cstring actionName = EBPFObject::externalName(action);
auto paramStr =
Util::printf_format("%s->u.%s.%s", valueName, actionName, expression->toString());
return paramStr;
return absl::StrFormat("%s->u.%s.%s", valueName, actionName, expression->toString());
}

bool ActionTranslationVisitor::preorder(const IR::P4Action *act) {
Expand Down Expand Up @@ -512,12 +510,12 @@ void EBPFTable::emitKey(CodeBuilder *builder, cstring keyName) {

cstring msgStr, varStr;
if (memcpy) {
msgStr = Util::printf_format("Control: key %s", c->expression->toString());
msgStr = absl::StrFormat("Control: key %s", c->expression->toString());
builder->target->emitTraceMessage(builder, msgStr.c_str());
} else {
msgStr = Util::printf_format("Control: key %s=0x%%llx", c->expression->toString());
varStr = Util::printf_format("(unsigned long long) %s.%s", keyName.c_str(),
fieldName.c_str());
msgStr = absl::StrFormat("Control: key %s=0x%%llx", c->expression->toString());
varStr =
absl::StrFormat("(unsigned long long) %s.%s", keyName.c_str(), fieldName.c_str());
builder->target->emitTraceMessage(builder, msgStr.c_str(), 1, varStr.c_str());
}
}
Expand All @@ -538,21 +536,20 @@ void EBPFTable::emitAction(CodeBuilder *builder, cstring valueName, cstring acti
builder->newline();
builder->increaseIndent();

msgStr = Util::printf_format("Control: executing action %s", name);
msgStr = absl::StrFormat("Control: executing action %s", name);
builder->target->emitTraceMessage(builder, msgStr.c_str());
for (auto param : *(action->parameters)) {
auto etype = EBPFTypeFactory::instance->create(param->type);
unsigned width = etype->as<IHasWidth>().widthInBits();

if (width <= 64) {
convStr = Util::printf_format("(unsigned long long) (%s->u.%s.%s)", valueName, name,
param->toString());
msgStr = Util::printf_format("Control: param %s=0x%%llx (%d bits)",
param->toString(), width);
convStr = absl::StrFormat("(unsigned long long) (%s->u.%s.%s)", valueName, name,
param->toString());
msgStr = absl::StrFormat("Control: param %s=0x%%llx (%d bits)", param->toString(),
width);
builder->target->emitTraceMessage(builder, msgStr.c_str(), 1, convStr.c_str());
} else {
msgStr =
Util::printf_format("Control: param %s (%d bits)", param->toString(), width);
msgStr = absl::StrFormat("Control: param %s (%d bits)", param->toString(), width);
builder->target->emitTraceMessage(builder, msgStr.c_str());
}
}
Expand Down Expand Up @@ -782,7 +779,7 @@ void EBPFTable::emitLookup(CodeBuilder *builder, cstring key, cstring value) {
builder->emitIndent();
builder->appendFormat("for (int i = 0; i < sizeof(struct %s_mask) / 4; i++) ", keyTypeName);
builder->blockStart();
cstring str = Util::printf_format("*(((__u32 *) &%s) + i)", key);
cstring str = absl::StrFormat("*(((__u32 *) &%s) + i)", key);
builder->target->emitTraceMessage(
builder, "Control: [Ternary] Masking next 4 bytes of %llx with mask %llx", 2, str,
"mask[i]");
Expand All @@ -806,8 +803,9 @@ void EBPFTable::emitLookup(CodeBuilder *builder, cstring key, cstring value) {
builder->append("if (!tuple) ");
builder->blockStart();
builder->target->emitTraceMessage(
builder, Util::printf_format("Control: Tuples map %s not found during ternary lookup. Bug?",
instanceName));
builder, absl::StrFormat("Control: Tuples map %s not found during ternary lookup. Bug?",
instanceName)
.c_str());
builder->emitIndent();
builder->append("break;");
builder->newline();
Expand Down Expand Up @@ -861,7 +859,7 @@ cstring EBPFTable::p4ActionToActionIDName(const IR::P4Action *action) const {

cstring actionName = EBPFObject::externalName(action);
cstring tableInstance = dataMapName;
return Util::printf_format("%s_ACT_%s", tableInstance.toUpper(), actionName.toUpper());
return absl::StrFormat("%s_ACT_%s", tableInstance.toUpper(), actionName.toUpper());
}

/// As ternary has precedence over lpm, this function checks if any
Expand Down Expand Up @@ -1145,7 +1143,7 @@ void EBPFValueSet::emitTypes(CodeBuilder *builder) {
} else if (auto tuple = elemType->to<IR::Type_Tuple>()) {
int i = 0;
for (auto field : tuple->components) {
cstring name = Util::printf_format("field%d", i++);
cstring name = absl::StrFormat("field%d", i++);
fieldEmitter(field, name);
fieldNames.emplace_back(std::make_pair(name, field));
}
Expand Down Expand Up @@ -1191,7 +1189,7 @@ void EBPFValueSet::emitKeyInitializer(CodeBuilder *builder, const IR::SelectExpr
}

cstring dst =
Util::printf_format("%s.%s", keyVarName.c_str(), fieldNames.at(i).first.c_str());
absl::StrFormat("%s.%s", keyVarName.c_str(), fieldNames.at(i).first.c_str());
builder->appendFormat("__builtin_memcpy(&%s, &(", dst.c_str());
codeGen->visit(keyExpr);
builder->appendFormat("[0]), sizeof(%s))", dst.c_str());
Expand Down
Loading

0 comments on commit 7124b4e

Please sign in to comment.