Disable specific MSVC warnings so that W4 warnings can be enabled.

[andrew.w.kaylor]

Summary

These changes disable a set of warnings for the MSVC compiler so that stricter warnings can be enabled with useful results.

The following warnings are being suppressed:

warning C4100: unreferenced formal parameter (263666 times)
warning C4127: conditional expression is constant (101120 times)
warning C4512: assignment operator could not be generated (40472 times)
warning C4505: unreferenced local function has been removed (6606 times)
warning C4610: [...] can never be instantiated (1714 times)
warning C4510: default constructor could not be generated (1714 times)
warning C4702: unreachable code (1343 times)
warning C4706: assignment within conditional expression (296 times)
warning C4245: signed/unsigned mismatch (962 times)
warning C4310: cast truncates constant value (216 times)
warning C4701: potentially uninitialized local variable (123 times)
warning C4703: potentially uninitialized local pointer variable (40 times)
warning C4389: signed/unsigned mismatch (28 times)
warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable (2 times)

We may choose to re-enable some of these at a later time, but it would involve more effort than I am prepared to make at this time.

I have made changes to fix the few instances of the following warnings that occur:

warning C4189: local variable is initialized but not referenced (6 times)
warning C4204: nonstandard extension used : non-constant aggregate initializer (4 times)
warning C4805: unsafe mix of type <X> and type <Y> in operation (2 times)

[andrew.w.kaylor]

cmake/modules/HandleLLVMOptions.cmake

290 ↗

(On Diff #22534)

I'm not sure what happened to the spacing here. I'll fix it before committing.

lib/Support/APFloat.cpp

1433 ↗

(On Diff #22534)

This addresses warning C4805: unsafe mix of type <X> and type <Y> in operation.

lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp

641 ↗

(On Diff #22534)

This addresses warning C4189: local variable is initialized but not referenced.

AlwaysPrintImm0 is a template parameter that can short circuit the condition so that ImmOffs and Op are never referenced.

lib/Target/NVPTX/NVPTXAsmPrinter.cpp

507 ↗

(On Diff #22534)

This addresses warning C4189: local variable is initialized but not referenced.

Because isVirtualRegister() is a static function, the TRI variable was not actually referenced.

lib/Target/PowerPC/PPCInstrInfo.cpp

117 ↗

(On Diff #22534)

This addresses warning C4189: local variable is initialized but not referenced.

Because isVirtualRegister() is a static function, the TRI variable was not actually referenced.

tools/llvm-c-test/metadata.c

21 ↗

(On Diff #22534)

This addresses warning C4204: nonstandard extension used : non-constant aggregate initializer.

34 ↗

(On Diff #22534)

This addresses warning C4204: nonstandard extension used : non-constant aggregate initializer.

[dblaikie]

cmake/modules/HandleLLVMOptions.cmake

286 ↗

(On Diff #22534)

You mentioned this one fired ~200 times - this might actually be worth doing a small sample check to see what it's firing on. Perhaps these are things we should fix - unless they appear in dependent expressions in templates, in which case they're probably false positives.

The rest of these disables I'm totally fine with - if you want, please commit them separately/ahead of the rest of the review.

lib/Support/APFloat.cpp

1433 ↗

(On Diff #22534)

This one's been fixed (slightly differently) by Aaron Ballman. We had some more discussions about the same issues/principles in that review thread too - I think he's OK with the idea of disabling this warning.

lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp

641 ↗

(On Diff #22534)

This seems like a step backwards, actually - we try to deliberately put the cheap tests first (and a compile time constant is the cheapest test). Granted any compiler worth its salt will kill the dead expressions, but there's no need to ask it to do that work.

I'm fairly strongly in favor of disabling this warning rather than making this code change.

lib/Target/NVPTX/NVPTXAsmPrinter.cpp

507 ↗

(On Diff #22534)

The code change is good, the motivation is questionable - I'd probably fix the code but still disable the warning. A clang-tidy check for calling static functions from a non-static context might be nice to have, but it doesn't really raise to the level of compiler warning.

tools/llvm-c-test/metadata.c

21 ↗

(On Diff #22534)

Clang has a warning for this (-Wc99-extensions) but we don't enable it, so I'd say it's reasonable to disable MSVC's equivalent for consistency. Unless someone wants to look at turning Clang's version on & seeing what breaks...

[andrew.w.kaylor]

cmake/modules/HandleLLVMOptions.cmake

286 ↗

(On Diff #22534)

These look pretty harmless. All of the ones I looked at were something like these:

uint8_t(~0U)
(char)0xFF
(uint16_t)-1U

lib/Support/APFloat.cpp

1433 ↗

(On Diff #22534)

OK

lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp

641 ↗

(On Diff #22534)

If it were just cases like this covered by the warning I would definitely agree that it's preferable to disable the warning. The problem, as Aaron Ballman mentioned, is that this same warning reports the basic case.

void foo() {
  int i = 0;
  // Doesn't use i
}

That's exactly the sort of problem that I want to see when compiling with MSVC that motivated me to undertake this change set.

I hear what you're saying about the change here moving backward, though the way the code is written the compiler still has to know that the calls above the condition have no side effects to eliminate them. How about if instead I rewrite the condition like this?

if (AlwaysPrintImm0 ||
    ARM_AM::getAM5Offset(MO2.getImm()) ||
    ARM_AM::getAM5Op(MO2.getImm()) == ARM_AM::sub) {

tools/llvm-c-test/metadata.c

21 ↗

(On Diff #22534)

I have no objection to disabling this warning.

[andrew.w.kaylor]

If your goal is to avoid breaking the Clang -Werror build, I don't think there's much you can do other than use a Clang build. But honestly enough people don't use it that those of us who do are used to cleaning up what gets through, so I wouldn't worry about it too much if I were you.

My goal is to be able to turn on the “all warnings” option for MSVC builds. I understand that this won’t exactly replicate the Clang warning set, but I’m working from the premise that warnings should be fixed because it makes the code better. So even if a /W4 build with MSVC doesn’t report everything that Clang would report, I still think it will be helpful.

I think that as a side effect it has the potential to make breaking the Clang -Werror less common, at least for me personally.

Anyway, if you don’t object, I’ll take out the disputed changes and commit the changes we’ve agreed on. We can see how this evolves from there.

-Andy

From: [e-mail] [mailto:[e-mail]] On Behalf Of David Blaikie
Sent: Tuesday, March 24, 2015 11:43 AM
To: [e-mail]
Cc: [e-mail]
Subject: Re: [PATCH] Disable specific MSVC warnings so that W4 warnings can be enabled.

llvm/trunk/cmake/modules/HandleLLVMOptions.cmake

set(msvc_warning_flags
     -wd4456 # Suppress 'declaration of 'var' hides local variable'
     -wd4457 # Suppress 'declaration of 'var' hides function parameter'
     -wd4458 # Suppress 'declaration of 'var' hides class member'
     -wd4459 # Suppress 'declaration of 'var' hides global declaration'
     -wd4503 # Suppress ''identifier' : decorated name length exceeded, name was truncated'
     -wd4624 # Suppress ''derived class' : destructor could not be generated because a base class destructor is inaccessible'
     -wd4722 # Suppress 'function' : destructor never returns, potential memory leak
     -wd4800 # Suppress ''type' : forcing value to bool 'true' or 'false' (performance warning)'
+    -wd4100 # Suppress 'unreferenced formal parameter'
+    -wd4127 # Suppress 'conditional expression is constant'
+    -wd4512 # Suppress 'assignment operator could not be generated'
+    -wd4505 # Suppress 'unreferenced local function has been removed'
+    -wd4610 # Suppress '<class> can never be instantiated'
+    -wd4510 # Suppress 'default constructor could not be generated'
+    -wd4702 # Suppress 'unreachable code'
+    -wd4245 # Suppress 'signed/unsigned mismatch'
+    -wd4706 # Suppress 'assignment within conditional expression'
+    -wd4310 # Suppress 'cast truncates constant value'
+    -wd4701 # Suppress 'potentially uninitialized local variable'
+    -wd4703 # Suppress 'potentially uninitialized local pointer variable'
+    -wd4389 # Suppress 'signed/unsigned mismatch'
+    -wd4611 # Suppress 'interaction between '_setjmp' and C++ object destruction is non-portable'
+    -wd4805 # Suppress 'unsafe mix of type <type> and type <type> in operation'
+    -wd4204 # Suppress 'nonstandard extension used : non-constant aggregate initializer'
     
     # Promoted warnings.
     -w14062 # Promote 'enumerator in switch of enum is not handled' to level 1 warning.
 
     # Promoted warnings to errors.
     -we4238 # Promote 'nonstandard extension used : class rvalue used as lvalue' to error.
     )
 

llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp

 
 void NVPTXAsmPrinter::EmitFunctionBodyEnd() {
   OutStreamer.EmitRawText(StringRef("}\n"));
   VRegMapping.clear();
 }
 
 void NVPTXAsmPrinter::emitImplicitDef(const MachineInstr *MI) const {
   unsigned RegNo = MI->getOperand(0).getReg();
-  const TargetRegisterInfo *TRI = nvptxSubtarget->getRegisterInfo();
-  if (TRI->isVirtualRegister(RegNo)) {
+  if (TargetRegisterInfo::isVirtualRegister(RegNo)) {
     OutStreamer.AddComment(Twine("implicit-def: ") +
                            getVirtualRegisterName(RegNo));
   } else {
     OutStreamer.AddComment(Twine("implicit-def: ") +
                            nvptxSubtarget->getRegisterInfo()->getName(RegNo));
   }
   OutStreamer.AddBlankLine();
 }

llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp

int PPCInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,
                                     const MachineInstr *UseMI,
                                     unsigned UseIdx) const {
   int Latency = PPCGenInstrInfo::getOperandLatency(ItinData, DefMI, DefIdx,
                                                    UseMI, UseIdx);
 
   const MachineOperand &DefMO = DefMI->getOperand(DefIdx);
   unsigned Reg = DefMO.getReg();
 
-  const TargetRegisterInfo *TRI = &getRegisterInfo();
   bool IsRegCR;
-  if (TRI->isVirtualRegister(Reg)) {
+  if (TargetRegisterInfo::isVirtualRegister(Reg)) {
     const MachineRegisterInfo *MRI =
       &DefMI->getParent()->getParent()->getRegInfo();
     IsRegCR = MRI->getRegClass(Reg)->hasSuperClassEq(&PPC::CRRCRegClass) ||
               MRI->getRegClass(Reg)->hasSuperClassEq(&PPC::CRBITRCRegClass);
   } else {
     IsRegCR = PPC::CRRCRegClass.contains(Reg) ||
               PPC::CRBITRCRegClass.contains(Reg);
   }