Skip to content

Commit 9b3b160

Browse files
authored
Fix #15028 (release: check premium ids) (#8851)
1 parent 167f49d commit 9b3b160

8 files changed

Lines changed: 147 additions & 33 deletions

createrelease

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,12 @@
2323
# cd ~/cppchecksolutions/addon/coverage
2424
# CPPCHECK_REPO=~/cppchecksolutions/cppcheck python3 coverage.py --code
2525
#
26-
# Check every isPremiumEnabled call: TODO write helper script
27-
# - every id should be in --errorlist
28-
# git grep 'isPremiumEnabled[(]"' | sed 's/.*isPremiumEnabled[(]"//' | sed 's/".*//' | sort | uniq > ids1.txt
29-
# ./cppcheck --errorlist | grep ' id="' | sed 's/.* id="//' | sed 's/".*//' | sort | uniq > ids2.txt
30-
# diff -y ids1.txt ids2.txt
31-
# - premiumaddon: check coverage.py
32-
# python3 coverage.py --id ; sort ids-*.txt | uniq > ~/cppcheck/ids3.txt
33-
# diff -y ids2.txt ids3.txt
26+
# Check that every premium check id is a real, known Cppcheck error id: every
27+
# id passed to isPremiumEnabled(...) and every id referenced by the premium
28+
# addon's MISRA/CERT/AUTOSAR/CWE coverage mapping (coverage.py) must show up
29+
# in `cppcheck --errorlist`, otherwise the id was renamed/removed and the
30+
# premium check or compliance mapping is silently broken.
31+
# tools/release-check-premium-ids.py
3432
#
3533
# Windows installer:
3634
# - ensure latest build was successful

lib/checkclass.cpp

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3841,6 +3841,18 @@ const Check::FileInfo * CheckClass::loadFileInfoFromXml(const tinyxml2::XMLEleme
38413841
return fileInfo;
38423842
}
38433843

3844+
static ErrorMessage oneDefinitionRuleViolationErrorMessage(std::list<ErrorMessage::FileLocation> locationList, const std::string &file0, const std::string &symbolName)
3845+
{
3846+
return ErrorMessage(std::move(locationList),
3847+
file0,
3848+
Severity::error,
3849+
"$symbol:" + symbolName +
3850+
"\nThe one definition rule is violated, different classes/structs have the same name '$symbol'",
3851+
"ctuOneDefinitionRuleViolation",
3852+
CWE_ONE_DEFINITION_RULE,
3853+
Certainty::normal);
3854+
}
3855+
38443856
bool CheckClass::analyseWholeProgram(const CTU::FileInfo &ctu, const std::list<const Check::FileInfo*> &fileInfo, const Settings& settings, ErrorLogger &errorLogger)
38453857
{
38463858
(void)ctu;
@@ -3879,15 +3891,7 @@ bool CheckClass::analyseWholeProgram(const CTU::FileInfo &ctu, const std::list<c
38793891
locationList.emplace_back(nameLoc.fileName, nameLoc.lineNumber, nameLoc.column);
38803892
locationList.emplace_back(it->second.fileName, it->second.lineNumber, it->second.column);
38813893

3882-
const ErrorMessage errmsg(std::move(locationList),
3883-
fi->file0,
3884-
Severity::error,
3885-
"$symbol:" + nameLoc.className +
3886-
"\nThe one definition rule is violated, different classes/structs have the same name '$symbol'",
3887-
"ctuOneDefinitionRuleViolation",
3888-
CWE_ONE_DEFINITION_RULE,
3889-
Certainty::normal);
3890-
errorLogger.reportErr(errmsg);
3894+
errorLogger.reportErr(oneDefinitionRuleViolationErrorMessage(std::move(locationList), fi->file0, nameLoc.className));
38913895

38923896
foundErrors = true;
38933897
}
@@ -3968,5 +3972,5 @@ void CheckClass::getErrorMessages(ErrorLogger& errorLogger, const Settings &sett
39683972
c.virtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
39693973
c.thisUseAfterFree(nullptr, nullptr, nullptr);
39703974
c.unsafeClassRefMemberError(nullptr, "UnsafeClass::var");
3971-
// TODO: ctuOneDefinitionRuleViolation
3975+
errorLogger.reportErr(oneDefinitionRuleViolationErrorMessage({}, "", "classname"));
39723976
}

lib/checkcondition.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1516,9 +1516,7 @@ void CheckConditionImpl::clarifyConditionError(const Token *tok, bool assign, bo
15161516

15171517
void CheckConditionImpl::alwaysTrueFalse()
15181518
{
1519-
const bool pedantic = mSettings.isPremiumEnabled("alwaysTrue") ||
1520-
mSettings.isPremiumEnabled("alwaysFalse") ||
1521-
mSettings.isPremiumEnabled("knownConditionTrueFalse");
1519+
const bool pedantic = mSettings.isPremiumEnabled("knownConditionTrueFalse");
15221520

15231521
if (!pedantic && !mSettings.severity.isEnabled(Severity::style))
15241522
return;

lib/checkother.cpp

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4513,18 +4513,16 @@ void CheckOtherImpl::checkComparePointers()
45134513
if (const Token* parent1 = getParentLifetime(v1.tokvalue, mSettings.library))
45144514
if (var2 == parent1->variable())
45154515
continue;
4516-
comparePointersError(tok, &v1, &v2);
4516+
comparePointersError(tok, &v1, &v2, Token::simpleMatch(tok, "-"));
45174517
}
45184518
}
45194519
}
45204520

4521-
void CheckOtherImpl::comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2)
4521+
void CheckOtherImpl::comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2, bool subtract)
45224522
{
45234523
ErrorPath errorPath;
4524-
std::string verb = "Comparing";
4525-
if (Token::simpleMatch(tok, "-"))
4526-
verb = "Subtracting";
4527-
const char * const id = (verb[0] == 'C') ? "comparePointers" : "subtractPointers";
4524+
const std::string verb = subtract ? "Subtracting" : "Comparing";
4525+
const char * const id = subtract ? "subtractPointers" : "comparePointers";
45284526
if (v1) {
45294527
errorPath.emplace_back(v1->tokvalue->variable()->nameToken(), "Variable declared here.");
45304528
errorPath.insert(errorPath.end(), v1->errorPath.cbegin(), v1->errorPath.cend());
@@ -4993,8 +4991,8 @@ void CheckOther::getErrorMessages(ErrorLogger& errorLogger, const Settings &sett
49934991
c.shadowError(nullptr, "local variable", nullptr, "member");
49944992
c.knownArgumentError(nullptr, nullptr, nullptr, "x", false);
49954993
c.knownPointerToBoolError(nullptr, nullptr);
4996-
c.comparePointersError(nullptr, nullptr, nullptr);
4997-
// TODO: subtractPointers
4994+
c.comparePointersError(nullptr, nullptr, nullptr, false);
4995+
c.comparePointersError(nullptr, nullptr, nullptr, true);
49984996
c.redundantAssignmentError(nullptr, nullptr, "var", false);
49994997
c.redundantInitializationError(nullptr, nullptr, "var", false);
50004998
c.redundantContinueError(nullptr);

lib/checkother.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ class CPPCHECKLIB CheckOtherImpl : public CheckImpl {
325325
void shadowError(const Token *shadows, const std::string &shadowsType, const Token *shadowed, const std::string &shadowedType);
326326
void knownArgumentError(const Token *tok, const Token *ftok, const ValueFlow::Value *value, const std::string &varexpr, bool isVariableExpressionHidden);
327327
void knownPointerToBoolError(const Token* tok, const ValueFlow::Value* value);
328-
void comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2);
328+
void comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2, bool subtract);
329329
void checkModuloOfOneError(const Token *tok);
330330
void unionZeroInitError(const Token *tok, const UnionMember& largestMember);
331331

lib/checkuninitvar.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1557,7 +1557,7 @@ void CheckUninitVarImpl::uninitdataError(const Token *tok, const std::string &va
15571557

15581558
void CheckUninitVarImpl::uninitvarError(const Token *tok, const std::string &varname, ErrorPath errorPath)
15591559
{
1560-
if (diag(tok))
1560+
if (tok && diag(tok))
15611561
return;
15621562
errorPath.emplace_back(tok, "");
15631563
reportError(std::move(errorPath),
@@ -1572,7 +1572,7 @@ void CheckUninitVarImpl::uninitvarError(const Token* tok, const ValueFlow::Value
15721572
{
15731573
if (!mSettings.isEnabled(&v))
15741574
return;
1575-
if (diag(tok))
1575+
if (tok && diag(tok))
15761576
return;
15771577
const Token* ltok = tok;
15781578
if (tok && Token::simpleMatch(tok->astParent(), ".") && astIsRHS(tok))
@@ -1810,7 +1810,8 @@ void CheckUninitVar::getErrorMessages(ErrorLogger& errorLogger, const Settings&
18101810

18111811
ValueFlow::Value v{};
18121812

1813-
c.uninitvarError(nullptr, v); // TODO: does not produce any output
1813+
c.uninitvarError(nullptr, v);
1814+
c.uninitvarError(nullptr, "varname", ErrorPath{});
18141815
c.uninitdataError(nullptr, "varname");
18151816
c.uninitStructMemberError(nullptr, "a.b");
18161817
}

lib/preprocessor.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1067,6 +1067,7 @@ void Preprocessor::getErrorMessages(ErrorLogger &errorLogger, const Settings &se
10671067
preprocessor.missingInclude(loc, "", SystemHeader);
10681068
preprocessor.error(loc, "message", simplecpp::Output::ERROR);
10691069
preprocessor.error(loc, "message", simplecpp::Output::SYNTAX_ERROR);
1070+
preprocessor.error(loc, "message", simplecpp::Output::DIRECTIVE_AS_MACRO_PARAMETER);
10701071
preprocessor.error(loc, "message", simplecpp::Output::UNHANDLED_CHAR_ERROR);
10711072
preprocessor.error(loc, "message", simplecpp::Output::INCLUDE_NESTED_TOO_DEEPLY);
10721073
preprocessor.error(loc, "message", simplecpp::Output::FILE_NOT_FOUND);

tools/release-check-premium-ids.py

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
#!/usr/bin/env python3
2+
#
3+
# Automates the release checklist item in `createrelease`:
4+
#
5+
# # Check every isPremiumEnabled call: TODO write helper script
6+
# # - every id should be in --errorlist
7+
# # - premiumaddon: check coverage.py
8+
#
9+
# Two checks are performed:
10+
#
11+
# 1. Every id passed to isPremiumEnabled("...") in lib/ must be a real,
12+
# known Cppcheck error id (i.e. it must show up in `cppcheck
13+
# --errorlist`). Otherwise the premium check can never actually be
14+
# turned on/off since its id does not exist.
15+
#
16+
# 2. Every Cppcheck id referenced by the premium addon's compliance
17+
# mapping tables (MISRA/CERT/AUTOSAR/CWE -> Cppcheck id, generated by
18+
# `coverage.py --id`) must also show up in `cppcheck --errorlist`.
19+
# Otherwise the mapping refers to an id that has been renamed or
20+
# removed.
21+
#
22+
# A warning is printed for every id that fails a check; exit status is 1
23+
# if any warning was printed, 0 otherwise.
24+
#
25+
# Usage:
26+
# tools/release-check-premium-ids.py [path-to-addon/coverage]
27+
#
28+
# The addon/coverage directory defaults to ../addon/coverage (relative to
29+
# this repo, i.e. a checkout of the premium addon repo next to this one).
30+
# If it can't be found, check 2 is skipped.
31+
32+
import argparse
33+
import glob
34+
import os
35+
import re
36+
import subprocess
37+
import sys
38+
39+
REPO_DIR = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
40+
CPPCHECK_BIN = os.path.join(REPO_DIR, 'cppcheck')
41+
42+
ID_RE = re.compile(r' id="([^"]*)"')
43+
IS_PREMIUM_ENABLED_RE = re.compile(r'isPremiumEnabled\s*\(\s*"([^"]*)"')
44+
45+
warning_count = 0
46+
47+
48+
def warn(message):
49+
global warning_count
50+
warning_count += 1
51+
print('warning: ' + message, file=sys.stderr)
52+
53+
54+
def get_errorlist_ids():
55+
out = subprocess.check_output([CPPCHECK_BIN, '--errorlist'], universal_newlines=True)
56+
return set(ID_RE.findall(out))
57+
58+
59+
def get_is_premium_enabled_ids():
60+
ids = set()
61+
for filename in glob.glob(os.path.join(REPO_DIR, 'lib', '*.cpp')) + glob.glob(os.path.join(REPO_DIR, 'lib', '*.h')):
62+
with open(filename, 'rt', encoding='utf-8') as f:
63+
ids.update(IS_PREMIUM_ENABLED_RE.findall(f.read()))
64+
return ids
65+
66+
67+
def get_addon_coverage_ids(addon_coverage_dir):
68+
coverage_py = os.path.join(addon_coverage_dir, 'coverage.py')
69+
if not os.path.isfile(coverage_py):
70+
print(f"(skipped premium addon coverage.py check: '{coverage_py}' not found)", file=sys.stderr)
71+
return None
72+
73+
ids_files = glob.glob(os.path.join(addon_coverage_dir, 'ids-*.txt'))
74+
for f in ids_files:
75+
os.remove(f)
76+
77+
env = dict(os.environ, CPPCHECK_REPO=REPO_DIR)
78+
subprocess.check_call(['python3', 'coverage.py', '--id'], cwd=addon_coverage_dir, env=env, stdout=subprocess.DEVNULL)
79+
80+
ids = set()
81+
ids_files = glob.glob(os.path.join(addon_coverage_dir, 'ids-*.txt'))
82+
for filename in ids_files:
83+
with open(filename, 'rt', encoding='utf-8') as f:
84+
ids.update(line.strip() for line in f if line.strip())
85+
os.remove(filename)
86+
return ids
87+
88+
89+
def main():
90+
parser = argparse.ArgumentParser(description='Check that isPremiumEnabled() ids and premium addon coverage ids are known Cppcheck error ids.')
91+
parser.add_argument('addon_coverage_dir', nargs='?', default=os.path.join(REPO_DIR, '..', 'addon', 'coverage'),
92+
help='path to the premium addon coverage/ directory (default: ../addon/coverage)')
93+
args = parser.parse_args()
94+
95+
if not os.access(CPPCHECK_BIN, os.X_OK):
96+
print(f"error: '{CPPCHECK_BIN}' not found or not executable, build it first (make -j$(nproc))", file=sys.stderr)
97+
return 1
98+
99+
errorlist_ids = get_errorlist_ids()
100+
101+
for id_ in sorted(get_is_premium_enabled_ids() - errorlist_ids):
102+
warn(f'isPremiumEnabled("{id_}") but there is no such id in --errorlist')
103+
104+
addon_coverage_dir = os.path.realpath(args.addon_coverage_dir)
105+
addon_ids = get_addon_coverage_ids(addon_coverage_dir)
106+
if addon_ids is not None:
107+
for id_ in sorted(addon_ids - errorlist_ids):
108+
warn(f'premium addon coverage.py references id "{id_}" but there is no such id in --errorlist')
109+
110+
return 1 if warning_count else 0
111+
112+
113+
if __name__ == '__main__':
114+
sys.exit(main())

0 commit comments

Comments
 (0)