This is an archive of the discontinued LLVM Phabricator instance.

Fix some warnings on MSVC
ClosedPublic

Authored by aganea on Jan 4 2019, 12:54 PM.

Details

Summary

Fixes the following warnings on (latest) MSVC:

  1. In Kaleidoscope-Ch2: "warning LNK4199: /DELAYLOAD:shell32.dll ignored; no imports found from shell32.dll"
  1. In gtest (see this link):
f:\svn\llvm\utils\unittest\googlemock\include\gmock\gmock-matchers.h(183): warning C5046: 'testing::MatcherInterface<T>::~MatcherInterface': Symbol involving type with internal linkage not defined
23>        with
23>        [
23>            T=`anonymous-namespace'::CustomSubError &
23>        ] (compiling source file F:\svn\llvm\unittests\Support\ErrorTest.cpp)
  1. In SupportTests: cl : Command line warning D9025: overriding '/W4' with '/w'

Diff Detail

Repository
rL LLVM

Event Timeline

aganea created this revision.Jan 4 2019, 12:54 PM

Thanks for working on this, Alexandre!

  1. ignore "warning LNK4199: /DELAYLOAD:shell32.dll ignored; no imports found from shell32.dll

Doesn't this warning indicate that Kaleidoscope-Ch2 is not actually calling any functions from the shell32.dll it is trying to delay-load.
Fixing it instead of disabling the warning might be a better move, what do you think?

The rest looks good to me.

  1. ignore "warning LNK4199: /DELAYLOAD:shell32.dll ignored; no imports found from shell32.dll

Doesn't this warning indicate that Kaleidoscope-Ch2 is not actually calling any functions from the shell32.dll it is trying to delay-load.
Fixing it instead of disabling the warning might be a better move, what do you think?

I've tried several things and I can't find a good way to do that. Would you have anything to suggest?
-delayload:shell32.dll is added in /llvm/trunk/lib/Support/CMakeLists.txt. I don't see how to disable that from within /llvm/trunk/examples/Kaleidoscope/Chapter2/CMakeLists.txt?

rnk added a comment.Jan 15 2019, 3:26 PM

Non gmock changes look good.

utils/unittest/googlemock/include/gmock/gmock-matchers.h
145–148 ↗(On Diff #180302)

C5046 seems like some kind of undocumented warning:
https://developercommunity.visualstudio.com/content/problem/288509/undocumented-warning-c5046-symbol-involving-type-w.html

Maybe it will go away in new versions of VS if we do nothing? I don't see anything wrong with this code. It has an implicit virtual destructor because its base class has one.

I guess I'd like to hear more about how this comes up in practice before applying this change, especially since it's imported gmock code.

aganea marked an inline comment as done.Jan 16 2019, 4:26 PM
aganea added inline comments.
utils/unittest/googlemock/include/gmock/gmock-matchers.h
145–148 ↗(On Diff #180302)

Yeah it's a warning that came up in the last months, since one of the VS2017 15.8.XX upgrades. The warning I'm seeing is this:

46>f:\svn\llvm\utils\unittest\googlemock\include\gmock\gmock-matchers.h(186): warning C5046: 'testing::MatcherInterface<T>::~MatcherInterface': Symbol involving type with internal linkage not defined
46>        with
46>        [
46>            T=`anonymous-namespace'::CustomSubError &
46>        ] (compiling source file F:\svn\llvm\unittests\Support\ErrorTest.cpp)

You're probably right, it's most likely a spurious warning. For a similar example, your linked webpage says:

Jonathan Emmett [MSFT]
Thanks for this example. This is indeed a false-positive and we have a fix for this particular issue related to special member functions. It will be addressed in Visual Studio 2019.

Maybe add -ignore:5046 for ErrorTest.cpp in the meanwhile?

aganea updated this revision to Diff 182192.Jan 16 2019, 5:03 PM
rnk added inline comments.Jan 23 2019, 11:10 AM
utils/unittest/googlemock/include/gmock/gmock-matchers.h
145–148 ↗(On Diff #180302)

Sounds reasonable.

aganea marked an inline comment as done.Jan 23 2019, 12:38 PM
aganea added inline comments.
unittests/Support/CMakeLists.txt
93 ↗(On Diff #182192)

@rnk: Regarding your last comment, please see my latest updated diff (lines 92-100 below). This fixes the warning.

rnk accepted this revision.Jan 23 2019, 1:31 PM

lgtm

This revision is now accepted and ready to land.Jan 23 2019, 1:31 PM
This revision was automatically updated to reflect the committed changes.

Hi @aganea ,

With regards to

93 if( CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 19.15 )

I believe

VERSION_GREATER_EQUAL

requires CMake version 3.7, see the CMake 3.7 release notes.

The LLVM documents and llvm/CmakeLists.txt list the minimum required CMake version for LLVM as 3.4.3. I believe that the minimum CMake version should be updated or an alternative method for checking the compiler version should be used.

Hi Chris, indeed this was fixed in rL352378