These are intended to mimic warnings available in gcc.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
- 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
Comment Actions
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.
Comment Actions
FWIW, neither of these "unused-but-set" warnings were triggered on any of that code (although some other warnings were).
Comment Actions
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
Comment Actions
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.
Comment Actions
Great. If it's been approved, I am not able to commit, so I think someone else will have to do that.
Comment Actions
I can land this for you, but could you update the description to be a bit more descriptive? Explaining what the warnings do.
Comment Actions
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
Comment Actions
yeah you have to manually go to "Edit Revision" and update the message.
I'll just amend it locally though
Comment Actions
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.)
Comment Actions
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.
Comment Actions
That seems like a real finding to me, deleting selected should fix it and clean up the code.
Comment Actions
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!
Comment Actions
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.
Comment Actions
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.
Comment Actions
One more false-positive:
void foo() {
extern int yydebug;
yydebug = 1;
}It was triggered in ISPC build.
Comment Actions
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.
Comment Actions
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!
Comment Actions
This difference has caused some confusion already for the Linux kernel:
https://lore.kernel.org/r/202104280827.lscZW8Xg-lkp@intel.com
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.
Comment Actions
It's quite obviously used, since it's the condition variable for the loop.
Is clang CFG not modelling that correctly?
Comment Actions
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.
Comment Actions
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.
This comment was removed by mbenfield.
Comment Actions
There is no such requirement. I don't recall that happening basically ever, actually.
Comment Actions
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.
Comment Actions
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
Comment Actions
So it seems there are 3 issues here:
- local variables declared extern should not trigger this warning. I'll fix this.
- 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.
- 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)
Comment Actions
Yes, the best solution. Also consider to check gcc’s test-suite - if they have this type of testcase, then this is wanted behaviour.
Comment Actions
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)
Comment Actions
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.
Comment Actions
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).
Comment Actions
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)
Comment Actions
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.
Comment Actions
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.
Comment Actions
Will also make a new revision tomorrow with these warnings added to the appropriate groups.
Comment Actions
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
Comment Actions
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.
| clang/include/clang/Basic/DiagnosticGroups.td | ||
|---|---|---|
| 876 | llvm-project/clang/include/clang/Basic/DiagnosticGroups.td:874:25: error: Variable not defined: 'UnusedButSetVariable' UnusedButSetVariable, UnusedPropertyIvar]>, ^ | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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; ^
Comment Actions
Count a non-ODR use as a reference, so that examples like that of nickdesaulniers don't warn.
Comment Actions
Any idea what the purpose of this code is/why it's not a reasonable thing to warn on?
Comment Actions
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.
Comment Actions
Testing Diff 342071 on the mainline Linux kernel, just building x86_64 defconfig triggers 19 instances of this warning; all look legit. ;)
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.
Comment Actions
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:
- add an expected-warning;
- use a pragma to disable the warning;
- put an unused attribute on the variables in question;
- 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.
Comment Actions
Ah, I missed that they were _disabling_ the warning; I was thinking they were _enabling_ it. Sorry for the noise; LGTM.
Comment Actions
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.
Comment Actions
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
Comment Actions
FWIW gcc does not warn in this case regardless of any cast:
void f() {
int x;
x = 0;
sizeof(x);
}Comment Actions
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.