From f1ddd1a3525025e58ac5fa590942c331e026569f Mon Sep 17 00:00:00 2001 From: olabetskyi <153490942+olabetskyi@users.noreply.github.com> Date: Fri, 7 Jun 2024 12:26:38 +0300 Subject: [PATCH] Fix #5115, #10093 (False positive: redundantAssignment when using a union) (#6484) --- lib/checkother.cpp | 11 ++++++- lib/fwdanalysis.cpp | 21 ++++++++++-- test/testother.cpp | 80 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 3 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 03e1aa7575e..815259f775c 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -510,8 +510,17 @@ void CheckOther::checkRedundantAssignment() else start = tok->findExpressionStartEndTokens().second->next(); + const Token * tokenToCheck = tok->astOperand1(); + + // Check if we are working with union + for (const Token* tempToken = tokenToCheck; Token::simpleMatch(tempToken, ".");) { + tempToken = tempToken->astOperand1(); + if (tempToken && tempToken->variable() && tempToken->variable()->type() && tempToken->variable()->type()->isUnionType()) + tokenToCheck = tempToken; + } + // Get next assignment.. - const Token *nextAssign = fwdAnalysis.reassign(tok->astOperand1(), start, scope->bodyEnd); + const Token *nextAssign = fwdAnalysis.reassign(tokenToCheck, start, scope->bodyEnd); if (!nextAssign) continue; diff --git a/lib/fwdanalysis.cpp b/lib/fwdanalysis.cpp index d735e6771e9..990cb173591 100644 --- a/lib/fwdanalysis.cpp +++ b/lib/fwdanalysis.cpp @@ -323,8 +323,25 @@ FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *expr, const Token * return Result(Result::Type::WRITE, parent->astParent()); return Result(Result::Type::READ); } - if (mWhat == What::Reassign && parent->valueType() && parent->valueType()->pointer && Token::Match(parent->astParent(), "%assign%") && parent == parent->astParent()->astOperand1()) - return Result(Result::Type::READ); + if (mWhat == What::Reassign) { + if (parent->variable() && parent->variable()->type() && parent->variable()->type()->isUnionType() && parent->varId() == expr->varId()) { + while (parent && Token::simpleMatch(parent->astParent(), ".")) + parent = parent->astParent(); + if (parent && parent->valueType() && Token::Match(parent->astParent(), "%assign%") && !Token::Match(parent->astParent()->astParent(), "%assign%") && parent->astParent()->astOperand1() == parent) { + const Token * assignment = parent->astParent()->astOperand2(); + while (Token::simpleMatch(assignment, ".") && assignment->varId() != expr->varId()) + assignment = assignment->astOperand1(); + if (assignment && assignment->varId() != expr->varId()) { + if (assignment->valueType() && assignment->valueType()->pointer) // Bailout + return Result(Result::Type::BAILOUT); + return Result(Result::Type::WRITE, parent->astParent()); + } + } + return Result(Result::Type::READ); + } + if (parent->valueType() && parent->valueType()->pointer && Token::Match(parent->astParent(), "%assign%")) + return Result(Result::Type::READ); + } if (Token::Match(parent->astParent(), "%assign%") && !parent->astParent()->astParent() && parent == parent->astParent()->astOperand1()) { if (mWhat == What::Reassign) diff --git a/test/testother.cpp b/test/testother.cpp index ad727af084f..462b32247fd 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -4472,6 +4472,86 @@ class TestOther : public TestFixture { " }\n" "}"); ASSERT_EQUALS("", errout_str()); + + // Ticket #5115 "redundantAssignment when using a union" + check("void main(void)\n" + "{\n" + " short lTotal = 0;\n" + " union\n" + " {\n" + " short l1;\n" + " struct\n" + " {\n" + " unsigned char b1;\n" + " unsigned char b2;\n" + " } b;\n" + " } u;\n" + " u.l1 = 1;\n" + " lTotal += u.b.b1;\n" + " u.l1 = 2;\n" //Should not show RedundantAssignment + "}", true, false, false); + ASSERT_EQUALS("", errout_str()); + + // Ticket #5115 "redundantAssignment when using a union" + check("void main(void)\n" + "{\n" + " short lTotal = 0;\n" + " union\n" + " {\n" + " short l1;\n" + " struct\n" + " {\n" + " unsigned char b1;\n" + " unsigned char b2;\n" + " } b;\n" + " } u;\n" + " u.l1 = 1;\n" + " u.l1 = 2;\n" + "}", true, false, false); + ASSERT_EQUALS("[test.cpp:13] -> [test.cpp:14]: (style) Variable 'u.l1' is reassigned a value before the old one has been used.\n", errout_str()); + + // Ticket #10093 "redundantAssignment when using a union" + check("typedef union fixed32_union {\n" + " struct {\n" + " unsigned32 abcd;\n" + " } u32;\n" + " struct {\n" + " unsigned16 ab;\n" + " unsigned16 cd;\n" + " } u16;" + " struct {\n" + " unsigned8 a;\n" + " unsigned8 b;\n" + " unsigned8 c;\n" + " unsigned8 d;\n" + " } b;\n" + "} fixed32;\n" + "void func1(void) {\n" + " fixed32 m;\n" + " m.u16.ab = 47;\n" + " m.u16.cd = 0;\n" + " m.u16.ab = m.u32.abcd / 53;\n" + "}", true, false, false); + ASSERT_EQUALS("", errout_str()); + + // Ticket #10093 "redundantAssignment when using a union" + check("#include \n" + "typedef union{\n" + " char as_char[4];\n" + " int as_int;\n" + "} union_t;\n" + "void fn(char *data, int len) {\n" + " int i;\n" + " for (i = 0; i < len; i++)\n" + " data[i] = 'a';\n" + "}\n" + "int main(int argc, char *argv[]) {\n" + " union_t u;\n" + " u.as_int = 42;\n" + " fn(&u.as_char[0], 4);\n" + " u.as_int = 0;\n" + "}", true, false, false); + ASSERT_EQUALS("", errout_str()); } void switchRedundantOperationTest() {