Skip to content

Add special function to match lifetimes #5320

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chrchr-github merged 15 commits into from
Aug 14, 2023
Merged

Add special function to match lifetimes #5320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3c02291
Update matching for lifetime
pfultz2 Aug 12, 2023
6152ef8
Remove modified changes
pfultz2 Aug 12, 2023
08aac77
Rename match function
pfultz2 Aug 12, 2023
c48da15
Format
pfultz2 Aug 12, 2023
2cdd596
Update tests
pfultz2 Aug 12, 2023
b73d9cf
Update function
pfultz2 Aug 13, 2023
7e4ee12
Rename function
pfultz2 Aug 13, 2023
5826730
Format
pfultz2 Aug 13, 2023
eec9156
Merge branch 'main' into match-lifetime
pfultz2 Aug 13, 2023
b4610c0
Fix another unit test
pfultz2 Aug 13, 2023
d488451
Add comment
pfultz2 Aug 13, 2023
7b64167
Fix todo test
pfultz2 Aug 13, 2023
cfcc6aa
Remove else
pfultz2 Aug 13, 2023
a734360
Remove move
pfultz2 Aug 13, 2023
ca9d626
Format
pfultz2 Aug 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/checkuninitvar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,9 @@ bool CheckUninitVar::checkScopeForVariable(const Token *tok, const Variable& var
if (!suppressErrors && Token::Match(tok, "%name% . %name%") && tok->strAt(2) == membervar && Token::Match(tok->next()->astParent(), "%cop%|return|throw|?"))
uninitStructMemberError(tok, tok->str() + "." + membervar);
else if (mTokenizer->isCPP() && !suppressErrors && Token::Match(tok, "%name%") && Token::Match(tok->astParent(), "return|throw|?")) {
if (std::any_of(tok->values().cbegin(), tok->values().cend(), std::mem_fn(&ValueFlow::Value::isUninitValue)))
if (std::any_of(tok->values().cbegin(), tok->values().cend(), [](const ValueFlow::Value& v) {
return v.isUninitValue() && !v.isInconclusive();
}))
uninitStructMemberError(tok, tok->str() + "." + membervar);
}
}
Expand Down
43 changes: 29 additions & 14 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,9 +641,10 @@ static void setTokenValue(Token* tok,
}
}

if (Token::simpleMatch(parent, "=") && astIsRHS(tok) && !value.isLifetimeValue()) {
setTokenValue(parent, std::move(value), settings);
return;
if (Token::simpleMatch(parent, "=") && astIsRHS(tok)) {
setTokenValue(parent, value, settings);
if (!value.isUninitValue())
return;
}

if (value.isContainerSizeValue() && astIsContainer(tok)) {
Expand Down Expand Up @@ -2429,6 +2430,20 @@ struct ValueFlowAnalyzer : Analyzer {
return settings;
}

// Returns Action::Match if its an exact match, return Action::Read if it partially matches the lifetime
Action analyzeLifetime(const Token* tok) const
{
if (!tok)
return Action::None;
if (match(tok))
return Action::Match;
if (Token::simpleMatch(tok, ".") && analyzeLifetime(tok->astOperand1()) != Action::None)
return Action::Read;
if (astIsRHS(tok) && Token::simpleMatch(tok->astParent(), "."))
return analyzeLifetime(tok->astParent());
return Action::None;
}

struct ConditionState {
bool dependent = true;
bool unknown = true;
Expand Down Expand Up @@ -2802,7 +2817,10 @@ struct ValueFlowAnalyzer : Analyzer {
return Action::None;
lifeTok = v.tokvalue;
}
if (lifeTok && match(lifeTok)) {
if (!lifeTok)
return Action::None;
Action la = analyzeLifetime(lifeTok);
if (la.matches()) {
Action a = Action::Read;
if (isModified(tok).isModified())
a = Action::Invalid;
Expand All @@ -2812,6 +2830,9 @@ struct ValueFlowAnalyzer : Analyzer {
return Action::Inconclusive;
return a;
}
if (la.isRead()) {
return isAliasModified(tok);
}
return Action::None;

} else if (isAlias(ref, inconclusive)) {
Expand Down Expand Up @@ -3295,12 +3316,6 @@ struct MemberExpressionAnalyzer : SubExpressionAnalyzer {
: SubExpressionAnalyzer(e, std::move(val), t, s), varname(std::move(varname))
{}

bool match(const Token* tok) const override
{
return SubExpressionAnalyzer::match(tok) ||
(Token::simpleMatch(tok->astParent(), ".") && SubExpressionAnalyzer::match(tok->astParent()));
}

bool submatch(const Token* tok, bool exact) const override
{
if (!Token::Match(tok, ". %var%"))
Expand Down Expand Up @@ -3330,6 +3345,8 @@ static std::string lifetimeType(const Token *tok, const ValueFlow::Value *val)
case ValueFlow::Value::LifetimeKind::Address:
if (astIsPointer(tok))
result = "pointer";
else if (Token::simpleMatch(tok, "=") && astIsPointer(tok->astOperand2()))
result = "pointer";
else
result = "object";
break;
Expand Down Expand Up @@ -7973,7 +7990,6 @@ static void valueFlowUninit(TokenList& tokenlist, const Settings* settings)
Token* start = findStartToken(var, tok->next(), &settings->library);

std::map<Token*, ValueFlow::Value> partialReads;
Analyzer::Result result;
if (const Scope* scope = var->typeScope()) {
if (Token::findsimplematch(scope->bodyStart, "union", scope->bodyEnd))
continue;
Expand All @@ -7988,7 +8004,7 @@ static void valueFlowUninit(TokenList& tokenlist, const Settings* settings)
continue;
}
MemberExpressionAnalyzer analyzer(memVar.nameToken()->str(), tok, uninitValue, tokenlist, settings);
result = valueFlowGenericForward(start, tok->scope()->bodyEnd, analyzer, *settings);
valueFlowGenericForward(start, tok->scope()->bodyEnd, analyzer, *settings);

for (auto&& p : *analyzer.partialReads) {
Token* tok2 = p.first;
Expand Down Expand Up @@ -8018,8 +8034,7 @@ static void valueFlowUninit(TokenList& tokenlist, const Settings* settings)
if (partial)
continue;

if (result.terminate != Analyzer::Terminate::Modified)
valueFlowForward(start, tok->scope()->bodyEnd, var->nameToken(), uninitValue, tokenlist, settings);
valueFlowForward(start, tok->scope()->bodyEnd, var->nameToken(), uninitValue, tokenlist, settings);
}
}

Expand Down
9 changes: 4 additions & 5 deletions test/testuninitvar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6209,7 +6209,7 @@ class TestUninitVar : public TestFixture {
" memcpy(wcsin, x, sizeof(wcsstruct));\n" // <- warning
" x->wcsprm = NULL;\n" // <- no warning
"}");
ASSERT_EQUALS("[test.cpp:7]: (error) Uninitialized variable: x.wcsprm\n", errout.str());
ASSERT_EQUALS("[test.cpp:7]: (error) Uninitialized variable: x\n", errout.str());

valueFlowUninit("struct wcsstruct {\n"
" int *wcsprm;\n"
Expand Down Expand Up @@ -7037,8 +7037,7 @@ class TestUninitVar : public TestFixture {
" memcpy(in, s, sizeof(S));\n"
" s->p = NULL;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:4]: (error) Uninitialized variable: s.p\n",
errout.str());
ASSERT_EQUALS("[test.cpp:4]: (error) Uninitialized variable: s\n", errout.str());

valueFlowUninit("struct S {\n" // #11321
" int a = 0;\n"
Expand Down Expand Up @@ -7298,7 +7297,7 @@ class TestUninitVar : public TestFixture {
" A::B b;\n"
" x.push_back(b);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: b.i\n", errout.str());
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: b\n", errout.str());

valueFlowUninit("struct A {\n"
" struct B {\n"
Expand All @@ -7322,7 +7321,7 @@ class TestUninitVar : public TestFixture {
" A a;\n"
" x.push_back(a);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: a.j\n", errout.str());
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: a\n", errout.str());
Copy link
Copy Markdown
Collaborator

@danmar danmar Aug 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ideally it's better to write a member that is not initialized. It's something explicit that the user can investigate carefully.

Does this PR mean the analysis will be better? Less false negatives or false positives?

Copy link
Copy Markdown
Contributor Author

@pfultz2 pfultz2 Aug 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ideally it's better to write a member that is not initialized. It's something explicit that the user can investigate carefully.

We write the variables when it is partially initialized, but if all members are uninitialized we just write the struct.

Does this PR mean the analysis will be better?

Yes because we will track aliases better across all of our analyzers not just SubExpressionAnalyzer. This should lead to less FPs. And will help enable more FNs in the future as well.


valueFlowUninit("struct S { struct T { int* p; } t[2]; };\n" // #11018
"void f() {\n"
Expand Down