From d1c47a328785ece9a8ffd9bbca3138818b8203b3 Mon Sep 17 00:00:00 2001 From: _ov4 Date: Sun, 26 Jul 2026 12:51:55 +0000 Subject: fix: close correctness gaps found by post-refactor adversarial review Confirmed by a 17-agent review pass over the refactor diff (12 findings verified against the code and reproduced end-to-end): - marshalCallArgs: BYREF arguments must match the declared parameter type exactly (was: silent bit-reinterpretation, e.g. REAL passed to BYREF INTEGER printed garbage with exit 0); whole-array arguments are rejected (was: callee assignment smashed the array data pointer -> runtime SIGSEGV) - Null guards for CHR/LCASE/UCASE/IS_NUM/NUM_TO_STR/STR_TO_NUM/ASC argument emission (was: compiler SIGSEGV on e.g. CHR(undeclared)), matching the guards MID/RIGHT/LEFT already had - ParseReturnStmt: propagate expression-parse failure (was: diagnostic printed but exit 0) and recognize bare RETURN before ENDWHILE/UNTIL/NEXT (was: bogus diagnostic on legal early return) - emitPrototype rejects FUNCTION/PROCEDURE names that collide with runtime/libc symbols (was: FUNCTION strlen silently hijacked the string runtime; arity mismatch crashed createArgumentAllocas) or with builtins (was: silently shadowed); conflicting redefinitions keep the first signature - FOR STEP is evaluated exactly once, before the loop (was: once per iteration; the runtime-direction refactor had moved it to N+1) - RETURN values are coerced to the declared return type via the signature table (RETURN n in a RETURNS REAL function now compiles) - '&' coerces its operands to STRING (CHAR/number operands work); OUTPUT of a PROCEDURE call reports a clean error AGENTS docs updated to match. All positive samples remain byte-identical to the pre-refactor baseline. Co-Authored-By: Claude Fable 5 --- lib/CodeGen/CodeGen.cc | 76 +++++++++++++++++++++++++++++++++++--------- lib/CodeGen/FunctionGen.cc | 46 +++++++++++++++++++++++---- lib/CodeGen/StringHandler.cc | 1 + lib/Parser/ParserFunction.cc | 6 +++- 4 files changed, 106 insertions(+), 23 deletions(-) (limited to 'lib') diff --git a/lib/CodeGen/CodeGen.cc b/lib/CodeGen/CodeGen.cc index 31a0274..ffe10b1 100644 --- a/lib/CodeGen/CodeGen.cc +++ b/lib/CodeGen/CodeGen.cc @@ -45,6 +45,10 @@ const BuiltinInfo *findBuiltin(const std::string &Name) { CodeGen::~CodeGen() = default; +bool CodeGen::isBuiltinName(const std::string &Name) { + return findBuiltin(Name) != nullptr; +} + void CodeGen::reportError(const char *Fmt, ...) { va_list Args; va_start(Args, Fmt); @@ -500,6 +504,9 @@ Value *CodeGen::emitExpr(ExprAST *Expr) { Value *R = emitExpr(Bin->getRHS()); if (!L || !R) return nullptr; if (Bin->getOp() == '&') { + L = coerceValueToType(L, resolveType("STRING")); + R = coerceValueToType(R, resolveType("STRING")); + if (!L || !R) return nullptr; return StrHandler->emitConcat(L, R); } return ArithHandler->emitBinaryOp(Bin->getOp(), L, R, Bin->getLine()); @@ -536,13 +543,18 @@ Value *CodeGen::emitExpr(ExprAST *Expr) { return StrHandler->emitLeft(Str, Len); } if (Name == "LCASE") { - return StrHandler->emitLCase(emitExpr(Call->getArgs()[0].get())); + Value *Str = emitExpr(Call->getArgs()[0].get()); + if (!Str) return nullptr; + return StrHandler->emitLCase(Str); } if (Name == "UCASE") { - return StrHandler->emitUCase(emitExpr(Call->getArgs()[0].get())); + Value *Str = emitExpr(Call->getArgs()[0].get()); + if (!Str) return nullptr; + return StrHandler->emitUCase(Str); } if (Name == "ASC") { Value *ArgVal = emitExpr(Call->getArgs()[0].get()); + if (!ArgVal) return nullptr; const TypeInfo *ArgType = getExprTypeInfo(Call->getArgs()[0].get()); Value *CharVal = nullptr; if (ArgType && ArgType->isChar()) { @@ -555,14 +567,18 @@ Value *CodeGen::emitExpr(ExprAST *Expr) { } if (Name == "CHR") { Value *IntVal = coerceValueToType(emitExpr(Call->getArgs()[0].get()), resolveType("INTEGER")); + if (!IntVal) return nullptr; Value *CharVal = Builder->CreateTrunc(IntVal, Type::getInt8Ty(*TheContext), "chr_val"); return coerceValueToType(CharVal, resolveType("STRING")); } if (Name == "IS_NUM") { - return StrConvHandler->emitIsNum(emitExpr(Call->getArgs()[0].get())); + Value *Str = emitExpr(Call->getArgs()[0].get()); + if (!Str) return nullptr; + return StrConvHandler->emitIsNum(Str); } if (Name == "NUM_TO_STR") { Value *NumV = emitExpr(Call->getArgs()[0].get()); + if (!NumV) return nullptr; bool IsReal = NumV->getType()->isDoubleTy(); if (NumV->getType()->isIntegerTy(8)) { NumV = coerceValueToType(NumV, resolveType("INTEGER")); @@ -570,7 +586,9 @@ Value *CodeGen::emitExpr(ExprAST *Expr) { return StrConvHandler->emitNumToStr(NumV, IsReal); } if (Name == "STR_TO_NUM") { - return StrConvHandler->emitStrToNum(emitExpr(Call->getArgs()[0].get()), true); + Value *Str = emitExpr(Call->getArgs()[0].get()); + if (!Str) return nullptr; + return StrConvHandler->emitStrToNum(Str, true); } std::vector Args; @@ -602,18 +620,36 @@ bool CodeGen::marshalCallArgs(const std::string &Callee, bool IsByRef = Sig->Params[i].second; ExprAST *ArgExpr = ArgExprs[i].get(); + // Parameters can never be arrays, so a whole-array argument is always + // a user error (and would smash the data pointer if let through). + if (auto *Var = dynamic_cast(ArgExpr)) { + const SymbolInfo *Info = getSymbolInfo(Var->getName()); + if (Info && Info->IsArray) { + reportError("Cannot pass whole array %s to %s; pass an element like %s[i]", + Var->getName().c_str(), Callee.c_str(), Var->getName().c_str()); + return false; + } + } + if (IsByRef) { auto *Var = dynamic_cast(ArgExpr); if (!Var) { reportError("BYREF argument to %s must be a variable", Callee.c_str()); return false; } - Value *Ptr = getNamedValue(Var->getName()); - if (!Ptr) { + const SymbolInfo *Info = getSymbolInfo(Var->getName()); + if (!Info) { reportError("Unknown variable %s in BYREF call", Var->getName().c_str()); return false; } - Out.push_back(Ptr); + // BYREF aliases the storage, so the types must match exactly. + if (Info->TypeName != TypeName) { + reportError("BYREF argument %s to %s must be of type %s (got %s)", + Var->getName().c_str(), Callee.c_str(), + TypeName.c_str(), Info->TypeName.c_str()); + return false; + } + Out.push_back(Info->Storage); } else { Value *V = coerceValueToType(emitExpr(ArgExpr), resolveType(TypeName)); if (!V) return false; @@ -716,6 +752,15 @@ void CodeGen::emitForStmt(ForStmtAST *Stmt) { Value *Alloca = Symbol->Storage; Builder->CreateStore(StartVal, Alloca); + // STEP is evaluated exactly once, before the loop. + Value *StepVal = nullptr; + if (Stmt->getStep()) { + StepVal = coerceValueToType(emitExpr(Stmt->getStep()), resolveType("INTEGER")); + if (!StepVal) return; + } else { + StepVal = ConstantInt::get(*TheContext, APInt(64, 1)); + } + BasicBlock *CondBB = BasicBlock::Create(*TheContext, "forcond", TheFunction); BasicBlock *LoopBB = BasicBlock::Create(*TheContext, "forloop", TheFunction); BasicBlock *IncBB = BasicBlock::Create(*TheContext, "forinc", TheFunction); @@ -728,14 +773,6 @@ void CodeGen::emitForStmt(ForStmtAST *Stmt) { Value *EndVal = coerceValueToType(emitExpr(Stmt->getEnd()), resolveType("INTEGER")); if (!EndVal) return; - Value *StepVal = nullptr; - if (Stmt->getStep()) { - StepVal = coerceValueToType(emitExpr(Stmt->getStep()), resolveType("INTEGER")); - if (!StepVal) return; - } else { - StepVal = ConstantInt::get(*TheContext, APInt(64, 1)); - } - // The loop direction depends on the STEP value at run time. Value *StepNonNeg = Builder->CreateICmpSGE(StepVal, ConstantInt::get(*TheContext, APInt(64, 0)), @@ -795,6 +832,11 @@ void CodeGen::emitStmt(StmtAST *Stmt) { Value *RetVal = nullptr; if (Ret->getRetVal()) { RetVal = emitExpr(Ret->getRetVal()); + Function *F = Builder->GetInsertBlock()->getParent(); + if (const FuncSig *Sig = FuncGen->getSignature(F->getName().str())) { + RetVal = coerceValueToType(RetVal, resolveType(Sig->ReturnTypeName)); + if (!RetVal) return; + } } FuncGen->emitReturn(Ret, RetVal); return; @@ -885,6 +927,10 @@ void CodeGen::emitStmt(StmtAST *Stmt) { reportError("Cannot infer the type of the OUTPUT expression"); return; } + if (TypeInfo->isVoid()) { + reportError("Cannot OUTPUT the result of a PROCEDURE call"); + return; + } emitOutputValue(Val, TypeInfo, true); return; } diff --git a/lib/CodeGen/FunctionGen.cc b/lib/CodeGen/FunctionGen.cc index 2e5a704..b778bbe 100644 --- a/lib/CodeGen/FunctionGen.cc +++ b/lib/CodeGen/FunctionGen.cc @@ -1,10 +1,24 @@ #include "cps/FunctionGen.h" -#include "llvm/IR/Verifier.h" +#include "cps/CodeGen.h" #include using namespace llvm; using namespace cps; +// Symbols the compiler itself declares (libc + runtime); a user +// FUNCTION/PROCEDURE with one of these names would hijack the runtime. +static bool isReservedRuntimeName(const std::string &Name) { + static const char *Reserved[] = { + "main", "printf", "scanf", "malloc", "free", "strlen", "memcpy", + "strcpy", "strcat", "toupper", "tolower", "sprintf", "strtol", + "strtod", "memset", "exit", + }; + for (const char *R : Reserved) { + if (Name == R) return true; + } + return false; +} + Type *FunctionGen::getLLVMType(const std::string &TypeName) { Type *Resolved = Types.getLLVMType(TypeName); if (!Resolved) { @@ -45,6 +59,19 @@ const FuncSig *FunctionGen::getSignature(const std::string &Name) const { } Function *FunctionGen::emitPrototype(PrototypeAST *Proto) { + if (isReservedRuntimeName(Proto->getName())) { + fprintf(stderr, "Error: '%s' is a reserved runtime symbol and cannot be a FUNCTION/PROCEDURE name\n", + Proto->getName().c_str()); + HadError = true; + return nullptr; + } + if (CodeGen::isBuiltinName(Proto->getName())) { + fprintf(stderr, "Error: '%s' is a built-in function and cannot be redefined\n", + Proto->getName().c_str()); + HadError = true; + return nullptr; + } + FuncSig Sig; Sig.ReturnTypeName = Proto->getReturnType(); @@ -57,15 +84,24 @@ Function *FunctionGen::emitPrototype(PrototypeAST *Proto) { ArgTypes.push_back(T); Sig.Params.emplace_back(std::get<1>(Arg), std::get<2>(Arg)); } - Signatures[Proto->getName()] = std::move(Sig); Type *RetType = getLLVMType(Proto->getReturnType()); FunctionType *FT = FunctionType::get(RetType, ArgTypes, false); Function *F = Module.getFunction(Proto->getName()); + if (F && F->getFunctionType() != FT) { + fprintf(stderr, "Error: Function %s conflicts with an existing declaration of a different type\n", + Proto->getName().c_str()); + HadError = true; + return nullptr; + } if (!F) { F = Function::Create(FT, Function::ExternalLinkage, Proto->getName(), &Module); } + // Keep the first registration: on a duplicate definition the first one + // wins and the second is rejected by emitFunctionDef. + Signatures.emplace(Proto->getName(), std::move(Sig)); + unsigned Idx = 0; for (auto &Arg : F->args()) { if (Idx < Proto->getArgs().size()) { @@ -78,11 +114,7 @@ Function *FunctionGen::emitPrototype(PrototypeAST *Proto) { Function *FunctionGen::emitFunctionDef(FunctionDefAST *FuncAST, const std::function &StmtEmitter) { PrototypeAST *Proto = FuncAST->getProto(); - Function *TheFunction = Module.getFunction(Proto->getName()); - - if (!TheFunction) { - TheFunction = emitPrototype(Proto); - } + Function *TheFunction = emitPrototype(Proto); if (!TheFunction) return nullptr; if (!TheFunction->empty()) { diff --git a/lib/CodeGen/StringHandler.cc b/lib/CodeGen/StringHandler.cc index 783f8e5..546506d 100644 --- a/lib/CodeGen/StringHandler.cc +++ b/lib/CodeGen/StringHandler.cc @@ -128,6 +128,7 @@ Value *StringHandler::emitLeft(Value *Str, Value *Len) { // Shared loop for LCASE/UCASE: malloc a copy of Str with every byte run // through CaseFn (tolower/toupper). Value *StringHandler::emitCaseConvert(Value *Str, FunctionCallee CaseFn) { + if (!Str) return nullptr; Value *Len = emitLength(Str); Value *One = ConstantInt::get(Context, APInt(64, 1)); Value *AllocSize = Builder.CreateAdd(Len, One); diff --git a/lib/Parser/ParserFunction.cc b/lib/Parser/ParserFunction.cc index 6ce0b8e..7663f38 100644 --- a/lib/Parser/ParserFunction.cc +++ b/lib/Parser/ParserFunction.cc @@ -137,8 +137,12 @@ std::unique_ptr Parser::ParseCallStmt() { std::unique_ptr Parser::ParseReturnStmt() { getNextToken(); std::unique_ptr Expr = nullptr; - if (CurTok != tok_endif && CurTok != tok_else && CurTok != tok_endfunction && CurTok != tok_endprocedure) { + // A bare RETURN is recognized right before any block terminator. + if (CurTok != tok_endif && CurTok != tok_else && CurTok != tok_endfunction && + CurTok != tok_endprocedure && CurTok != tok_endwhile && CurTok != tok_until && + CurTok != tok_next && CurTok != tok_eof) { Expr = ParseExpression(); + if (!Expr) return nullptr; } return std::make_unique(std::move(Expr)); } -- cgit v1.3.1-4-g156e