Skip to content

Partial fix for #15048: shiftNegative for conditional value #8884

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

Open
chrchr-github wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Partial fix for #15048: shiftNegative for conditional value #8884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
995ad05
Update testother.cpp
chrchr-github Sep 23, 2026
0314f16
Update checkother.h
chrchr-github Sep 23, 2026
acc143d
Update checkother.cpp
chrchr-github Sep 23, 2026
61f4327
Update checkother.cpp
chrchr-github Sep 23, 2026
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
40 changes: 23 additions & 17 deletions lib/checkother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3513,9 +3513,11 @@ void CheckOtherImpl::redundantCopyError(const Token *tok,const std::string& varn
// Checking for shift by negative values
//---------------------------------------------------------------------------

static bool isNegative(const Token *tok, const Settings &settings)
static const ValueFlow::Value* isNegative(const Token *tok, const Settings &settings)
{
return tok->valueType() && tok->valueType()->sign == ValueType::SIGNED && tok->getValueLE(-1LL, settings);
if (!tok->valueType() || tok->valueType()->sign != ValueType::SIGNED)
return nullptr;
return tok->getValueLE(-1LL, settings);
}

void CheckOtherImpl::checkNegativeBitwiseShift()
Expand Down Expand Up @@ -3551,24 +3553,28 @@ void CheckOtherImpl::checkNegativeBitwiseShift()
if (ternary)
continue;

// Get negative rhs value. preferably a value which doesn't have 'condition'.
if (portability && isNegative(tok->astOperand1(), mSettings))
negativeBitwiseShiftError(tok, 1);
else if (isNegative(tok->astOperand2(), mSettings))
negativeBitwiseShiftError(tok, 2);
const ValueFlow::Value* value = isNegative(tok->astOperand1(), mSettings); // lhs
if (portability && value)
negativeBitwiseShiftError(tok, true, value);
else {
value = isNegative(tok->astOperand2(), mSettings); // rhs
if (value)
negativeBitwiseShiftError(tok, false, value);
}
}
}


void CheckOtherImpl::negativeBitwiseShiftError(const Token *tok, int op)
void CheckOtherImpl::negativeBitwiseShiftError(const Token *tok, bool isLHS, const ValueFlow::Value* v)
{
if (op == 1)
// LHS - this is used by intention in various software, if it
// is used often in a project and works as expected then this is
// a portability issue
reportError(tok, Severity::portability, "shiftNegativeLHS", "Shifting a negative value is technically undefined behaviour", CWE758, Certainty::normal);
else // RHS
reportError(tok, Severity::error, "shiftNegative", "Shifting by a negative value is undefined behaviour", CWE758, Certainty::normal);
// LHS - this is used by intention in various software, if it
// is used often in a project and works as expected then this is
// a portability issue
const char* id = isLHS ? "shiftNegativeLHS" : "shiftNegative";
const std::string msg = isLHS ? "Shifting a negative value is technically undefined behaviour" : "Shifting by a negative value is undefined behaviour";
const Severity severity = isLHS ? Severity::portability : (v && v->errorSeverity() && !v->conditional ? Severity::error : Severity::warning);

@danmar danmar Oct 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is an AI review. Take it with a grain of salt and feel free to reject it by resolving the comment.

getErrorMessages() calls this with v == nullptr, so --errorlist now reports shiftNegative as severity="warning" instead of severity="error". I checked this with a build of the PR. Tools that use the error list (GUI, docs, severity mappings) would see the downgrade even though a known negative shift is still an error. Maybe treat a missing value as the non-conditional case:

Suggested change
const Severity severity = isLHS ? Severity::portability : (v && v->errorSeverity() && !v->conditional ? Severity::error : Severity::warning);
const Severity severity = isLHS ? Severity::portability : (!v || (v->errorSeverity() && !v->conditional) ? Severity::error : Severity::warning);

Apart from that, the new severity matches how zerodiv/zerodivcond handle the same patterns. I compared with int d = 0; if (b) return i; return i / d;.

const ErrorPath errorPath = getErrorPath(tok, v, msg);
reportError(errorPath, severity, id, msg, CWE758, Certainty::normal);
}

//---------------------------------------------------------------------------
Expand Down Expand Up @@ -4928,8 +4934,8 @@ void CheckOther::getErrorMessages(ErrorLogger& errorLogger, const Settings &sett
c.zerodivError(nullptr, nullptr);
c.misusedScopeObjectError(nullptr, "varname");
c.invalidPointerCastError(nullptr, "float *", "double *", false, false);
c.negativeBitwiseShiftError(nullptr, 1);
c.negativeBitwiseShiftError(nullptr, 2);
c.negativeBitwiseShiftError(nullptr, true);
c.negativeBitwiseShiftError(nullptr, false);
c.raceAfterInterlockedDecrementError(nullptr);
c.invalidFreeError(nullptr, "malloc", false);
c.overlappingWriteUnion(nullptr);
Expand Down
2 changes: 1 addition & 1 deletion lib/checkother.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ class CPPCHECKLIB CheckOtherImpl : public CheckImpl {
void unsignedPositiveError(const Token *tok, const ValueFlow::Value *v, const std::string &varname);
void pointerPositiveError(const Token *tok, const ValueFlow::Value *v);
void suspiciousSemicolonError(const Token *tok);
void negativeBitwiseShiftError(const Token *tok, int op);
void negativeBitwiseShiftError(const Token *tok, bool isLHS, const ValueFlow::Value *v = nullptr);
void redundantCopyError(const Token *tok, const std::string &varname);
void incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean);
void varFuncNullUBError(const Token *tok);
Expand Down
19 changes: 19 additions & 0 deletions test/testother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ class TestOther : public TestFixture {
TEST_CASE(checkRedundantCopy);

TEST_CASE(checkNegativeShift);
TEST_CASE(checkNegativeShiftErrorPath);

TEST_CASE(incompleteArrayFill);

Expand Down Expand Up @@ -10490,6 +10491,24 @@ class TestOther : public TestFixture {
ASSERT_EQUALS("", errout_str());
}

void checkNegativeShiftErrorPath() {
setMultiline();
Settings s = settings1;
s.templateLocation = "{file}:{line}:note:{info}\n";

check("int f(int i, bool b) {\n"
" int s = -1;\n"
" if (b)\n"
" return i;\n"
" return i << s;\n"
"}\n", dinit(CheckOptions, $.settings = &s));
ASSERT_EQUALS("[test.cpp:5:14]: warning: Shifting by a negative value is undefined behaviour [shiftNegative]\n"
"[test.cpp:2:14]: note: Assignment 's=-1', assigned value is -1\n"
"[test.cpp:3:9]: note: Assuming condition is false\n"
"[test.cpp:5:14]: note: Shifting by a negative value is undefined behaviour\n",
errout_str());
}

void incompleteArrayFill() {
check("void f() {\n"
" int a[5];\n"
Expand Down
Loading