[Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[mbenfield]

Summary

These are intended to mimic warnings available in gcc.

[george.burgess.iv]

clang/lib/Sema/SemaStmt.cpp

437 ↗

(On Diff #337877)

It seems like these two warnings are doing analyses with identical requirements (e.g., the function's body must be present), but are taking two different sets of variables into account while doing so.

Is there a way we can rephrase this so we only need to walk the AST checking once for all of this?

[mbenfield]

clang/lib/Sema/SemaDecl.cpp

13794

Unfortunately the RecursiveASTVisitor's non-overridden member functions don't take const.

[xbolva00]

These warnings are not enabled by any other flags. This is different from gcc, where -Wunused-but-set-variable is enabled by -Wextra in combination with either -Wunused or -Wall.

IMHO we should follow gcc here.

[mbenfield]

Respond to comments.

• Use /// for function comment

• Improve the text of function comment

• Use SmallPtrSet instead of SmallDenseMap

• Use reference instead of pointer in AllUsesAreSetsVisitor

• Capture with [&]

• const in several places

• pragmas to ignore warnings in vector-gcc-compat.c

• in C++ test files, comment that we are following the lead of gcc in not warning for a struct

[mbenfield]

Capitalization and periods for comments.

[mbenfield]

clang/lib/Sema/SemaDecl.cpp

13805

Not possible here. TraverseStmt doesn't take a const parameter.

13867–13872

Isn't it essentially at the beginning of the function as-is? If the warning is disabled for all parameters, it'll return false right away for all of them. However, I will add an extra check to end as soon as possible once no candidates are found. Let me know if it isn't ideal.

13867–13872

I didn't modify this. I'm not sure there would be any advantage to checking if warnings are disabled for every parameter before just doing all the checks for all of them. Please push back if you think otherwise.

13904–13907

Ditto.

clang/lib/Sema/SemaStmt.cpp

437 ↗

(On Diff #337877)

I'll think about this. It might be tricky.

437 ↗

(On Diff #337877)

I'll implement this. It will be a fairly different approach though.

[mbenfield]

clang/lib/Sema/SemaStmt.cpp

437 ↗

(On Diff #337877)

I have this implemented. I am going to run a performance comparison on the two approaches before making a new revision here.

[aeubanks]

Running this over Chromium, I'm getting

$ cat /tmp/a.cc
struct A {
  int i;
  A(int i): i(i) {}
};

$ ./build/rel/bin/clang++ -fsyntax-only /tmp/a.cc -Wunused-but-set-parameter
/tmp/a.cc:3:9: warning: parameter 'i' set but not used [-Wunused-but-set-parameter]
  A(int i): i(i) {}
        ^
1 warning generated.

[aeubanks]

-Wunused-but-set-variable is firing on x at [1]

[1] https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/absl/synchronization/mutex.h;drc=07cb7b184515ad207d30f00d0b00b8ce96d0a750;l=947

../../third_party/abseil-cpp/absl/synchronization/mutex.h:947:6: error: variable 'x' set but not used [-Werror,-Wunused-but-set-variable]
  T *x = static_cast<T *>(c->arg_);

[mbenfield]

clang/lib/Sema/SemaStmt.cpp

437 ↗

(On Diff #337877)

An informal comparison reveals no significant performance difference between these two approaches. I think it's a tradeoff: if you walk each block and/or each function separately, you do have to do more walks, but you're likely to be able to terminate your walk early (because you already ran into a use of each variable/parameter).

OTOH, if you walk everything in one go, it is only one walk, but you definitely have to walk the whole thing (because you might run into another CompoundStmt with more declarations).

I find the current approach conceptually simpler and so would prefer to keep it as-is. If you disagree let me know.

[mbenfield]

In D100581#2693131, @xbolva00 wrote:

These warnings are not enabled by any other flags. This is different from gcc, where -Wunused-but-set-variable is enabled by -Wextra in combination with either -Wunused or -Wall.

IMHO we should follow gcc here.

I'd be happy to do so, but there are two issues:

1. I'm not sure this is feasible and fits in with how Clang's diagnostics are organized. AFAICT clang's diagnostics are not set up to have a diagnostic enabled only if two other flags are set. If I'm wrong please let me know.

2. In gcc, this is how -Wunused-parameter behaves, but clang's -Wunused-parameter is already different. In clang, it's enabled by -Wextra regardless of -Wall or -Wunused.

[mbenfield]

Fixed misfirings reported by aeubanks and added tests for those cases.

[george.burgess.iv]

Just a few more nits and LGTM. We probably want the thoughts of someone with ownership in warnings to be sure. +rtrieu might be good?

clang/lib/Sema/SemaDecl.cpp

13794

Yeah, I was thinking of StmtVisitor's interface -- my mistake

13806

since we try to exit early, should this be

if (!TraverseStmt(LHS))
  return false;

(and similar below)?

13814

nit: LLVM doesn't like superfluous parens; please write this as return !S.erase(DRE->getFoundDecl()) || S.size();

13867–13872

Ah, I was misreading a bit; didn't realize that we were using the SourceLoc of each individual parameter to feed into getDiagnosticLevel. Yeah, this is fine as-is.

13879

nit: LLVM generally tries to write

auto *Foo = bar();
if (Foo)
  use(Foo);

as

if (auto *Foo = bar())
  use(Foo)

in part because the latter makes a bit better use of scopes

clang/test/Sema/vector-gcc-compat.c

1

nit: is it possible to add -Wno-unused-but-set-parameter & -Wno-unused-but-set-variable as a part of the RUN line?

if it becomes too long, you can use \ to wrap it:

// RUN: foo bar \
// RUN:     baz

[xbolva00]

In D100581#2697697, @mbenfield wrote:

In D100581#2693131, @xbolva00 wrote:

These warnings are not enabled by any other flags. This is different from gcc, where -Wunused-but-set-variable is enabled by -Wextra in combination with either -Wunused or -Wall.

IMHO we should follow gcc here.

I'd be happy to do so, but there are two issues:

1. I'm not sure this is feasible and fits in with how Clang's diagnostics are organized. AFAICT clang's diagnostics are not set up to have a diagnostic enabled only if two other flags are set. If I'm wrong please let me know.

2. In gcc, this is how -Wunused-parameter behaves, but clang's -Wunused-parameter is already different. In clang, it's enabled by -Wextra regardless of -Wall or -Wunused.

I am a little bit worried that another off by default warning is not ideal from user point of view. Either the user simply would fail to find out that there is a new option or will be surprised that gcc fires and clang does not even when we claim we implemented this “gcc’s” warning.

About your points, @rsmith may help you.

[aeubanks]

another false positive:

../../third_party/abseil-cpp/absl/strings/internal/str_format/float_conversion.cc:879:20: error: variable 'kBufSizeForHexFloatRepr' set but not used [-Werror,-Wunused-but-set-variable]
  constexpr size_t kBufSizeForHexFloatRepr =

https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/absl/strings/internal/str_format/float_conversion.cc;drc=dcb2703f69f9d66a848106b97f6c932a1d02f84e;l=879

[mbenfield]

In D100581#2699412, @xbolva00 wrote:

I am a little bit worried that another off by default warning is not ideal from user point of view. Either the user simply would fail to find out that there is a new option or will be surprised that gcc fires and clang does not even when we claim we implemented this “gcc’s” warning.

Understood. How about putting -Wunused-but-set-parameter in -Wextra (which is what clang does for -Wunused-parameter) and -Wunused-but-set-variable in -Wunused (which is what gcc does)? I don't know why the parameter ones go in -Wextra and the variable ones go in -Wunused, but this seems most consistent with currrent diagnostic groups.

[xbolva00]

So.... I checked gcc on godbolt since gcc docs are not so clear.

UnusedButSetParameter is ignored with:

• -Wunused alone
• -Wextra alone (wtf)
• -Wall -Wunused

But works with -Wextra -Wunused or -Wall -Wextra.

So I think we should put UnusedButSetParameter under -Wextra

def Extra : DiagGroup<"extra", [
    DeprecatedCopy,
    MissingFieldInitializers,
    IgnoredQualifiers,
    InitializerOverrides,
    SemiBeforeMethodBody,
    MissingMethodReturnType,
    SignCompare,
    UnusedButSetParameter
    UnusedParameter,
    NullPointerArithmetic,
    EmptyInitStatement,
    StringConcatation,
    FUseLdPath,
  ]>;

UnusedButSetVariable is part of -Wall and -Wunused (gcc's -Wunused is enabled by -Wall.)

So I would just put UnusedButSetVariable under -Wunused (and -Wunused is part of -Wall):

def Unused : DiagGroup<"unused",
                       [UnusedArgument, UnusedButSetVariable,
                        UnusedFunction, UnusedLabel,
                        // UnusedParameter, (matches GCC's behavior)
                        // UnusedTemplate, (clean-up libc++ before enabling)
                        // UnusedMemberFunction, (clean-up llvm before enabling)
                        UnusedPrivateField, UnusedLambdaCapture,
                        UnusedLocalTypedef, UnusedValue, UnusedVariable,
                        UnusedPropertyIvar]>,
                        DiagCategory<"Unused Entity Issue">;

WDYT?

clang/test/Sema/warn-unused-but-set-variables-cpp.cpp

1 ↗

(On Diff #338423)

I would suggest move it to clang/test/SemaCXX/warn-unused-but-set-variables.cpp

[mbenfield]

Response to review comments.

• Diagnostic groups: UnusedButSetVariable into Unused and UnusedButSetParameter into Extra

• Early exit from VisitBinaryOperator

• Idiomatic if (Decl *D = ...)

• Add -Wno-unused-but-set-parameter and -Wno-unused-but-set-variable to many tests (necessary since these were added to diagnostic groups)

• Don't flag for const or constexpr variables (fixes the false positive reported by aeubanks)

• Test case for const and constexpr variables

• Move C++ test cases to SemaCXX

[mbenfield]

In D100581#2700083, @xbolva00 wrote:

So I think we should put UnusedButSetParameter under -Wextra

So I would just put UnusedButSetVariable under -Wunused (and -Wunused is part of -Wall):

WDYT?

Sounds good; I've done this now.

[xbolva00]

Thanks, looks really good.

Meanwhile I collected some interesting testcases from gcc bugzilla (reported as false positives), please try:
https://godbolt.org/z/747ndKEvd - No "unused-but-set" warnings expected.

[dblaikie]

FWIW, I'd love it if we could do a full dead-store warning, which would be a superset of this. I think we have enough infrastructure in the analysis based warnings (I think the sufficiency of the infrastructure is demonstrated by the "may be used uninitialized" warnings). Such a warning would subsume these narrower "set but not used" type of warnings (though would require the analysis warning infrastructure).

[xbolva00]

In D100581#2706467, @dblaikie wrote:

FWIW, I'd love it if we could do a full dead-store warning, which would be a superset of this. I think we have enough infrastructure in the analysis based warnings (I think the sufficiency of the infrastructure is demonstrated by the "may be used uninitialized" warnings). Such a warning would subsume these narrower "set but not used" type of warnings (though would require the analysis warning infrastructure).

• Compile time cost could be a problem.
• Do we need stronger dead store warning? Clang analyzer checks for dead stores, no?

I would rather have this on by default with -Wall, than something stronger off by default

[dblaikie]

In D100581#2706554, @xbolva00 wrote:

In D100581#2706467, @dblaikie wrote:

FWIW, I'd love it if we could do a full dead-store warning, which would be a superset of this. I think we have enough infrastructure in the analysis based warnings (I think the sufficiency of the infrastructure is demonstrated by the "may be used uninitialized" warnings). Such a warning would subsume these narrower "set but not used" type of warnings (though would require the analysis warning infrastructure).

• Compile time cost could be a problem.

Given that we managed to enable -Wsometimes-uninitialized, which is a CFG-sensitive warning under -Wall:

$ clang++-tot maybe-uninit.cpp -Wall -c
maybe-uninit.cpp:4:7: warning: variable 'i' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
  if (b)
      ^
maybe-uninit.cpp:6:6: note: uninitialized use occurs here
  f1(i);
     ^
maybe-uninit.cpp:4:3: note: remove the 'if' if its condition is always true
  if (b)
  ^~~~~~
maybe-uninit.cpp:3:8: note: initialize the variable 'i' to silence this warning
  int i;
       ^
        = 0
1 warning generated.

Then I think similar infrastructure could be used to implement a dead-store warning too.

• Do we need stronger dead store warning? Clang analyzer checks for dead stores, no?

Catching something at compile time's a fair bit of increased value over using the static analyzer (at least at Google we don't use the static analyzer, for instance)

Oh, right - https://bugs.llvm.org/show_bug.cgi?id=24506 documents one of the cases that came up that I think a dead store warning would be rather helpful at catching. (though I think we probably now catch the int case -Wzero-as-null-pointer-constant - though that's not on by default, for instance because it's too pervasive, but a dead store warning would catch the buggy case without require a stylistic change to a whole codebase - and also a dead store warning could catch the case with, say a T** parameter where you mean to set the caller's value to nullptr, but instead set the local copy to nullptr) - in these simple examples unused-but-set catches them, but it's not uncommon to test a parameter for non-null before writing, for instance you might mean "if (x) *x = nullptr" but write "if (x) x = nullptr" - dead store would catch that but unused-but-set would not.

[mbenfield]

In D100581#2705780, @xbolva00 wrote:

Meanwhile I collected some interesting testcases from gcc bugzilla (reported as false positives), please try:
https://godbolt.org/z/747ndKEvd - No "unused-but-set" warnings expected.

FWIW, neither of these "unused-but-set" warnings were triggered on any of that code (although some other warnings were).

[mbenfield]

Fix test warning-wall.c.

[aeubanks]

Looks good in terms of false positives on Chrome. This did uncover a lot of issues, although many of them are due to only using some variable in some config with #ifdef

[xbolva00]

I think this is good to go. Later anybody can build improvements on top of this patch and maybe work on some more powerful “dead stores” warnings.

[mbenfield]

Great. If it's been approved, I am not able to commit, so I think someone else will have to do that.

[aeubanks]

I can land this for you, but could you update the description to be a bit more descriptive? Explaining what the warnings do.

[mbenfield]

In D100581#2717182, @aeubanks wrote:

I can land this for you, but could you update the description to be a bit more descriptive? Explaining what the warnings do.

Are you looking at the updated message in the commit, or the old summary displayed in Phabricator? It seems Phabricator doesn't change the summary when I update the commit description. Let me know whether this is what you thought, or if the current commit message is not adequate. The current commit message is

[Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

These are intended to mimic warnings available in gcc.

-Wunused-but-set-variable is triggered in the case of a variable which
appears on the LHS of an assignment but not otherwise used.

For instance:

void f() {
int x;
x = 0;
}

-Wunused-but-set-parameter works similarly, but for function parameters
instead of variables.

In C++, they are triggered only for scalar types; otherwise, they are
triggered for all types. This is gcc's behavior.

-Wunused-but-set-parameter is controlled by -Wextra, while
-Wunused-but-set-variable is controlled by -Wunused. This is slightly
different from gcc's behavior, but seems most consistent with clang's
behavior for -Wunused-parameter and -Wunused-variable.

Differential Revision: https://reviews.llvm.org/D100581

[aeubanks]

yeah you have to manually go to "Edit Revision" and update the message.

I'll just amend it locally though

[uabelho]

I noticed that with this patch
libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp
fails:

error: 'warning' diagnostics seen but not expected: 
  File /repo/uabelho/master-github/libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp Line 51: variable 's' set but not used
1 error generated.

(I don't know anything about the testcase I just noticed it failed when trying to uplift my downstream repo.)

[Abpostelnicu]

I think this added a regression, where you have this context:

uint32_t LocalAccessible::SelectedItemCount() {
  uint32_t count = 0;
  AccIterator iter(this, filters::GetSelected);
  LocalAccessible* selected = nullptr;
  while ((selected = iter.Next())) ++count;

  return count;
}

[task 2021-04-27T02:39:42.523Z] 02:39:42    ERROR -  /builds/worker/checkouts/gecko/accessible/generic/LocalAccessible.cpp:2455:20: error: variable 'selected' set but not used [-Werror,-Wunused-but-set-variable]
[task 2021-04-27T02:39:42.523Z] 02:39:42     INFO -    LocalAccessible* selected = nullptr;
[task 2021-04-27T02:39:42.523Z] 02:39:42     INFO -                     ^
[task 2021-04-27T02:39:42.523Z] 02:39:42     INFO -  1 error generated.

The file in cause is this one.

Indeed the value stored in selected is not used but this check is a little bit too strict.

[aeubanks]

In D100581#2718840, @uabelho wrote:

I noticed that with this patch
libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp
fails:

error: 'warning' diagnostics seen but not expected: 
  File /repo/uabelho/master-github/libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp Line 51: variable 's' set but not used
1 error generated.

(I don't know anything about the testcase I just noticed it failed when trying to uplift my downstream repo.)

looks like this was fixed in 7f98209da6349891b7a74eeadace34d38e7aaadc

[aeubanks]

In D100581#2718947, @Abpostelnicu wrote:

I think this added a regression, where you have this context:

uint32_t LocalAccessible::SelectedItemCount() {
  uint32_t count = 0;
  AccIterator iter(this, filters::GetSelected);
  LocalAccessible* selected = nullptr;
  while ((selected = iter.Next())) ++count;

  return count;
}

[task 2021-04-27T02:39:42.523Z] 02:39:42    ERROR -  /builds/worker/checkouts/gecko/accessible/generic/LocalAccessible.cpp:2455:20: error: variable 'selected' set but not used [-Werror,-Wunused-but-set-variable]
[task 2021-04-27T02:39:42.523Z] 02:39:42     INFO -    LocalAccessible* selected = nullptr;
[task 2021-04-27T02:39:42.523Z] 02:39:42     INFO -                     ^
[task 2021-04-27T02:39:42.523Z] 02:39:42     INFO -  1 error generated.

The file in cause is this one.

Indeed the value stored in selected is not used but this check is a little bit too strict.

That seems like a real finding to me, deleting selected should fix it and clean up the code.

[eugenis]

I fixed another one in https://reviews.llvm.org/rG561f4b9087457e9ea3cf4aeb57dcd507e2fa6258
Please watch the bots after you land changes like this one and don't let them stay broken for an entire day!

[aeubanks]

Sorry about that, although I don't think emails went to me since the email was somebody else's. But point taken, I'll coordinate better in the future when landing someone else's patch.

[eugenis]

Right, our commit process is a bit strange. Ideally, the change author should initiate commit after they have the necessary approvals, and then be responsible to deal with any fallout in a timely manner. As things are, the author does not control the timing of the commit, and the committer does not get the bot messages.

[nickdesaulniers]

I think this is triggering some false positives in builds of the Linux kernel.

[nickdesaulniers]

Huh, maybe not: https://godbolt.org/z/PnE1fMGWo

[dbabokin]

One more false-positive:

void foo() {
  extern int yydebug;
  yydebug = 1;
}

It was triggered in ISPC build.

[mbenfield]

In D100581#2721705, @dbabokin wrote:

One more false-positive:

void foo() {
  extern int yydebug;
  yydebug = 1;
}

It was triggered in ISPC build.

Thanks for the report. It seems the thing to do is to modify the warning so that variables declared extern are not candidates for this warning. I will make a new commit with that modification unless there are other suggestions.

[dbabokin]

Thanks for the report. It seems the thing to do is to modify the warning so that variables marked extern are not candidates for this warning. I will make a new commit with that modification unless there are other suggestions.

I agree, this seems to be the right fix. Thanks!

[nathanchance]

In D100581#2721430, @nickdesaulniers wrote:

Huh, maybe not: https://godbolt.org/z/PnE1fMGWo

This difference has caused some confusion already for the Linux kernel:

https://lore.kernel.org/r/[e-mail]

https://elixir.bootlin.com/linux/v5.12/source/drivers/pinctrl/intel/pinctrl-intel.c#L1746

https://elixir.bootlin.com/linux/v5.12/source/include/linux/gpio/driver.h#L485

I am not sure it is appropriate to warn there.

[lebedev.ri]

In D100581#2723329, @nathanchance wrote:

In D100581#2721430, @nickdesaulniers wrote:

Huh, maybe not: https://godbolt.org/z/PnE1fMGWo

This difference has caused some confusion already for the Linux kernel:

It's quite obviously used, since it's the condition variable for the loop.
Is clang CFG not modelling that correctly?

[lebedev.ri]

Also, from the reviewer list, it doesn't look like anyone with much familiarity with clang/clang diags actually participated in the review?
I think this might be the point to revert and re-review.

[mbenfield]

In D100581#2723348, @lebedev.ri wrote:

Also, from the reviewer list, it doesn't look like anyone with much familiarity with clang/clang diags actually participated in the review?
I think this might be the point to revert and re-review.

Alright, someone with the capability please revert if that's the correct action.

[Abpostelnicu]

Please revert this. This change hasn’t been tested nor reviewed enough. Also I don’t remember seeing this proposed on cfe dev mailing list.

[mbenfield]

This comment was removed by mbenfield.

[lebedev.ri]

In D100581#2723505, @Abpostelnicu wrote:

Also I don’t remember seeing this proposed on cfe dev mailing list.

There is no such requirement. I don't recall that happening basically ever, actually.

[Abpostelnicu]

In D100581#2723611, @lebedev.ri wrote:

In D100581#2723505, @Abpostelnicu wrote:

Also I don’t remember seeing this proposed on cfe dev mailing list.

There is no such requirement. I don't recall that happening basically ever, actually.

Of course, but I would have wanted to see this discussed on the mailing list since the pool of the affected actors by this is very big.

[lebedev.ri]

In D100581#2723621, @Abpostelnicu wrote:

In D100581#2723611, @lebedev.ri wrote:

In D100581#2723505, @Abpostelnicu wrote:

Also I don’t remember seeing this proposed on cfe dev mailing list.

There is no such requirement. I don't recall that happening basically ever, actually.

Of course, but I would have wanted to see this discussed on the mailing list since the pool of the affected actors by this is very big.

If we subtract the false-positives, i'm not really sure what should be discussed.

[aeubanks]

reverted
regarding having a reviewer who is knowledgable in clang diagnostics, I assumed that george.burgess.iv was knowledgable and was happy with the change after his comments, perhaps I should have waited for an explicit LGTM

[lebedev.ri]

In D100581#2723709, @aeubanks wrote:

reverted
regarding having a reviewer who is knowledgable in clang diagnostics, I assumed that george.burgess.iv was knowledgable and was happy with the change after his comments, perhaps I should have waited for an explicit LGTM

It's fine, there are always bugs everywhere, won't all be caught.

[mbenfield]

So it seems there are 3 issues here:

1. local variables declared extern should not trigger this warning. I'll fix this.

2. Should x in this code trigger the warning:

int x;
int y = (x = 0);

These are essentially the "false positives" reported in the Linux kernel. I find it questionable to consider these "false positives"; it seems to me that x is not really used. But gcc's behavior is to not trigger here so maybe following it is best.

3. If x above should not trigger the warning, it might be difficult to detect this case with an AST walk using the current data. Would it be acceptable to add a bit to VarDecl or Decl indicating whether it's ever read (not just referenced), which could then be set, maybe in Sema::ActOnFinishFullExpr ? This would then remove the need for the new AST walk altogether. (it seems gcc has such a bit)

[nickdesaulniers]

In D100581#2723789, @mbenfield wrote:

But gcc's behavior is to not trigger here so maybe following it is best.

Perhaps file a bug against GCC to find whether that was intentional or a bug?

[xbolva00]

In D100581#2723794, @nickdesaulniers wrote:

In D100581#2723789, @mbenfield wrote:

But gcc's behavior is to not trigger here so maybe following it is best.

Perhaps file a bug against GCC to find whether that was intentional or a bug?

Yes, the best solution. Also consider to check gcc’s test-suite - if they have this type of testcase, then this is wanted behaviour.

[rsmith]

When this change returns, I'd like to see a different implementation strategy. Running a recursive AST visitation after the fact is generally not the right way to look for this kind of issue; adding an extra pass to speculatively hunt for each kind of warning we might want to issue is too expensive. Instead, I think we should do something simpler and cheaper, such as tracking, for each variable in scope, the number of potentially-evaluated non-assignment references to that variable. For example:

• in DoMarkVarDeclReferenced, increment a per-variable counter if the variable is a local and the reference is an odr-use
• in IgnoredValueConversions, if we're discarding a potentially-evaluated assignment or increment expression acting on a local variable, decrement the counter for that variable
• in ActOnPopScope, or perhaps in DiagnoseUnusedDecl, when we're diagnosing unused variables, also diagnose set-but-not-used variables (where the variable is marked as used but the counter is equal to zero)

[mbenfield]

In D100581#2723801, @xbolva00 wrote:

Yes, the best solution. Also consider to check gcc’s test-suite - if they have this type of testcase, then this is wanted behaviour.

gcc has the test Wunused-var-5.c, where the warning -Wunused-but-set-variable is applied but

void bar (int, ...);
void f18 (void) { _Atomic int x = 0; int y = 3; bar (x = y); }

is not expected to trigger the warning, which does seem to imply this is intentional.

[mbenfield]

Another try at these warnings, using the implementation strategy outlined by rsmith.

A couple other notes:

• At the moment I've removed these warnings from the diagnostic groups -Wextra and -Wunused. It was suggested by aeubanks that the groups could be added in a later commit, once these warnings have been determined to not be too disruptive. Of course I can change this now if requested.

• I've just tried to mimic gcc's behavior as closely as possible, including not warning if an assignment is used, and not warning on nonscalar types in C++ (except that in some cases gcc inexplicably does warn on nonscalar types; test on the file vector-gcc-compat.c to compare... I haven't determined any rationale to when it chooses to warn in these cases).

[dblaikie]

In D100581#2727330, @mbenfield wrote:

Another try at these warnings, using the implementation strategy outlined by rsmith.

A couple other notes:

• At the moment I've removed these warnings from the diagnostic groups -Wextra and -Wunused. It was suggested by aeubanks that the groups could be added in a later commit, once these warnings have been determined to not be too disruptive. Of course I can change this now if requested.

• I've just tried to mimic gcc's behavior as closely as possible, including not warning if an assignment is used, and not warning on nonscalar types in C++ (except that in some cases gcc inexplicably does warn on nonscalar types; test on the file vector-gcc-compat.c to compare... I haven't determined any rationale to when it chooses to warn in these cases).

Got a link/examples of cases GCC does and doesn't warn about? I'd assume it'd have something to do with the triviality or non-triviality of certain operations of the nonscalar types (eg: is the type trivially assignable/trivially destructible)

[xbolva00]

I've removed these warnings from the diagnostic groups -Wextra

But to remove all false positives, you need reports from users of -Wextra. Otherwise this would be dead code with no users.

I suggest keeping it in groups. Now you can ask folks to try this patch with kernel, Firefox, Chrome, XYZ and of course LLVM. Also you can try gcc’s testcases for this warning. If it is OK, I see no reasons to remove it from those groups.

[nickdesaulniers]

I'm happy to retest with a few Linux kernel builds...tomorrow!

[mbenfield]

In D100581#2727357, @dblaikie wrote:

Got a link/examples of cases GCC does and doesn't warn about? I'd assume it'd have something to do with the triviality or non-triviality of certain operations of the nonscalar types (eg: is the type trivially assignable/trivially destructible)

This doesn't seem to be what determines it. AFAICT it never warns for a struct in C++. However, if I do

gcc -fsyntax-only -Wunused-but-set-variable clang/test/Sema/vector-gcc-compat.c

I get warnings for the variables v2i64_r and v4i32_r.

But if I do

g++ -fsyntax-only -Wunused-but-set-variable clang/test/Sema/vector-gcc-compat.c

I only get warnings for the variable v2i64_r.

I will investigate a little more tomorrow.

[mbenfield]

Will also make a new revision tomorrow with these warnings added to the appropriate groups.

[aeubanks]

regarding adding these to a warning group in a separate change, it's easier to revert a smaller second change that adds this to warning groups, and easier to iterate upon in tree when there are discovered issues
it's not dead code if it has tests

[xbolva00]

In D100581#2727460, @aeubanks wrote:

regarding adding these to a warning group in a separate change, it's easier to revert a smaller second change that adds this to warning groups, and easier to iterate upon in tree when there are discovered issues
it's not dead code if it has tests

I meant dead code in terms of users if second patch are not gonna come anytime soon after first one :)
You can always commit this patch as two - first with new warning, second - add to groups.

[dblaikie]

In D100581#2727432, @mbenfield wrote:

In D100581#2727357, @dblaikie wrote:

Got a link/examples of cases GCC does and doesn't warn about? I'd assume it'd have something to do with the triviality or non-triviality of certain operations of the nonscalar types (eg: is the type trivially assignable/trivially destructible)

This doesn't seem to be what determines it. AFAICT it never warns for a struct in C++. However, if I do

gcc -fsyntax-only -Wunused-but-set-variable clang/test/Sema/vector-gcc-compat.c

I get warnings for the variables v2i64_r and v4i32_r.

But if I do

g++ -fsyntax-only -Wunused-but-set-variable clang/test/Sema/vector-gcc-compat.c

I only get warnings for the variable v2i64_r.

I will investigate a little more tomorrow.

Ah, might have a size threshold?

[mbenfield]

A second commit with UnusedButSetParameter and UnusedButSetVariable into groups.

[nickdesaulniers]

clang/include/clang/Basic/DiagnosticGroups.td

876

llvm-project/clang/include/clang/Basic/DiagnosticGroups.td:874:25: error: Variable not defined: 'UnusedButSetVariable'

UnusedButSetVariable, UnusedPropertyIvar]>,
^

[mbenfield]

Apparently arc and/or Phabricator doesn't like my attempt to split this review into two separate commits. Next revision will have only one commit.

[mbenfield]

Correct struct and vector behavior.

gcc doesn't warn for any structs in C++. However, it _does_ warn for vector types. Those warnings are sometimes masked by other errors, leading to my earlier belief that it is inconsistent about warning for vector types.

[nickdesaulniers]

I see lots of instances from the kernel that look like this when reduced:

$ cat foo.c
int page;
int put_page_testzero(int);
void foo (void) {
  int zeroed;
  zeroed = put_page_testzero(page);
  ((void)(sizeof(( long)(!zeroed))));
}
$ clang -c -Wall foo.c
foo.c:4:7: warning: variable 'zeroed' set but not used [-Wunused-but-set-variable]
  int zeroed;
      ^

[mbenfield]

Count a non-ODR use as a reference, so that examples like that of nickdesaulniers don't warn.

[dblaikie]

In D100581#2730557, @nickdesaulniers wrote:

I see lots of instances from the kernel that look like this when reduced:

$ cat foo.c
int page;
int put_page_testzero(int);
void foo (void) {
  int zeroed;
  zeroed = put_page_testzero(page);
  ((void)(sizeof(( long)(!zeroed))));
}
$ clang -c -Wall foo.c
foo.c:4:7: warning: variable 'zeroed' set but not used [-Wunused-but-set-variable]
  int zeroed;
      ^

Any idea what the purpose of this code is/why it's not a reasonable thing to warn on?

[nickdesaulniers]

The additions of -Wno-unused-but-set-variable to:

• clang/test/SemaObjC/foreach.m
• clang/test/SemaCXX/sizeless-1.cpp
• clang/test/SemaCXX/shift.cpp
• clang/test/SemaCXX/goto.cpp
• clang/test/Sema/vector-gcc-compat.cpp
• clang/test/Sema/vector-gcc-compat.c
• clang/test/FixIt/fixit.cpp
• clang/test/CodeGen/builtins-arm.c
• clang/test/CodeGen/builtins-riscv.c
• clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp

...
all the one line changes to tests that add this warning flag to the run line look questionable to me; we generally don't add new warning flags to a ton of existing tests. Were those cases that were previously failing? Were those accidentally committed? I would have expected this change to be:

• changes to enable the flag and do something
• block of additional test cases
• changes to existing tests if they were failing this new check inadvertantly.

But adding a new warning flags to a handful of existing tests' RUN lines is unusual.

[nickdesaulniers]

Testing Diff 342071 on the mainline Linux kernel, just building x86_64 defconfig triggers 19 instances of this warning; all look legit. ;)

In D100581#2730708, @dblaikie wrote:

In D100581#2730557, @nickdesaulniers wrote:

I see lots of instances from the kernel that look like this when reduced:

$ cat foo.c
int page;
int put_page_testzero(int);
void foo (void) {
  int zeroed;
  zeroed = put_page_testzero(page);
  ((void)(sizeof(( long)(!zeroed))));
}
$ clang -c -Wall foo.c
foo.c:4:7: warning: variable 'zeroed' set but not used [-Wunused-but-set-variable]
  int zeroed;
      ^

Any idea what the purpose of this code is/why it's not a reasonable thing to warn on?

include/linux/build_bug.h:

25 /*                                                                                                                                                                          
26  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the                                                                                                 
27  * expression but avoids the generation of any code, even if that expression                                                                                                
28  * has side-effects.                                                                                                                                                        
29  */                                                                                                                                                                         
30 #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))

include/linux/mmdebug.h:

57 #define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)

mm/internal.h:

410 static inline unsigned long                                                                                                                                                 
411 vma_address(struct page *page, struct vm_area_struct *vma)                                                                                                                  
412 {                                                                                                                                                                           
413   unsigned long start, end;                                                                                                                                                 
414                                                                                                                                                                             
415   start = __vma_address(page, vma);                                                                                                                                         
416   end = start + thp_size(page) - PAGE_SIZE;                                                                                                                                 
417                                                                                                                                                                             
418   /* page should be within @vma mapping range */                                                                                                                            
419   VM_BUG_ON_VMA(end < vma->vm_start || start >= vma->vm_end, vma);

The warning (in previous revisions was triggering on end in vma_address.

[mbenfield]

In D100581#2730557, @nickdesaulniers wrote:

I see lots of instances from the kernel that look like this when reduced:

But adding a new warning flags to a handful of existing tests' RUN lines is unusual.

These tests use -Wall for whatever reason, and they assign but don't use several variables, so we'll get warnings on them unless I add -Wno-unused-but-set-variable. Other alternatives:

1. add an expected-warning;

2. use a pragma to disable the warning;

3. put an unused attribute on the variables in question;

4. introduce an artificial use of these variables.

I previously used item 1, but somebody earlier in an earlier comment requested that instead I add -Wno-unused-but-set-variable instead.

[nickdesaulniers]

In D100581#2730760, @mbenfield wrote:

In D100581#2730557, @nickdesaulniers wrote:

I see lots of instances from the kernel that look like this when reduced:

But adding a new warning flags to a handful of existing tests' RUN lines is unusual.

These tests use -Wall for whatever reason, and they assign but don't use several variables, so we'll get warnings on them unless I add -Wno-unused-but-set-variable. Other alternatives:

1. add an expected-warning;

2. use a pragma to disable the warning;

3. put an unused attribute on the variables in question;

4. introduce an artificial use of these variables.

I previously used item 1, but somebody earlier in an earlier comment requested that instead I add -Wno-unused-but-set-variable instead.

Ah, I missed that they were _disabling_ the warning; I was thinking they were _enabling_ it. Sorry for the noise; LGTM.

[dblaikie]

In D100581#2730743, @nickdesaulniers wrote:

Testing Diff 342071 on the mainline Linux kernel, just building x86_64 defconfig triggers 19 instances of this warning; all look legit. ;)

In D100581#2730708, @dblaikie wrote:

In D100581#2730557, @nickdesaulniers wrote:

I see lots of instances from the kernel that look like this when reduced:

$ cat foo.c
int page;
int put_page_testzero(int);
void foo (void) {
  int zeroed;
  zeroed = put_page_testzero(page);
  ((void)(sizeof(( long)(!zeroed))));
}
$ clang -c -Wall foo.c
foo.c:4:7: warning: variable 'zeroed' set but not used [-Wunused-but-set-variable]
  int zeroed;
      ^

Any idea what the purpose of this code is/why it's not a reasonable thing to warn on?

include/linux/build_bug.h:

25 /*                                                                                                                                                                          
26  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the                                                                                                 
27  * expression but avoids the generation of any code, even if that expression                                                                                                
28  * has side-effects.                                                                                                                                                        
29  */                                                                                                                                                                         
30 #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))

include/linux/mmdebug.h:

57 #define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)

mm/internal.h:

410 static inline unsigned long                                                                                                                                                 
411 vma_address(struct page *page, struct vm_area_struct *vma)                                                                                                                  
412 {                                                                                                                                                                           
413   unsigned long start, end;                                                                                                                                                 
414                                                                                                                                                                             
415   start = __vma_address(page, vma);                                                                                                                                         
416   end = start + thp_size(page) - PAGE_SIZE;                                                                                                                                 
417                                                                                                                                                                             
418   /* page should be within @vma mapping range */                                                                                                                            
419   VM_BUG_ON_VMA(end < vma->vm_start || start >= vma->vm_end, vma);

The warning (in previous revisions was triggering on end in vma_address.

Ah, fair enough. I guess this means the warning has the same false positive with a value that's initialized unconditional, but then read only in an assert (& thus unread in a non-asserts build)? As much as we're used to accepting that as a limitation of assert, I can see how that'd be good to avoid for new warnings to reduce false positives/code that would require modifications despite there being no bug in it.

[aeubanks]

casting to void is the normal way to get around these warnings, these new warnings should also not diagnose anything with a cast to void

[mbenfield]

FWIW gcc does not warn in this case regardless of any cast:

void f() {
  int x;
  x = 0;
  sizeof(x);
}

[aeubanks]

If you'd like you can split this into separate changes, one for adding the warning and another for adding it into warning groups, either way is fine.
Seems like the reported false positives have been addressed.

[pcc]

This warning seems to have a lot of false positives on things like reference arguments that are used as output parameters. For example here is a small sample of output from a stage2 build of part of LLVM:

In file included from ../llvm/lib/BinaryFormat/Minidump.cpp:9:
In file included from ../llvm/include/llvm/BinaryFormat/Minidump.h:21:
In file included from ../llvm/include/llvm/ADT/BitmaskEnum.h:16:
../llvm/include/llvm/Support/MathExtras.h:822:9: warning: variable 'Overflowed' set but not used [-Wunused-but-set-variable]
  bool &Overflowed = ResultOverflowed ? *ResultOverflowed : Dummy;
        ^
../llvm/include/llvm/Support/MathExtras.h:936:72: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
std::enable_if_t<std::is_signed<T>::value, T> MulOverflow(T X, T Y, T &Result) {
                                                                       ^
In file included from ../llvm/lib/BinaryFormat/Minidump.cpp:9:
In file included from ../llvm/include/llvm/BinaryFormat/Minidump.h:22:
In file included from ../llvm/include/llvm/ADT/DenseMapInfo.h:20:
../llvm/include/llvm/ADT/StringRef.h:511:37: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
    getAsInteger(unsigned Radix, T &Result) const {
                                    ^
../llvm/include/llvm/ADT/StringRef.h:522:37: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
    getAsInteger(unsigned Radix, T &Result) const {
                                    ^
../llvm/include/llvm/ADT/StringRef.h:545:39: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
    consumeInteger(unsigned Radix, T &Result) {
                                      ^
../llvm/include/llvm/ADT/StringRef.h:556:39: warning: parameter 'Result' set but not used [-Wunused-but-set-parameter]
    consumeInteger(unsigned Radix, T &Result) {
                                      ^
6 warnings generated.

Could you please take a look?

[mbenfield]

Don't warn for reference or dependent types (fixing false positives).

Also fix a few places where this warning is correctly triggered. In a couple
places I did that by adding attribute((unused)) rather than removing the
unused variable.

[xbolva00]

Also fix a few places where this warning is correctly triggered.

Create new patch please - dont mix fixes with new warning within one patch.

[mbenfield]

Move fixes into a separate change https://reviews.llvm.org/D102942.

[lebedev.ri]

Removing from queue - i don't expect to review this.
Looks like this has been reverted twice now, presumably llvm stage 2/linux kernel/chrome
should be enough of a coverage to be sure there isn't any obvious false-positives this time.

[Abpostelnicu]

I think there is a false positive with this @george.burgess.iv:
In this we have the warning triggered:
mozglue/baseprofiler/core/platform-linux-android.cpp:216:9: error: variable 'r' set but not used [-Werror,-Wunused-but-set-variable]

SigHandlerCoordinator() {
  PodZero(&mUContext);
  int r = sem_init(&mMessage2, /* pshared */ 0, 0);
  r |= sem_init(&mMessage3, /* pshared */ 0, 0);
  r |= sem_init(&mMessage4, /* pshared */ 0, 0);
  MOZ_ASSERT(r == 0);
}

[xbolva00]

In D100581#2792854, @Abpostelnicu wrote:

I think there is a false positive with this @george.burgess.iv:
In this we have the warning triggered:
mozglue/baseprofiler/core/platform-linux-android.cpp:216:9: error: variable 'r' set but not used [-Werror,-Wunused-but-set-variable]

SigHandlerCoordinator() {
  PodZero(&mUContext);
  int r = sem_init(&mMessage2, /* pshared */ 0, 0);
  r |= sem_init(&mMessage3, /* pshared */ 0, 0);
  r |= sem_init(&mMessage4, /* pshared */ 0, 0);
  MOZ_ASSERT(r == 0);
}

If MOZ_ASSERT is expanded to nothing, than the warning is correct. (void)r; trick should work here..

[ldionne]

Hello! There are still some false positives, for example this one is breaking the libc++ CI: https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a8848

Would it be possible to either fix this quickly or revert the change until the issue has been fixed? Our pre-commit CI is going to be stalled until this is fixed. Thanks!

[dblaikie]

In D100581#2793775, @ldionne wrote:

Hello! There are still some false positives, for example this one is breaking the libc++ CI: https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a8848

Would it be possible to either fix this quickly or revert the change until the issue has been fixed? Our pre-commit CI is going to be stalled until this is fixed. Thanks!

Looks like a true positive in libc++ ( https://github.com/llvm/llvm-project/blob/main/libcxx/include/regex ) - the "j" variable is initialized, then incremented, but never read (except to increment). Is that a bug in libc++? Was j meant to be used for something?

[ldionne]

In D100581#2793926, @dblaikie wrote:

In D100581#2793775, @ldionne wrote:

Hello! There are still some false positives, for example this one is breaking the libc++ CI: https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a8848

Would it be possible to either fix this quickly or revert the change until the issue has been fixed? Our pre-commit CI is going to be stalled until this is fixed. Thanks!

Looks like a true positive in libc++ ( https://github.com/llvm/llvm-project/blob/main/libcxx/include/regex ) - the "j" variable is initialized, then incremented, but never read (except to increment). Is that a bug in libc++? Was j meant to be used for something?

Oh, you're right! I was tripped by the fact that we did in fact "use" __j (in the sense that we do __j += ...), but indeed we never read the result. I'll work on fixing that issue. I'm not sure whether it's a bug or not yet, that code was modified 11 years ago, but I'll do my research.

[mbenfield]

In D100581#2792854, @Abpostelnicu wrote:

I think there is a false positive with this @george.burgess.iv:
In this we have the warning triggered:
mozglue/baseprofiler/core/platform-linux-android.cpp:216:9: error: variable 'r' set but not used [-Werror,-Wunused-but-set-variable]

SigHandlerCoordinator() {
  PodZero(&mUContext);
  int r = sem_init(&mMessage2, /* pshared */ 0, 0);
  r |= sem_init(&mMessage3, /* pshared */ 0, 0);
  r |= sem_init(&mMessage4, /* pshared */ 0, 0);
  MOZ_ASSERT(r == 0);
}

Indeed as pointed out above I think this is a true positive unfortunately.

[sberg]

Is it intentional that this warns about volatile variables as in

void f(char * p) {
    volatile char c = 0;
    c ^= *p;
}

(I see that GCC warns about volatile too, at least when you replace the ^= with =, so assume the answer is "yes", but would just like to see that confirmed (ideally with a test case even?).)

[mbenfield]

In D100581#2795966, @sberg wrote:

Is it intentional that this warns about volatile variables as in

Yes, volatile was intentional. Alright, I will add a test for this case.

[raj.khem]

http://sprunge.us/FJzZXL is a file from harfbuzz and it warns

a.cc:28670:32: error: variable 'supp_size' set but not used [-Werror,-Wunused-but-set-variable]
    unsigned int size0, size1, supp_size = 0;

I do not have -Werror enabled but it still is reported as error. There is no way to disable this warning ?

[xbolva00]

In D100581#2798079, @raj.khem wrote:

http://sprunge.us/FJzZXL is a file from harfbuzz and it warns

a.cc:28670:32: error: variable 'supp_size' set but not used [-Werror,-Wunused-but-set-variable]
    unsigned int size0, size1, supp_size = 0;

I do not have -Werror enabled but it still is reported as error. There is no way to disable this warning ?

Well, you have

#pragma GCC diagnostic error "-Wunused"

[Abpostelnicu]

In D100581#2798079, @raj.khem wrote:

http://sprunge.us/FJzZXL is a file from harfbuzz and it warns

a.cc:28670:32: error: variable 'supp_size' set but not used [-Werror,-Wunused-but-set-variable]
    unsigned int size0, size1, supp_size = 0;

You have, with pragma gcc error, look at hb.hh. The problem is that clang has an error dealing with cascading pragma when setting different levels.

I do not have -Werror enabled but it still is reported as error. There is no way to disable this warning ?

Go to the hb GitHub repo, I’ve fixed this in master branch, it will propagate soon to a new release.

[xbolva00]

This comment was removed by xbolva00.

[xbolva00]

This comment was removed by xbolva00.

clang/include/clang/Basic/DiagnosticGroups.td

 def UnusedPrivateField : DiagGroup<"unused-private-field">;
 def UnusedFunction : DiagGroup<"unused-function", [UnneededInternalDecl]>;
 def UnusedTemplate : DiagGroup<"unused-template", [UnneededInternalDecl]>;
 def UnusedMemberFunction : DiagGroup<"unused-member-function",
                                      [UnneededMemberFunction]>;
 def UnusedLabel : DiagGroup<"unused-label">;
 def UnusedLambdaCapture : DiagGroup<"unused-lambda-capture">;
 def UnusedParameter : DiagGroup<"unused-parameter">;
+def UnusedButSetParameter : DiagGroup<"unused-but-set-parameter">;
 def UnusedResult : DiagGroup<"unused-result">;
 def PotentiallyEvaluatedExpression : DiagGroup<"potentially-evaluated-expression">;
 def UnevaluatedExpression : DiagGroup<"unevaluated-expression",
                                       [PotentiallyEvaluatedExpression]>;
 def UnusedValue : DiagGroup<"unused-value", [UnusedComparison, UnusedResult,
                                              UnevaluatedExpression]>;
 def UnusedConstVariable : DiagGroup<"unused-const-variable">;
 def UnusedVariable : DiagGroup<"unused-variable",
                                [UnusedConstVariable]>;
+def UnusedButSetVariable : DiagGroup<"unused-but-set-variable">;
 def UnusedLocalTypedef : DiagGroup<"unused-local-typedef">;
 def UnusedPropertyIvar :  DiagGroup<"unused-property-ivar">;
 def UnusedGetterReturnValue : DiagGroup<"unused-getter-return-value">;
 def UsedButMarkedUnused : DiagGroup<"used-but-marked-unused">;
 def UserDefinedLiterals : DiagGroup<"user-defined-literals">;
 def UserDefinedWarnings : DiagGroup<"user-defined-warnings">;
 def ReorderCtor : DiagGroup<"reorder-ctor">;
 def ReorderInitList : DiagGroup<"reorder-init-list">;

 
 def Unused : DiagGroup<"unused",
                        [UnusedArgument, UnusedFunction, UnusedLabel,
                         // UnusedParameter, (matches GCC's behavior)
                         // UnusedTemplate, (clean-up libc++ before enabling)
                         // UnusedMemberFunction, (clean-up llvm before enabling)
                         UnusedPrivateField, UnusedLambdaCapture,
                         UnusedLocalTypedef, UnusedValue, UnusedVariable,
-                        UnusedPropertyIvar]>,
+                        UnusedButSetVariable, UnusedPropertyIvar]>,
                         DiagCategory<"Unused Entity Issue">;
 
 // Format settings.
 def FormatInvalidSpecifier : DiagGroup<"format-invalid-specifier">;
 def FormatSecurity : DiagGroup<"format-security">;
 def FormatNonStandard : DiagGroup<"format-non-iso">;
 def FormatY2K : DiagGroup<"format-y2k">;
 def FormatPedantic : DiagGroup<"format-pedantic">;
def Extra : DiagGroup<"extra", [
     DeprecatedCopy,
     MissingFieldInitializers,
     IgnoredQualifiers,
     InitializerOverrides,
     SemiBeforeMethodBody,
     MissingMethodReturnType,
     SignCompare,
     UnusedParameter,
+    UnusedButSetParameter,
     NullPointerArithmetic,
     EmptyInitStatement,
     StringConcatation,
     FUseLdPath,
   ]>;
 
 def Most : DiagGroup<"most", [
     CharSubscript,

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_interrupt_attribute_invalid : Warning<
    "functions that have %select{no parameters|a 'void' return type}1">,
    InGroup<IgnoredAttributes>;
 def warn_riscv_repeated_interrupt_attribute : Warning<
   "repeated RISC-V 'interrupt' attribute">, InGroup<IgnoredAttributes>;
 def note_riscv_repeated_interrupt_attribute : Note<
   "repeated RISC-V 'interrupt' attribute is here">;
 def warn_unused_parameter : Warning<"unused parameter %0">,
   InGroup<UnusedParameter>, DefaultIgnore;
+def warn_unused_but_set_parameter : Warning<"parameter %0 set but not used">,
+  InGroup<UnusedButSetParameter>, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,
   InGroup<UnusedVariable>, DefaultIgnore;
+def warn_unused_but_set_variable : Warning<"variable %0 set but not used">,
+  InGroup<UnusedButSetVariable>, DefaultIgnore;
 def warn_unused_local_typedef : Warning<
   "unused %select{typedef|type alias}0 %1">,
   InGroup<UnusedLocalTypedef>, DefaultIgnore;
 def warn_unused_property_backing_ivar :
   Warning<"ivar %0 which backs the property is not "
   "referenced in this property's accessor">,
   InGroup<UnusedPropertyIvar>, DefaultIgnore;
 def warn_unused_const_variable : Warning<"unused variable %0">,

clang/include/clang/Sema/Sema.h

private:
     FPOptions OldFPFeaturesState;
     FPOptionsOverride OldOverrides;
   };
 
   void addImplicitTypedef(StringRef Name, QualType T);
 
   bool WarnedStackExhausted = false;
 
+  /// Increment when we find a reference; decrement when we find an ignored
+  /// assignment.  Ultimately the value is 0 if every reference is an ignored
+  /// assignment.
+  llvm::DenseMap<const VarDecl *, int> RefsMinusAssignments;
+
 public:
   Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
        TranslationUnitKind TUKind = TU_Complete,
        CodeCompleteConsumer *CompletionConsumer = nullptr);
   ~Sema();
 
   /// Perform initialization that occurs after the parser has been
   /// initialized but before it parses anything.
public:
   void MarkUnusedFileScopedDecl(const DeclaratorDecl *D);
 
   /// DiagnoseUnusedExprResult - If the statement passed in is an expression
   /// whose result is unused, warn.
   void DiagnoseUnusedExprResult(const Stmt *S);
   void DiagnoseUnusedNestedTypedefs(const RecordDecl *D);
   void DiagnoseUnusedDecl(const NamedDecl *ND);
 
+  /// If VD is set but not otherwise used, diagnose, for a parameter or a
+  /// variable.
+  void DiagnoseUnusedButSetDecl(const VarDecl *VD);
+
   /// Emit \p DiagID if statement located on \p StmtLoc has a suspicious null
   /// statement as a \p Body, and it is located on the same line.
   ///
   /// This helps prevent bugs due to typos, such as:
   ///     if (condition);
   ///       do_stuff();
   void DiagnoseEmptyStmtBody(SourceLocation StmtLoc,
                              const Stmt *Body,

clang/lib/Sema/SemaDecl.cpp

void Sema::DiagnoseUnusedDecl(const NamedDecl *D) {
   else if (isa<LabelDecl>(D))
     DiagID = diag::warn_unused_label;
   else
     DiagID = diag::warn_unused_variable;
 
   Diag(D->getLocation(), DiagID) << D << Hint;
 }
 
+void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD) {
+  // If it's not referenced, it can't be set.
+  if (!VD->isReferenced() || !VD->getDeclName() || VD->hasAttr<UnusedAttr>())
+    return;
+
+  const auto *Ty = VD->getType().getTypePtr()->getBaseElementTypeUnsafe();
+  if (const TagType *TT = Ty->getAs<TagType>()) {
+    const TagDecl *Tag = TT->getDecl();
+    if (Tag->hasAttr<UnusedAttr>())
+      return;
+    // In C++, don't warn for record types that don't have WarnUnusedAttr, to
+    // mimic gcc's behavior.
+    if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(Tag)) {
+      if (!RD->hasAttr<WarnUnusedAttr>())
+        return;
+    }
+  }
+
+  auto iter = RefsMinusAssignments.find(VD);
+  if (iter == RefsMinusAssignments.end())
+    return;
+
+  assert(iter->getSecond() >= 0 &&
+         "Found a negative number of references to a VarDecl");
+  if (iter->getSecond() != 0)
+    return;
+  unsigned DiagID = isa<ParmVarDecl>(VD) ? diag::warn_unused_but_set_parameter
+                                         : diag::warn_unused_but_set_variable;
+  Diag(VD->getLocation(), DiagID) << VD;
+}
+
 static void CheckPoppedLabel(LabelDecl *L, Sema &S) {
   // Verify that we have no forward references left.  If so, there was a goto
   // or address of a label taken, but no definition of it.  Label fwd
   // definitions are indicated with a null substmt which is also not a resolved
   // MS inline assembly label name.
   bool Diagnose = false;
   if (L->isMSAsmLabel())
     Diagnose = !L->isResolvedMSAsmLabel();
for (auto *TmpD : S->decls()) {
 
     // Diagnose unused variables in this scope.
     if (!S->hasUnrecoverableErrorOccurred()) {
       DiagnoseUnusedDecl(D);
       if (const auto *RD = dyn_cast<RecordDecl>(D))
         DiagnoseUnusedNestedTypedefs(RD);
     }
 
+    if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
+      DiagnoseUnusedButSetDecl(VD);
+      RefsMinusAssignments.erase(VD);
+    }
+
     if (!D->getDeclName()) continue;
 
     // If this was a forward reference to a label, verify it was defined.
     if (LabelDecl *LD = dyn_cast<LabelDecl>(D))
       CheckPoppedLabel(LD, *this);
 
     // Remove this name from our lexical scope, and warn on it if we haven't
     // already.
for (const ParmVarDecl *Parameter : Parameters) {
     if (!Parameter->isReferenced() && Parameter->getDeclName() &&
         !Parameter->hasAttr<UnusedAttr>()) {
       Diag(Parameter->getLocation(), diag::warn_unused_parameter)
         << Parameter->getDeclName();
     }
   }
 }
 
 void Sema::DiagnoseSizeOfParametersAndReturnValue(
     ArrayRef<ParmVarDecl *> Parameters, QualType ReturnTy, NamedDecl *D) {
   if (LangOpts.NumLargeByValueCopy == 0) // No check.
     return;
 
   // Warn if the return value is pass-by-value and larger than the specified
   // threshold.
   if (!ReturnTy->isDependentType() && ReturnTy.isPODType(Context)) {
     unsigned Size = Context.getTypeSizeInChars(ReturnTy).getQuantity();
     if (Size > LangOpts.NumLargeByValueCopy)
       Diag(D->getLocation(), diag::warn_return_value_size) << D << Size;
   }
 
   // Warn if any parameter is pass-by-value and larger than the specified
   // threshold.
   for (const ParmVarDecl *Parameter : Parameters) {
     QualType T = Parameter->getType();
     if (T->isDependentType() || !T.isPODType(Context))
       continue;
     unsigned Size = Context.getTypeSizeInChars(T).getQuantity();
     if (Size > LangOpts.NumLargeByValueCopy)
       Diag(Parameter->getLocation(), diag::warn_parameter_size)
           << Parameter << Size;
   }
 }
 
 ParmVarDecl *Sema::CheckParameter(DeclContext *DC, SourceLocation StartLoc,
                                   SourceLocation NameLoc, IdentifierInfo *Name,
                                   QualType T, TypeSourceInfo *TSInfo,
                                   StorageClass SC) {
   // In ARC, infer a lifetime qualifier for appropriate parameter types.
   if (getLangOpts().ObjCAutoRefCount &&
       T.getObjCLifetime() == Qualifiers::OCL_None &&
       T->isObjCLifetimeType()) {
 
     Qualifiers::ObjCLifetime lifetime;
 
     // Special cases for arrays:
     //   - if it's const, use __unsafe_unretained
     //   - otherwise, it's an error
     if (T->isArrayType()) {
       if (!T.isConstQualified()) {
if (auto *LSI = getEnclosingLambda())
       LSI->LocalPacks.push_back(New);
 
   if (New->getType().hasNonTrivialToPrimitiveDestructCUnion() ||
       New->getType().hasNonTrivialToPrimitiveCopyCUnion())
     checkNonTrivialCUnion(New->getType(), New->getLocation(),
                           NTCUC_FunctionParam, NTCUK_Destruct|NTCUK_Copy);
 
   // Parameters can not be abstract class types.
   // For record types, this is done by the AbstractClassUsageDiagnoser once
   // the class has been completely parsed.
   if (!CurContext->isRecord() &&
       RequireNonAbstractType(NameLoc, T, diag::err_abstract_type_in_decl,
                              AbstractParamType))
     New->setInvalidDecl();
 
   // Parameter declarators cannot be interface types. All ObjC objects are
   // passed by reference.
   if (T->isObjCObjectType()) {
     SourceLocation TypeEndLoc =
         getLocForEndOfToken(TSInfo->getTypeLoc().getEndLoc());
     Diag(NameLoc,
          diag::err_object_cannot_be_passed_returned_by_value) << 1 << T
       << FixItHint::CreateInsertion(TypeEndLoc, "*");
     T = Context.getObjCObjectPointerType(T);
     New->setType(T);
   }
 
   // ISO/IEC TR 18037 S6.7.3: "The type of an object with automatic storage
   // duration shall not be qualified by an address-space qualifier."
   // Since all parameters have automatic store duration, they can not have
   // an address space.
   if (T.getAddressSpace() != LangAS::Default &&
       // OpenCL allows function arguments declared to be an array of a type
       // to be qualified with an address space.
       !(getLangOpts().OpenCL &&
         (T->isArrayType() || T.getAddressSpace() == LangAS::opencl_private))) {
     Diag(NameLoc, diag::err_arg_with_address_space);
     New->setInvalidDecl();
   }
 
   // PPC MMA non-pointer types are not allowed as function argument types.
   if (Context.getTargetInfo().getTriple().isPPC64() &&
       CheckPPCMMAType(New->getOriginalType(), New->getLocation())) {
     New->setInvalidDecl();
   }
 
   return New;
 }
 
 void Sema::ActOnFinishKNRParamDeclarations(Scope *S, Declarator &D,
                                            SourceLocation LocAfterDecls) {
   DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
 
   // Verify 6.9.1p6: 'every identifier in the identifier list shall be declared'
   // for a K&R function.
   if (!FTI.hasPrototype) {
     for (int i = FTI.NumParams; i != 0; /* decrement in loop */) {
       --i;

clang/lib/Sema/SemaExpr.cpp

if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
       llvm_unreachable("Unexpected expression");
     }
   }
 
   assert(MaybeODRUseExprs.empty() &&
          "MarkVarDeclODRUsed failed to cleanup MaybeODRUseExprs?");
 }
 
-static void DoMarkVarDeclReferenced(Sema &SemaRef, SourceLocation Loc,
-                                    VarDecl *Var, Expr *E) {
+static void DoMarkVarDeclReferenced(
+    Sema &SemaRef, SourceLocation Loc, VarDecl *Var, Expr *E,
+    llvm::DenseMap<const VarDecl *, int> &RefsMinusAssignments) {
   assert((!E || isa<DeclRefExpr>(E) || isa<MemberExpr>(E) ||
           isa<FunctionParmPackExpr>(E)) &&
          "Invalid Expr argument to DoMarkVarDeclReferenced");
   Var->setReferenced();
 
   if (Var->isInvalidDecl())
     return;
 
static void DoMarkVarDeclReferenced(
   auto *MSI = Var->getMemberSpecializationInfo();
   TemplateSpecializationKind TSK = MSI ? MSI->getTemplateSpecializationKind()
                                        : Var->getTemplateSpecializationKind();
 
   OdrUseContext OdrUse = isOdrUseContext(SemaRef);
   bool UsableInConstantExpr =
       Var->mightBeUsableInConstantExpressions(SemaRef.Context);
 
+  if (Var->isLocalVarDeclOrParm() && !Var->hasExternalStorage()) {
+    RefsMinusAssignments.insert({Var, 0}).first->getSecond()++;
+  }
+
   // C++20 [expr.const]p12:
   //   A variable [...] is needed for constant evaluation if it is [...] a
   //   variable whose name appears as a potentially constant evaluated
   //   expression that is either a contexpr variable or is of non-volatile
   //   const-qualified integral type or of reference type
   bool NeededForConstantEvaluation =
       isPotentiallyConstantEvaluatedContext(SemaRef) && UsableInConstantExpr;
 
case OdrUseContext::Dependent:
     break;
   }
 }
 
 /// Mark a variable referenced, and check whether it is odr-used
 /// (C++ [basic.def.odr]p2, C99 6.9p3).  Note that this should not be
 /// used directly for normal expressions referring to VarDecl.
 void Sema::MarkVariableReferenced(SourceLocation Loc, VarDecl *Var) {
-  DoMarkVarDeclReferenced(*this, Loc, Var, nullptr);
+  DoMarkVarDeclReferenced(*this, Loc, Var, nullptr, RefsMinusAssignments);
 }
 
-static void MarkExprReferenced(Sema &SemaRef, SourceLocation Loc,
-                               Decl *D, Expr *E, bool MightBeOdrUse) {
+static void
+MarkExprReferenced(Sema &SemaRef, SourceLocation Loc, Decl *D, Expr *E,
+                   bool MightBeOdrUse,
+                   llvm::DenseMap<const VarDecl *, int> &RefsMinusAssignments) {
   if (SemaRef.isInOpenMPDeclareTargetContext())
     SemaRef.checkDeclIsAllowedInOpenMPTarget(E, D);
 
   if (VarDecl *Var = dyn_cast<VarDecl>(D)) {
-    DoMarkVarDeclReferenced(SemaRef, Loc, Var, E);
+    DoMarkVarDeclReferenced(SemaRef, Loc, Var, E, RefsMinusAssignments);
     return;
   }
 
   SemaRef.MarkAnyDeclReferenced(Loc, D, MightBeOdrUse);
 
   // If this is a call to a method via a cast, also mark the method in the
   // derived class used in case codegen can devirtualize the call.
   const MemberExpr *ME = dyn_cast<MemberExpr>(E);
if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(E->getDecl()))
     if (Method->isVirtual() &&
         !Method->getDevirtualizedMethod(Base, getLangOpts().AppleKext))
       OdrUse = false;
 
   if (auto *FD = dyn_cast<FunctionDecl>(E->getDecl()))
     if (!isConstantEvaluated() && FD->isConsteval() &&
         !RebuildingImmediateInvocation)
       ExprEvalContexts.back().ReferenceToConsteval.insert(E);
-  MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse);
+  MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse,
+                     RefsMinusAssignments);
 }
 
 /// Perform reference-marking and odr-use handling for a MemberExpr.
 void Sema::MarkMemberReferenced(MemberExpr *E) {
   // C++11 [basic.def.odr]p2:
   //   A non-overloaded function whose name appears as a potentially-evaluated
   //   expression or a member of a set of candidate functions, if selected by
   //   overload resolution when referred to from a potentially-evaluated
   //   expression, is odr-used, unless it is a pure virtual function and its
   //   name is not explicitly qualified.
   bool MightBeOdrUse = true;
   if (E->performsVirtualDispatch(getLangOpts())) {
     if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(E->getMemberDecl()))
       if (Method->isPure())
         MightBeOdrUse = false;
   }
   SourceLocation Loc =
       E->getMemberLoc().isValid() ? E->getMemberLoc() : E->getBeginLoc();
-  MarkExprReferenced(*this, Loc, E->getMemberDecl(), E, MightBeOdrUse);
+  MarkExprReferenced(*this, Loc, E->getMemberDecl(), E, MightBeOdrUse,
+                     RefsMinusAssignments);
 }
 
 /// Perform reference-marking and odr-use handling for a FunctionParmPackExpr.
 void Sema::MarkFunctionParmPackReferenced(FunctionParmPackExpr *E) {
   for (VarDecl *VD : *E)
-    MarkExprReferenced(*this, E->getParameterPackLocation(), VD, E, true);
+    MarkExprReferenced(*this, E->getParameterPackLocation(), VD, E, true,
+                       RefsMinusAssignments);
 }
 
 /// Perform marking for a reference to an arbitrary declaration.  It
 /// marks the declaration referenced, and performs odr-use checking for
 /// functions and variables. This method should not be used when building a
 /// normal expression which refers to a variable.
 void Sema::MarkAnyDeclReferenced(SourceLocation Loc, Decl *D,
                                  bool MightBeOdrUse) {

clang/lib/Sema/SemaExprCXX.cpp

return new (Context)
       CXXNoexceptExpr(Context.BoolTy, Operand, CanThrow, KeyLoc, RParen);
 }
 
 ExprResult Sema::ActOnNoexceptExpr(SourceLocation KeyLoc, SourceLocation,
                                    Expr *Operand, SourceLocation RParen) {
   return BuildCXXNoexceptExpr(KeyLoc, Operand, RParen);
 }
 
+static void MaybeDecrementCount(
+    Expr *E, llvm::DenseMap<const VarDecl *, int> &RefsMinusAssignments) {
+  DeclRefExpr *LHS = nullptr;
+  if (BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
+    if (!BO->isAssignmentOp())
+      return;
+    LHS = dyn_cast<DeclRefExpr>(BO->getLHS());
+  } else if (CXXOperatorCallExpr *COCE = dyn_cast<CXXOperatorCallExpr>(E)) {
+    if (!COCE->isAssignmentOp())
+      return;
+    LHS = dyn_cast<DeclRefExpr>(COCE->getArg(0));
+  }
+  if (!LHS)
+    return;
+  VarDecl *VD = dyn_cast<VarDecl>(LHS->getDecl());
+  if (!VD)
+    return;
+  auto iter = RefsMinusAssignments.find(VD);
+  if (iter == RefsMinusAssignments.end())
+    return;
+  iter->getSecond()--;
+}
+
 /// Perform the conversions required for an expression used in a
 /// context that ignores the result.
 ExprResult Sema::IgnoredValueConversions(Expr *E) {
+  MaybeDecrementCount(E, RefsMinusAssignments);
+
   if (E->hasPlaceholderType()) {
     ExprResult result = CheckPlaceholderExpr(E);
     if (result.isInvalid()) return E;
     E = result.get();
   }
 
   // C99 6.3.2.1:
   //   [Except in specific positions,] an lvalue that does not have

clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp

-// RUN: %clang_cc1 -std=c++11 %s -Wunused -Wno-unused-lambda-capture -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wunused -Wno-unused-but-set-variable -Wno-unused-lambda-capture -verify
 
 void odr_used() {
   int i = 17;
   [i]{}();
 }
 
 struct ReachingThis {
   static void static_foo() {

clang/test/CodeGen/2007-10-30-Volatile.c

-// RUN: %clang_cc1 -emit-llvm %s -o /dev/null -Wall -Werror
+// RUN: %clang_cc1 -emit-llvm %s -o /dev/null -Wall -Wno-unused-but-set-variable -Werror
 void bork() {
   char * volatile p = 0;
   volatile int cc = 0;
   p += cc;
 }

clang/test/CodeGen/X86/x86_32-xsave.c

-// RUN: %clang_cc1 %s -DTEST_XSAVE -O0 -triple=i686-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVE
-// RUN: %clang_cc1 %s -DTEST_XSAVE -O0 -triple=i686-unknown-unknown -target-feature +xsave -fno-signed-char -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVE
+// RUN: %clang_cc1 %s -DTEST_XSAVE -O0 -triple=i686-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVE
+// RUN: %clang_cc1 %s -DTEST_XSAVE -O0 -triple=i686-unknown-unknown -target-feature +xsave -fno-signed-char -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVE
 
-// RUN: %clang_cc1 %s -DTEST_XGETBV -O0 -triple=i686-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XGETBV
-// RUN: %clang_cc1 %s -DTEST_XSETBV -O0 -triple=i686-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSETBV
+// RUN: %clang_cc1 %s -DTEST_XGETBV -O0 -triple=i686-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XGETBV
+// RUN: %clang_cc1 %s -DTEST_XSETBV -O0 -triple=i686-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSETBV
 
-// RUN: %clang_cc1 %s -DTEST_XSAVEOPT -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsaveopt -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVEOPT
-// RUN: %clang_cc1 %s -DTEST_XSAVEOPT -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsaveopt -fno-signed-char -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVEOPT
+// RUN: %clang_cc1 %s -DTEST_XSAVEOPT -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsaveopt -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVEOPT
+// RUN: %clang_cc1 %s -DTEST_XSAVEOPT -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsaveopt -fno-signed-char -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVEOPT
 
-// RUN: %clang_cc1 %s -DTEST_XSAVEC -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsavec -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVEC
-// RUN: %clang_cc1 %s -DTEST_XSAVEC -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsavec -fno-signed-char -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVEC
+// RUN: %clang_cc1 %s -DTEST_XSAVEC -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsavec -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVEC
+// RUN: %clang_cc1 %s -DTEST_XSAVEC -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsavec -fno-signed-char -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVEC
 
-// RUN: %clang_cc1 %s -DTEST_XSAVES -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsaves -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVES
-// RUN: %clang_cc1 %s -DTEST_XSAVES -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsaves -fno-signed-char -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVES
+// RUN: %clang_cc1 %s -DTEST_XSAVES -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsaves -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVES
+// RUN: %clang_cc1 %s -DTEST_XSAVES -O0 -triple=i686-unknown-unknown -target-feature +xsave -target-feature +xsaves -fno-signed-char -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVES
 
 // Don't include mm_malloc.h, it's system specific.
 #define __MM_MALLOC_H
 #include <x86intrin.h>
 
 void test() {
   unsigned long long tmp_ULLi;
   unsigned int       tmp_Ui;

clang/test/CodeGen/X86/x86_64-xsave.c

-// RUN: %clang_cc1 %s -DTEST_XSAVE -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVE
-// RUN: %clang_cc1 %s -DTEST_XSAVE -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -fno-signed-char -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVE
+// RUN: %clang_cc1 %s -DTEST_XSAVE -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVE
+// RUN: %clang_cc1 %s -DTEST_XSAVE -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -fno-signed-char -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVE
 
-// RUN: %clang_cc1 %s -DTEST_XGETBV -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XGETBV
-// RUN: %clang_cc1 %s -DTEST_XSETBV -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -fno-signed-char -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSETBV
+// RUN: %clang_cc1 %s -DTEST_XGETBV -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XGETBV
+// RUN: %clang_cc1 %s -DTEST_XSETBV -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -fno-signed-char -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSETBV
 
-// RUN: %clang_cc1 %s -DTEST_XSAVEOPT -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsaveopt -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVEOPT
-// RUN: %clang_cc1 %s -DTEST_XSAVEOPT -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsaveopt -fno-signed-char -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVEOPT
+// RUN: %clang_cc1 %s -DTEST_XSAVEOPT -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsaveopt -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVEOPT
+// RUN: %clang_cc1 %s -DTEST_XSAVEOPT -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsaveopt -fno-signed-char -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVEOPT
 
-// RUN: %clang_cc1 %s -DTEST_XSAVEC -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsavec -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVEC
-// RUN: %clang_cc1 %s -DTEST_XSAVEC -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsavec -fno-signed-char -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVEC
+// RUN: %clang_cc1 %s -DTEST_XSAVEC -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsavec -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVEC
+// RUN: %clang_cc1 %s -DTEST_XSAVEC -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsavec -fno-signed-char -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVEC
 
-// RUN: %clang_cc1 %s -DTEST_XSAVES -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsaves -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVES
-// RUN: %clang_cc1 %s -DTEST_XSAVES -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsaves -fno-signed-char -emit-llvm -o - -Wall -Werror | FileCheck %s --check-prefix=XSAVES
+// RUN: %clang_cc1 %s -DTEST_XSAVES -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsaves -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVES
+// RUN: %clang_cc1 %s -DTEST_XSAVES -O0 -triple=x86_64-unknown-unknown -target-feature +xsave -target-feature +xsaves -fno-signed-char -emit-llvm -o - -Wall -Wno-unused-but-set-variable -Werror | FileCheck %s --check-prefix=XSAVES
 
 // Don't include mm_malloc.h, it's system specific.
 #define __MM_MALLOC_H
 #include <x86intrin.h>
 
 
 void test() {
   unsigned long long tmp_ULLi;

clang/test/CodeGen/builtins-arm.c

-// RUN: %clang_cc1 -Wall -Werror -triple thumbv7-eabi -target-cpu cortex-a8 -disable-O0-optnone -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
+// RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -Werror -triple thumbv7-eabi -target-cpu cortex-a8 -disable-O0-optnone -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
 
 #include <stdint.h>
 
 void *f0()
 {
   return __builtin_thread_pointer();
 }
 

clang/test/CodeGen/builtins-riscv.c

-// RUN: %clang_cc1 -Wall -Werror -triple riscv32 -disable-O0-optnone -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
-// RUN: %clang_cc1 -Wall -Werror -triple riscv64 -disable-O0-optnone -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
+// RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -Werror -triple riscv32 -disable-O0-optnone -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
+// RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -Werror -triple riscv64 -disable-O0-optnone -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
 
 void test_eh_return_data_regno() {
   // CHECK: store volatile i32 10
   // CHECK: store volatile i32 11
   volatile int res;
   res = __builtin_eh_return_data_regno(0);
   res = __builtin_eh_return_data_regno(1);
 }

clang/test/FixIt/fixit.cpp

-// RUN: %clang_cc1 -pedantic -Wall -Wno-comment -verify -fcxx-exceptions -x c++ -std=c++98 %s
+// RUN: %clang_cc1 -pedantic -Wall -Wno-unused-but-set-variable -Wno-comment -verify -fcxx-exceptions -x c++ -std=c++98 %s
 // RUN: cp %s %t-98
-// RUN: not %clang_cc1 -pedantic -Wall -Wno-comment -fcxx-exceptions -fixit -x c++ -std=c++98 %t-98
-// RUN: %clang_cc1 -fsyntax-only -pedantic -Wall -Werror -Wno-comment -fcxx-exceptions -x c++ -std=c++98 %t-98
+// RUN: not %clang_cc1 -pedantic -Wall -Wno-unused-but-set-variable -Wno-comment -fcxx-exceptions -fixit -x c++ -std=c++98 %t-98
+// RUN: %clang_cc1 -fsyntax-only -pedantic -Wall -Wno-unused-but-set-variable -Werror -Wno-comment -fcxx-exceptions -x c++ -std=c++98 %t-98
 // RUN: not %clang_cc1 -fsyntax-only -pedantic -fdiagnostics-parseable-fixits -x c++ -std=c++11 %s 2>&1 | FileCheck %s
-// RUN: %clang_cc1 -pedantic -Wall -Wno-comment -verify -fcxx-exceptions -x c++ -std=c++11 %s
+// RUN: %clang_cc1 -pedantic -Wall -Wno-unused-but-set-variable -Wno-comment -verify -fcxx-exceptions -x c++ -std=c++11 %s
 // RUN: cp %s %t-11
-// RUN: not %clang_cc1 -pedantic -Wall -Wno-comment -fcxx-exceptions -fixit -x c++ -std=c++11 %t-11
-// RUN: %clang_cc1 -fsyntax-only -pedantic -Wall -Werror -Wno-comment -fcxx-exceptions -x c++ -std=c++11 %t-11
+// RUN: not %clang_cc1 -pedantic -Wall -Wno-unused-but-set-variable -Wno-comment -fcxx-exceptions -fixit -x c++ -std=c++11 %t-11
+// RUN: %clang_cc1 -fsyntax-only -pedantic -Wall -Wno-unused-but-set-variable -Werror -Wno-comment -fcxx-exceptions -x c++ -std=c++11 %t-11
 
 /* This is a test of the various code modification hints that are
    provided as part of warning or extension diagnostics. All of the
    warnings will be fixed by -fixit, and the resulting file should
    compile cleanly with -Werror -pedantic. */
 
 struct C1 {
   virtual void f();

clang/test/Misc/warning-wall.c

 CHECK-NEXT:      -Wunused-local-typedef
 CHECK-NEXT:      -Wunused-value
 CHECK-NEXT:        -Wunused-comparison
 CHECK-NEXT:        -Wunused-result
 CHECK-NEXT:        -Wunevaluated-expression
 CHECK-NEXT:          -Wpotentially-evaluated-expression
 CHECK-NEXT:      -Wunused-variable
 CHECK-NEXT:        -Wunused-const-variable
+CHECK-NEXT:      -Wunused-but-set-variable
 CHECK-NEXT:      -Wunused-property-ivar
 CHECK-NEXT:    -Wvolatile-register-var
 CHECK-NEXT:    -Wobjc-missing-super-calls
 CHECK-NEXT:    -Wobjc-designated-initializers
 CHECK-NEXT:    -Wobjc-flexible-array
 CHECK-NEXT:    -Woverloaded-virtual
 CHECK-NEXT:    -Wprivate-extern
 CHECK-NEXT:    -Wcast-of-sel-type

clang/test/Sema/shift.c

-// RUN: %clang_cc1 -Wall -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify %s
 
 #include <limits.h>
 
 #define WORD_BIT (sizeof(int) * CHAR_BIT)
 
 enum {
   X = 1 << 0,
   Y = 1 << 1,

clang/test/Sema/vector-gcc-compat.c

-// RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -triple x86_64-apple-darwin10
+// RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -Wno-unused-but-set-variable -triple x86_64-apple-darwin10
 
 // Test the compatibility of clang's vector extensions with gcc's vector
 // extensions for C. Notably &&, ||, ?: and ! are not available.
 typedef long long v2i64 __attribute__((vector_size(16)));
 typedef int v2i32 __attribute__((vector_size(8)));
 typedef short v2i16 __attribute__((vector_size(4)));
 typedef char v2i8 __attribute__((vector_size(2)));
 

 void uintTestType(char a, short b, int c, long long d);
 void uintTestTypeUnsigned(unsigned char a, unsigned short b, unsigned int c,
                           unsigned long long d);
 void uintTestConstant(v2u64 v2u64_a, v2u32 v2u32_a, v2u16 v2u16_a, v2u8 v2u8_a);
 void intTestConstant(v2i64 v2i64_a, v2i32 v2i32_a, v2i16 v2i16_a, v2i8 v2i8_a);
 
 void arithmeticTest(void) {
   v2i64 v2i64_a = (v2i64){0, 1};
   v2i64 v2i64_r;
 
   v2i64_r = v2i64_a + 1;
   v2i64_r = v2i64_a - 1;
   v2i64_r = v2i64_a * 1;
   v2i64_r = v2i64_a / 1;
   v2i64_r = v2i64_a % 1;
 
   v2i64_r = 1 + v2i64_a;

clang/test/Sema/vector-gcc-compat.cpp

-// RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -std=c++11 -triple x86_64-apple-darwin10
+// RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -Wno-unused-but-set-variable -std=c++11 -triple x86_64-apple-darwin10
 
 // Test the compatibility of clang++'s vector extensions with g++'s vector
 // extensions. In comparison to the extensions available in C, the !, ?:, && and
 // || operators work on vector types.
 
 typedef long long v2i64 __attribute__((vector_size(16))); // expected-warning {{'long long' is incompatible with C++98}}
 typedef int v2i32 __attribute__((vector_size(8)));
 typedef short v2i16 __attribute__((vector_size(4)));

clang/test/Sema/warn-unused-but-set-parameters.c

+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+       int y, // expected-warning{{parameter 'y' set but not used}}
+       int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+          int y, // expected-warning{{parameter 'y' set but not used}}
+          int z __attribute__((unused))) {
+    y = 0;
+    return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+void f3(struct S s) { // expected-warning{{parameter 's' set but not used}}
+  struct S t;
+  s = t;
+}

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

+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  struct S s; // expected-warning{{variable 's' set but not used}}
+  struct S t;
+  s = t;
+
+  // Don't warn for an extern variable.
+  extern int w;
+  w = 0;
+
+  // Following gcc, this should not warn.
+  int a;
+  w = (a = 0);
+
+  int x;
+  x = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^() {
+    int y; // expected-warning{{variable 'y' set but not used}}
+    y = 0;
+
+    int x;
+    x = 0;
+    return x;
+  };
+}
+
+void f2 (void) {
+  // Don't warn, even if it's only used in a non-ODR context.
+  int x;
+  x = 0;
+  (void) sizeof(x);
+}

clang/test/SemaCXX/goto.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -fblocks %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused-but-set-variable -fblocks %s
 
 // PR9463
 double *end;
 void f(bool b1, bool b2) {
   {
     do {
       int end = 0;
       if (b2) {

clang/test/SemaCXX/shift.cpp

-// RUN: %clang_cc1 -Wall -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_cc1 -Wall -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify=expected,cxx2a -std=c++2a %s
 
 #include <limits.h>
 
 #define WORD_BIT (sizeof(int) * CHAR_BIT)
 
 enum {
   X = 1 << 0,
   Y = 1 << 1,

clang/test/SemaCXX/sizeless-1.cpp

-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
 
 namespace std {
 struct type_info;
 }
 
 typedef __SVInt8_t svint8_t;
 typedef __SVInt16_t svint16_t;
 

clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp

+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+       int y, // expected-warning{{parameter 'y' set but not used}}
+       int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+          int y, // expected-warning{{parameter 'y' set but not used}}
+          int z __attribute__((unused))) {
+    y = 0;
+    return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};

clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp

+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+struct __attribute__((warn_unused)) SWarnUnused {
+  int j;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  // Unless it's marked with the warn_unused attribute.
+  struct SWarnUnused swu; // expected-warning{{variable 'swu' set but not used}}
+  struct SWarnUnused swu2;
+  swu = swu2;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+    int y; // expected-warning{{variable 'y' set but not used}}
+    y = 0;
+
+    int x;
+    x = 0;
+    return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}

clang/test/SemaObjC/foreach.m

-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
 
 void f(NSArray *a) {
     id keys;
     for (int i in a); /* expected-error{{selector element type 'int' is not a valid object}} */
     for ((id)2 in a); /* expected-error{{selector element is not a valid lvalue}} */