[Lldb-commits] [PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via lldb-commits]

mbenfield updated this revision to Diff 346904.
mbenfield added a comment.
Herald added subscribers: llvm-commits, lldb-commits, kbarton, hiraditya, 
nemanjai.
Herald added projects: LLDB, LLVM.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/vector-gcc-compat.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp
  llvm/lib/Target/AArch64/GISel/AArch64PostSelectOptimize.cpp
  llvm/lib/Target/ARM/ARMISelLowering.cpp
  llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
  llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
  llvm/lib/Target/X86/X86FloatingPoint.cpp
  llvm/utils/benchmark/src/complexity.cc

Index: llvm/utils/benchmark/src/complexity.cc
===
--- llvm/utils/benchmark/src/complexity.cc
+++ llvm/utils/benchmark/src/complexity.cc
@@ -76,7 +76,6 @@
 LeastSq MinimalLeastSq(const std::vector& n,
const std::vector& time,
BigOFunc* fitting_curve) {
-  double sigma_gn = 0.0;
   double sigma_gn_squared = 0.0;
   double sigma_time = 0.0;
   double sigma_time_gn = 0.0;
@@ -84,7 +83,6 @@
   // Calculate least square fitting parameter
   for (size_t i = 0; i < n.size(); ++i) {
 double gn_i = fitting_curve(n[i]);
-sigma_gn += gn_i;
 sigma_gn_squared += gn_i * gn_i;
 sigma_time += time[i];
 sigma_time_gn += time[i] * gn_i;
Index: llvm/lib/Target/X86/X86FloatingPoint.cpp
===
--- llvm/lib/Target/X86/X86FloatingPoint.cpp
+++ llvm/lib/Target/X86/X86FloatingPoint.cpp
@@ -1526,7 +1526,7 @@
 
 // Scan the assembly for ST registers used, defined and clobbered. We can
 // only tell clobbers from defs by looking at the asm descriptor.
-unsigned STUses = 0, STDefs = 0, STClobbers = 0, STDeadDefs = 0;
+unsigned STUses = 0, STDefs = 0, STClobbers = 0;
 unsigned NumOps = 0;
 SmallSet FRegIdx;
 unsigned RCID;
@@ -1559,8 +1559,6 @@
   case InlineAsm::Kind_RegDef:
   case InlineAsm::Kind_RegDefEarlyClobber:
 STDefs |= (1u << STReg);
-if (MO.isDead())
-  STDeadDefs |= (1u << STReg);
 break;
   case InlineAsm::Kind_Clobber:
 STClobbers |= (1u << STReg);
Index: llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
===
--- llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
+++ llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
@@ -415,5 +415,5 @@
   }
 
   BlockSizes.clear();
-  return true;
+  return EverMadeChange;
 }
Index: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
===
--- llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
+++ llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
@@ -1247,7 +1247,7 @@
   if (LoLoop.Preheader)
 LoLoop.Start = SearchForStart(LoLoop.Preheader);
   else
-return false;
+return Changed;
 
   // Find the low-overhead loop components and decide whether or not to fall
   // back to a normal loop. Also look for a vctp instructions and decide
@@ -1281,7 +1281,7 @@
   LLVM_DEBUG(LoLoop.dump());
   if (!LoLoop.FoundAllComponents()) {
 LLVM_DEBUG(dbgs() << "ARM Loops: Didn't find loop start, update, end\n");
-return false;
+return Changed;
   }
 
   assert(LoLoop.Start->getOpcode() != ARM::t2WhileLoopStart &&
Index: llvm/lib/Target/ARM/ARMISelLowering.cpp
===
--- llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -6

[Lldb-commits] [PATCH] D102942: Remove or use variables which are unused but set.

[Michael Benfield via Phabricator via lldb-commits]

mbenfield created this revision.
Herald added subscribers: pengfei, lebedev.ri, kbarton, hiraditya, nemanjai.
Herald added a reviewer: lebedev.ri.
mbenfield requested review of this revision.
Herald added projects: LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits.

This is in preparation for the -Wunused-but-set-variable warning.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102942

Files:
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp
  llvm/lib/Target/AArch64/GISel/AArch64PostSelectOptimize.cpp
  llvm/lib/Target/ARM/ARMISelLowering.cpp
  llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
  llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
  llvm/lib/Target/X86/X86FloatingPoint.cpp
  llvm/utils/benchmark/src/complexity.cc

Index: llvm/utils/benchmark/src/complexity.cc
===
--- llvm/utils/benchmark/src/complexity.cc
+++ llvm/utils/benchmark/src/complexity.cc
@@ -76,7 +76,6 @@
 LeastSq MinimalLeastSq(const std::vector& n,
const std::vector& time,
BigOFunc* fitting_curve) {
-  double sigma_gn = 0.0;
   double sigma_gn_squared = 0.0;
   double sigma_time = 0.0;
   double sigma_time_gn = 0.0;
@@ -84,7 +83,6 @@
   // Calculate least square fitting parameter
   for (size_t i = 0; i < n.size(); ++i) {
 double gn_i = fitting_curve(n[i]);
-sigma_gn += gn_i;
 sigma_gn_squared += gn_i * gn_i;
 sigma_time += time[i];
 sigma_time_gn += time[i] * gn_i;
Index: llvm/lib/Target/X86/X86FloatingPoint.cpp
===
--- llvm/lib/Target/X86/X86FloatingPoint.cpp
+++ llvm/lib/Target/X86/X86FloatingPoint.cpp
@@ -1526,7 +1526,7 @@
 
 // Scan the assembly for ST registers used, defined and clobbered. We can
 // only tell clobbers from defs by looking at the asm descriptor.
-unsigned STUses = 0, STDefs = 0, STClobbers = 0, STDeadDefs = 0;
+unsigned STUses = 0, STDefs = 0, STClobbers = 0;
 unsigned NumOps = 0;
 SmallSet FRegIdx;
 unsigned RCID;
@@ -1559,8 +1559,6 @@
   case InlineAsm::Kind_RegDef:
   case InlineAsm::Kind_RegDefEarlyClobber:
 STDefs |= (1u << STReg);
-if (MO.isDead())
-  STDeadDefs |= (1u << STReg);
 break;
   case InlineAsm::Kind_Clobber:
 STClobbers |= (1u << STReg);
Index: llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
===
--- llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
+++ llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
@@ -415,5 +415,5 @@
   }
 
   BlockSizes.clear();
-  return true;
+  return EverMadeChange;
 }
Index: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
===
--- llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
+++ llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
@@ -1247,7 +1247,7 @@
   if (LoLoop.Preheader)
 LoLoop.Start = SearchForStart(LoLoop.Preheader);
   else
-return false;
+return Changed;
 
   // Find the low-overhead loop components and decide whether or not to fall
   // back to a normal loop. Also look for a vctp instructions and decide
@@ -1281,7 +1281,7 @@
   LLVM_DEBUG(LoLoop.dump());
   if (!LoLoop.FoundAllComponents()) {
 LLVM_DEBUG(dbgs() << "ARM Loops: Didn't find loop start, update, end\n");
-return false;
+return Changed;
   }
 
   assert(LoLoop.Start->getOpcode() != ARM::t2WhileLoopStart &&
Index: llvm/lib/Target/ARM/ARMISelLowering.cpp
===
--- llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -6746,12 +6746,10 @@
   return SDValue();
 // NEON has a 64-bit VMOV splat where each byte is either 0 or 0xff.
 uint64_t BitMask = 0xff;
-uint64_t Val = 0;
 unsigned ImmMask = 1;
 Imm = 0;
 for (int ByteNum = 0; ByteNum < 8; ++ByteNum) {
   if (((SplatBits | SplatUndef) & BitMask) == BitMask) {
-Val |= BitMask;
 Imm |= ImmMask;
   } else if ((SplatBits & BitMask) != 0) {
 return SDValue();
Index: llvm/lib/Target/AArch64/GISel/AArch64PostSelectOptimize.cpp
===
--- llvm/lib/Target/AArch64/GISel/AArch64PostSelectOptimize.cpp
+++ llvm/lib/Target/AArch64/GISel/AArch64PostSelectOptimize.cpp
@@ -180,7 +180,7 @@
   bool Changed = false;
   for (auto &BB : MF)
 Changed |= optimizeNZCVDefs(BB);
-  return true;
+  return Changed;
 }
 
 char AArch64PostSelectOptimize::ID = 0;
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp
+++ lldb/

[Lldb-commits] [PATCH] D102942: Remove or use variables which are unused but set.

[Michael Benfield via Phabricator via lldb-commits]

mbenfield added a comment.
Herald added a subscriber: JDevlieghere.

Several variables I have just removed outright, including

llvm/lib/Target/X86/X86FloatingPoint.cpp: STDeadDefs
lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp: fp_bytes
lldb/source/Interpreter/CommandInterpreter.cpp: actual_cmd_name_len

Someone familiar with this code should probably look at these and determine 
whether these variables not being used could have lead to buggy behavior.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102942/new/

https://reviews.llvm.org/D102942

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D102942: Remove or use variables which are unused but set.

[Michael Benfield via Phabricator via lldb-commits]

mbenfield added a comment.

In D102942#2774819 , @dblaikie wrote:

> Sure, all sounds good - if you can, please reach out to the authors of any of 
> the semantics changing changes (the ones related to the `Changed` values in 
> transformations) to see if they could add missing test coverage.

Sure, I emailed Sam Parker and Amara Emerson.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102942/new/

https://reviews.llvm.org/D102942

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D102942: Remove or use variables which are unused but set.

[Michael Benfield via Phabricator via lldb-commits]

mbenfield added a comment.

I also heard via email from Amara Emerson that the change is fine for similar 
reasons.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102942/new/

https://reviews.llvm.org/D102942

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via lldb-commits]

mbenfield updated this revision to Diff 347111.
mbenfield added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/vector-gcc-compat.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* 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;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,50 @@
+// 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];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,38 @@
+// 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;
+}
+
+// Also don't warn for a reference.
+void f4(int &x) {
+  x = 0;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// 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-unu

[Lldb-commits] [PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via lldb-commits]

mbenfield added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via lldb-commits]

mbenfield added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits