Page MenuHomePhabricator

[Clang] Fix an unused-but-set-variable warning with volatile variable
ClosedPublic

Authored by yonghong-song on Mar 15 2022, 9:45 AM.

Details

Summary

For the following code,

void test() {
    volatile int j = 0;
    for (int i = 0; i < 1000; i++)
            j += 1;
    return;
}

If compiled with

clang -g -Wall -Werror -S -emit-llvm test.c

we will see the following error:

test.c:2:6: error: variable 'j' set but not used [-Werror,-Wunused-but-set-variable]
        volatile int j = 0;
                         ^

This is not quite right since 'j' is indeed used due to '+=' operator.
gcc doesn't emit error either in this case.
Also if we change 'j += 1' to 'j++', the warning will disappear
with latest clang.

Note that clang will issue the warning if the volatile declaration
involves only simple assignment (var = ...).

To fix the issue, in function MaybeDecrementCount(), if the
operator is a compound assignment (i.e., +=, -=, etc.) and the
variable is volatile, the count for RefsMinusAssignments will be
decremented, similar to 'j++' case.

Diff Detail

Event Timeline

yonghong-song created this revision.Mar 15 2022, 9:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 9:45 AM
yonghong-song requested review of this revision.Mar 15 2022, 9:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 9:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@eli.friedman Please feel free adding other reviewers if needed. Thanks!

efriedma added inline comments.
clang/test/Sema/warn-unused-but-set-variables.c
80

The handling of this testcase without your patch seems fine. Even if the value of j is technically read, it's not "used" in any meaningful way.

I think the distinguishing factor for the testcase we discussed before is the use of a "volatile". Volatile is rare, and indicates the user is intentionally doing something unusual, so it probably doesn't make sense to warn.

yonghong-song retitled this revision from [Clang] Fix an unused-but-set-variable warning with compond assignment to [Clang] Fix an unused-but-set-variable warning with volatile variable.
yonghong-song edited the summary of this revision. (Show Details)
  • only handle volatile variables with component assignment.
yonghong-song added inline comments.Mar 15 2022, 7:24 PM
clang/test/Sema/warn-unused-but-set-variables.c
80

@eli.friedman Just updated the patch to handle volatile variable only.

efriedma accepted this revision.Mar 21 2022, 11:18 AM

LGTM

clang/lib/Sema/SemaDecl.cpp
2033 ↗(On Diff #415665)

(Accidental change?)

This revision is now accepted and ready to land.Mar 21 2022, 11:18 AM
aaron.ballman added inline comments.
clang/lib/Sema/SemaExprCXX.cpp
7928
clang/test/Sema/warn-unused-but-set-variables.c
35

I'd like to see another test:

typedef volatile int volint;
volint l = 0;
l += 1; // no warning

This is not quite right since 'j' is indeed used due to '+=' operator.

is “set” - load, increment, store - but not used after “store”. So technically this is okay no?

If there is j = j + 1 .. do we warn ?

void test() {
    int j = 0;
    for (int i = 0; i < 1000; i++)
            j++;
    return;
}

Should not we warn in this case? (Currently no warning)

If there is j = j + 1 .. do we warn ?

j++;

Should not we warn in this case? (Currently no warning)

We probably should warn in both those cases; the variable clearly isn't "used" in any meaningful way.

yonghong-song added inline comments.Mar 21 2022, 12:53 PM
clang/lib/Sema/SemaDecl.cpp
2033 ↗(On Diff #415665)

Right. Thanks for catching this. This is a leftover from my previous debugging code and forgot to remove it.

clang/lib/Sema/SemaExprCXX.cpp
7928

Thanks, really embarrassing typo.

clang/test/Sema/warn-unused-but-set-variables.c
35

Will do.

If there is j = j + 1 .. do we warn ?

j++;

Should not we warn in this case? (Currently no warning)

We probably should warn in both those cases; the variable clearly isn't "used" in any meaningful way.

The 'j++' is not warned as it is not a BinaryOperator or CXXOperatorCallExpr so it is not handled and considered no warning by default.
This patch intends to handle volatile variables. We can have another patch to deal with this specific case.

  • fix typo
  • add a new test case with typedef of volatile type.
efriedma accepted this revision.Mar 21 2022, 1:48 PM

The 'j++' is not warned as it is not a BinaryOperator or CXXOperatorCallExpr so it is not handled and considered no warning by default.
This patch intends to handle volatile variables. We can have another patch to deal with this specific case.

Right, I wasn't suggesting you should add that in this patch. Just a general observation about cases we should consider diagnosing in the future.