From 9b98aee7f6a68affa3ee251f712c48edfad89bb8 Mon Sep 17 00:00:00 2001 From: Hugo Date: Sat, 10 Oct 2026 22:27:21 +0200 Subject: [PATCH 1/2] test(const-param): say when a callee's const parameter is the only evidence A parameter only passed where a pointer to const is expected is reported as "never used to modify the pointed object", yet the callee's body is not checked and may cast the qualifier away (#167). Add a fixture with such a callee, its expected message and a witness whose own body is the evidence, a check of the same case across files, and update the headline of the two existing expectations that rest on a callee's const parameter alone. --- run_test.py | 38 +++++++++++++++++++ .../advanced-cases.c | 2 +- .../advanced-cases.cpp | 2 +- .../const-callee-contract.c | 31 +++++++++++++++ 4 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 test/pointer_reference-const_correctness/const-callee-contract.c diff --git a/run_test.py b/run_test.py index 71b6bbca..81077629 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/test/pointer_reference-const_correctness/advanced-cases.c b/test/pointer_reference-const_correctness/advanced-cases.c index 09071acf..9c7b7adc 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 43be7c28..ab1ee7f7 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 00000000..0adf7c96 --- /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; +} From fc4dc031218a79dfe638f0317970e1bcb8d5ad39 Mon Sep 17 00:00:00 2001 From: Hugo Date: Sat, 10 Oct 2026 22:29:52 +0200 Subject: [PATCH 2/2] fix(const-param): say when a callee's const parameter is the only evidence A parameter passed where a pointer or reference to const is expected counted as not written, from the callee's declaration alone, within one file and across files (#157). The message then stated that the object is never modified, although the callee's body is not checked and may cast the qualifier away. Track that case as its own write state, kept by the merge whenever one use depends on it, and say only what the rule establishes: the parameter is only read, or passed where a pointer to const is expected, and the callee's body is not checked. The same parameters are reported, and a diagnostic established by the function's own uses keeps its message. Fixes #167 --- include/analysis/ConstParamAnalysis.hpp | 5 ++- src/analysis/ConstParamAnalysis.cpp | 27 ++++++++----- src/analyzer/DiagnosticEmitter.cpp | 51 +++++++++++++++++-------- 3 files changed, 56 insertions(+), 27 deletions(-) diff --git a/include/analysis/ConstParamAnalysis.hpp b/include/analysis/ConstParamAnalysis.hpp index 27db3d4d..3790d48d 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/src/analysis/ConstParamAnalysis.cpp b/src/analysis/ConstParamAnalysis.cpp index f55f9eb5..9ea9844c 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 096776f9..8e37f32c 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)