diff --git a/include/analysis/ConstParamAnalysis.hpp b/include/analysis/ConstParamAnalysis.hpp index 27db3d4..3790d48 100644 --- a/include/analysis/ConstParamAnalysis.hpp +++ b/include/analysis/ConstParamAnalysis.hpp @@ -28,7 +28,10 @@ namespace ctrace::stack::analysis std::uint64_t pointerConstOnly : 1 = false; // ex: T * const param std::uint64_t isReference : 1 = false; std::uint64_t isRvalueRef : 1 = false; - std::uint64_t reservedFlags : 61 = 0; + // The parameter is only passed to callees whose parameter is declared to point to const: + // their bodies are not read, so the object is not known to be unmodified (#167). + std::uint64_t reliesOnCalleeConst : 1 = false; + std::uint64_t reservedFlags : 60 = 0; }; // For the functions defined in the modules analyzed together, keyed by linker symbol: whether diff --git a/run_test.py b/run_test.py index 71b6bbc..8107762 100755 --- a/run_test.py +++ b/run_test.py @@ -2821,6 +2821,43 @@ def reported(files: list[str], extra: list[str]) -> Optional[list[str]]: return ok +def check_const_param_contract_message_across_files() -> bool: + """ + #167: a pointer passed only to a function defined in another file, whose parameter points to + const, is reported for that contract alone. The message must say so: the callee's body is not + checked, and this one casts the qualifier away. + """ + print("=== Testing the const parameter message when a callee's contract is the evidence ===") + sources = { + "def.c": "int bump(const int* p)\n{\n *(int*)p += 1;\n return *p;\n}\n", + "use.c": "int bump(const int* p);\n\nint use(int* p)\n{\n return bump(p);\n}\n", + } + headline = ( + "ConstParameterNotModified.Pointer: parameter 'p' in function 'use' is only read, " + "or passed where a pointer to const is expected" + ) + caveat = "the callee's body is not checked: it may still modify the pointed object through a cast" + old = "is never used to modify the pointed object" + with tempfile.TemporaryDirectory(prefix="ct_const_contract_") as tmp: + root = Path(tmp) + for name, text in sources.items(): + (root / name).write_text(text) + result = run_analyzer([str(root / "def.c"), str(root / "use.c"), "--format=json"]) + try: + payload = json.loads(result.stdout or "") + except json.JSONDecodeError: + return fail_check("no JSON output", (result.stdout or "") + (result.stderr or "")) + messages = [ + d.get("details", {}).get("message", "") + for d in payload.get("diagnostics", []) + if str(d.get("ruleId", "")).startswith("ConstParameterNotModified") + ] + if len(messages) != 1 or headline not in messages[0] or caveat not in messages[0] or old in messages[0]: + return fail_check(f"expected one contract message for use, got {messages}") + print(" ✅ use: reported for the contract of bump, and says so") + print() + return True + def check_duplicate_if_cross_tu() -> bool: """ #157: a function defined in another file is deterministic when every definition of it is, as @@ -5459,6 +5496,7 @@ def record_ok(ok: bool): check_uninitialized_cross_tu, check_uninitialized_cross_tu_no_effect, check_const_param_cross_tu, + check_const_param_contract_message_across_files, check_duplicate_if_cross_tu, check_global_read_before_write_identity_across_files, check_size_minus_one_cross_tu, diff --git a/src/analysis/ConstParamAnalysis.cpp b/src/analysis/ConstParamAnalysis.cpp index f55f9eb..9ea9844 100644 --- a/src/analysis/ConstParamAnalysis.cpp +++ b/src/analysis/ConstParamAnalysis.cpp @@ -3,6 +3,7 @@ #include "analysis/IRValueUtils.hpp" #include "analysis/AnalyzerUtils.hpp" +#include #include #include @@ -288,17 +289,22 @@ namespace ctrace::stack::analysis enum class ParamWriteState : std::uint8_t { NoWrite = 0, - Unknown = 1, - MayWrite = 2 + // No write, as far as the callees' declared const parameters say: their bodies are + // not read, and one may cast the qualifier away (#167). + NoWriteByContract = 1, + Unknown = 2, + MayWrite = 3 }; + static bool writesNothing(ParamWriteState state) + { + return state == ParamWriteState::NoWrite || state == ParamWriteState::NoWriteByContract; + } + + // The least certain of both. static ParamWriteState mergeParamWriteState(ParamWriteState lhs, ParamWriteState rhs) { - if (lhs == ParamWriteState::MayWrite || rhs == ParamWriteState::MayWrite) - return ParamWriteState::MayWrite; - if (lhs == ParamWriteState::Unknown || rhs == ParamWriteState::Unknown) - return ParamWriteState::Unknown; - return ParamWriteState::NoWrite; + return std::max(lhs, rhs); } static bool calleeParamIsReadOnly(const llvm::Function* callee, unsigned argIndex) @@ -374,7 +380,7 @@ namespace ctrace::stack::analysis if (it != otherModules->functions.end() && it->second.size() == CB.arg_size() && it->second[argIndex]) { - return ParamWriteState::NoWrite; + return ParamWriteState::NoWriteByContract; } } @@ -397,7 +403,7 @@ namespace ctrace::stack::analysis } if (calleeParamIsReadOnly(callee, argIndex)) - return ParamWriteState::NoWrite; + return ParamWriteState::NoWriteByContract; return ParamWriteState::Unknown; } @@ -700,7 +706,7 @@ namespace ctrace::stack::analysis const ParamWriteState writeState = readOnlyByMetadata ? ParamWriteState::NoWrite : valueWriteState(&Arg, F, otherModules); - if (writeState != ParamWriteState::NoWrite) + if (!writesNothing(writeState)) continue; const std::string paramName = @@ -719,6 +725,7 @@ namespace ctrace::stack::analysis typeInfo.isPointer && typeInfo.pointerConst && !typeInfo.pointeeConst; issue.isReference = typeInfo.isReference; issue.isRvalueRef = typeInfo.isRvalueReference; + issue.reliesOnCalleeConst = writeState == ParamWriteState::NoWriteByContract; issue.currentType = buildTypeString(typeInfo, baseName, false, true, paramName); if (typeInfo.isRvalueReference) diff --git a/src/analyzer/DiagnosticEmitter.cpp b/src/analyzer/DiagnosticEmitter.cpp index 096776f..8e37f32 100644 --- a/src/analyzer/DiagnosticEmitter.cpp +++ b/src/analyzer/DiagnosticEmitter.cpp @@ -1019,39 +1019,58 @@ namespace ctrace::stack::analyzer else if (issue.isReference) subLabel = issue.isRvalueRef ? "ReferenceRvaluePreferValue" : "Reference"; + // Resting on a callee's declared const parameter, the rule only knows how the + // parameter is used, not that the object is unmodified (#167). + const char* object = issue.isReference ? "referred" : "pointed"; + const std::string contractUse = std::string("only read, or passed where a ") + + (issue.isReference ? "reference" : "pointer") + + " to const is expected"; + body << "\t" << prefixForSeverity(DiagnosticSeverity::Info) + << " ConstParameterNotModified." << subLabel << ": parameter '" << paramName + << "' in function '" << displayFuncName << "' "; + if (issue.isRvalueRef) + { + body << "is an rvalue reference and is " + << (issue.reliesOnCalleeConst ? contractUse + : "never used to modify the referred object"); + } + else if (issue.pointerConstOnly) + { + body << "is declared '" << issue.currentType << "' but the pointed object is " + << (issue.reliesOnCalleeConst ? contractUse : "never modified"); + } + else if (issue.reliesOnCalleeConst) + { + body << "is " << contractUse; + } + else + { + body << "is never used to modify the " << object << " object"; + } + body << "\n"; + if (issue.isRvalueRef) { - body << "\t" << prefixForSeverity(DiagnosticSeverity::Info) - << " ConstParameterNotModified." << subLabel << ": parameter '" << paramName - << "' in function '" << displayFuncName - << "' is an rvalue reference and is never used to modify the referred " - "object\n"; body << kDiagIndentArrow << "consider passing by value (" << issue.suggestedType << ") or const reference (" << issue.suggestedTypeAlt << ")\n"; body << kDiagIndentArrow << "current type: " << issue.currentType << "\n"; } else if (issue.pointerConstOnly) { - body << "\t" << prefixForSeverity(DiagnosticSeverity::Info) - << " ConstParameterNotModified." << subLabel << ": parameter '" << paramName - << "' in function '" << displayFuncName << "' is declared '" - << issue.currentType << "' but the pointed object is never modified\n"; body << kDiagIndentArrow << "consider '" << issue.suggestedType << "' for API const-correctness\n"; } - else - { - body << "\t" << prefixForSeverity(DiagnosticSeverity::Info) - << " ConstParameterNotModified." << subLabel << ": parameter '" << paramName - << "' in function '" << displayFuncName << "' is never used to modify the " - << (issue.isReference ? "referred" : "pointed") << " object\n"; - } if (!issue.isRvalueRef) { body << kDiagIndentArrow << "current type: " << issue.currentType << "\n"; body << kDiagIndentArrow << "suggested type: " << issue.suggestedType << "\n"; } + if (issue.reliesOnCalleeConst) + { + body << kDiagIndentArrow << "the callee's body is not checked: it may still modify " + << "the " << object << " object through a cast\n"; + } DiagnosticBuilder builder; builder.function(issue.funcName) diff --git a/test/pointer_reference-const_correctness/advanced-cases.c b/test/pointer_reference-const_correctness/advanced-cases.c index 09071ac..9c7b7ad 100644 --- a/test/pointer_reference-const_correctness/advanced-cases.c +++ b/test/pointer_reference-const_correctness/advanced-cases.c @@ -20,7 +20,7 @@ void consume_mut(int* p) } // at line 25, column 0 -// [ !Info! ] ConstParameterNotModified.Pointer: parameter 'p' in function 'caller_const' is never used to modify the pointed object +// [ !Info! ] ConstParameterNotModified.Pointer: parameter 'p' in function 'caller_const' is only read, or passed where a pointer to const is expected // ↳ current type: int *p // ↳ suggested type: const int *p void caller_const(int* p) diff --git a/test/pointer_reference-const_correctness/advanced-cases.cpp b/test/pointer_reference-const_correctness/advanced-cases.cpp index 43be7c2..ab1ee7f 100644 --- a/test/pointer_reference-const_correctness/advanced-cases.cpp +++ b/test/pointer_reference-const_correctness/advanced-cases.cpp @@ -12,7 +12,7 @@ void consume_ref_mut(int& v) } // at line 17, column 0 -// [ !Info! ] ConstParameterNotModified.Reference: parameter 'v' in function 'caller_ref(int&)' is never used to modify the referred object +// [ !Info! ] ConstParameterNotModified.Reference: parameter 'v' in function 'caller_ref(int&)' is only read, or passed where a reference to const is expected // ↳ current type: int &v // ↳ suggested type: const int &v void caller_ref(int& v) diff --git a/test/pointer_reference-const_correctness/const-callee-contract.c b/test/pointer_reference-const_correctness/const-callee-contract.c new file mode 100644 index 0000000..0adf7c9 --- /dev/null +++ b/test/pointer_reference-const_correctness/const-callee-contract.c @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: Apache-2.0 +// #167: a parameter only passed where a pointer to const is expected is not known to be +// unmodified. The callee's body is not checked, and this one casts the qualifier away. +// not contains: parameter 'p' in function 'use' is never used to modify the pointed object + +static int bump(const int* p) +{ + *(int*)p += 1; + return *p; +} + +// at line 17, column 0 +// [ !Info! ] ConstParameterNotModified.Pointer: parameter 'p' in function 'use' is only read, or passed where a pointer to const is expected +// ↳ current type: int *p +// ↳ suggested type: const int *p +// ↳ the callee's body is not checked: it may still modify the pointed object through a cast +int use(int* p) +{ + return bump(p); +} + +// Without a call, the function's own body establishes that the object is not modified. + +// at line 28, column 0 +// [ !Info! ] ConstParameterNotModified.Pointer: parameter 'q' in function 'peek' is never used to modify the pointed object +// ↳ current type: int *q +// ↳ suggested type: const int *q +int peek(int* q) +{ + return *q; +}