556886 - New warnings from STL wrappers: "warning C4275: non dll-interface class 'stdext::exception' used as base for dll-interface class"

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For example

d:\msvs8\VC\INCLUDE\typeinfo(160) : warning C4275: non dll-interface class 'stdext::exception' used as base for dll-interface class 'std::bad_typeid'
        d:\msvs8\VC\INCLUDE\exception(241) : see declaration of 'stdext::exception'
        d:\msvs8\VC\INCLUDE\typeinfo(159) : see declaration of 'std::bad_typeid'

The problem is that for |#if _HAS_EXCEPTIONS| builds, std::exception is _CRTIMP_PURE, with |#if !_HAS_EXCEPTIONS|, it's not, whereas std::bad_typeid is always _CRTIMP2.  I'm not sure what difference this makes, so I'd propose just suppressing warning 4275 in the STL wrappers.  Objections?

Comment 1

[(no longer active)]

So, let me construct a scenario which could cause problems with this warning.  Let's say that the compiler sees:

// exe.cpp

class B {
  int foo;
  // methods...
};

class __declspec(dllimport) D : public B {
  void bar() { /* access foo */ }
  // methods...
};

// dll.cpp

class __declspec(dllexport) {
  double foo;
  // methods...
};

class __declspec(dllexport) D : public B {
  void bar() { /* access foo */ }
  // methods...
};

Now, let's say that you call D::bar at some point in the exe.  In that case, D::foo will end up touching B::foo, thinking that it's a double, whereas the actual D object created in the exe code has B::foo as an int.  You will crash, if you're lucky, and debugging this problem would be unimaginably hard.

Now, in this particular case, we're already doomed if an std::bad_typeid object is ever created (because presumably it will be thrown next), therefore it is safe to ignore the warning *only for this header*.  If there are other headers with similar problems, we should ignore the warning for them individually as well.  We can't however just take a general patch which disables the warning for all code, because we want to be warned if we're actually ever making the mistake I demonstrated above for code which would actually get executed.

Comment 3

[Daniel Holbert [:dholbert]]

FWIW, I posted a patch to globally disable this warning in duplicate Bug 575020.  (nothing special, just adds |CXXFLAGS="$CXXFLAGS -wd4275"| in two configure.in files)

Not sure that's the solution we want, but it's one way to avoid this warning.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

cjones, any further thoughts on this given comment #1? Still a major source of warning spam.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

As Ehsan points out, we've already lost if the problem this warning refers to surfaces (hey, a non-vacuous warning for once!).  Best would be to push a warning-disable before including this file then popping it (http://msdn.microsoft.com/en-us/library/2c8f766e%28v=VS.71%29.aspx).

Comment 6

[(no longer active)]

(In reply to comment #5)
> As Ehsan points out, we've already lost if the problem this warning refers to
> surfaces (hey, a non-vacuous warning for once!).  Best would be to push a
> warning-disable before including this file then popping it
> (http://msdn.microsoft.com/en-us/library/2c8f766e%28v=VS.71%29.aspx).

/me agrees!

Comment 7

[Ryan VanderMeulen [:RyanVM]]

I'm having a hard time understanding where to suppress the warning. As far as I can tell by reading memory/mozalloc, the STL wrappers encompass numerous files, but I don't see where "exception" and "typeinfo" come into play.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For VS9/VS8, I think we would want to put the push/pop around the include of <exception> from msvc_throw_wrapper.h.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

+#  pragma warning(disable:4275) //Suppress spurious MSVC warning
 #  include <exception>
+#  pragma warning(default:4275)

Didn't work on VS10 anyway.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

AHA! From the earlier-linked KB article:
For warning numbers greater than 4699, those associated with code generation, the warning pragma has effect only when placed outside function definitions.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

gah, nevermind. Was thinking 4725 for some reason...

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Ben, do you have any idea what to try here?

Comment 13

[Benjamin Smedberg]

Globally disable it with a -wd4275 compile flag?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Doesn't comment #1 recommend against globally disabling the warning?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I'm pretty sure I know why the #pragma approach hasn't been working so far. At least on my system, the following mozalloc files actually get built:
mozalloc.cpp
mozalloc_abort.cpp
msvc_raise_wrappers.cpp
mozalloc_oom.cpp
msvc_raise_wrappers.cpp
mozalloc_abort.cpp

There is no sign of msvc_throw_wrapper.cpp getting used at all. Furthermore, I put some junk in msvc_throw_wrapper.h that would break the build if hit, and the build went fine with the same warning as always. For that reason, I think that this warning is actually coming in from somewhere else.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Adding the push/pop like below in throw_msvc.h didn't work either.

#pragma warning( push )             //Suppress MSVC warning spam
#pragma warning( disable : 4275 )

#if defined(MOZ_MSVC_STL_WRAP__RAISE)
#  include "msvc_raise_wrappers.h"
#elif defined(MOZ_MSVC_STL_WRAP__Throw)
#  include "msvc_throw_wrapper.h"
#else
#  error "Unknown STL wrapper tactic"
#endif

#pragma warning( pop )

Comment 17

[(no longer active)]

(In reply to comment #15)
> There is no sign of msvc_throw_wrapper.cpp getting used at all. Furthermore, I
> put some junk in msvc_throw_wrapper.h that would break the build if hit, and
> the build went fine with the same warning as always. For that reason, I think
> that this warning is actually coming in from somewhere else.

The compiler output should say where the warnings are coming from, right?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Here's the chain that exception comes in from:

Note: including file:  c:\mozbuild\mozilla-central\objdir-fx\dist\include\nscore.h
Note: including file:   c:\mozbuild\mozilla-central\objdir-fx\dist\include\mozilla/mozalloc.h
Note: including file:    c:\mozbuild\mozilla-central\objdir-fx\dist\stl_wrappers\new
Note: including file:     c:\mozbuild\mozilla-central\objdir-fx\dist\include\mozilla/throw_msvc.h
Note: including file:      c:\mozbuild\mozilla-central\objdir-fx\dist\include\mozilla\msvc_raise_wrappers.h
Note: including file:       c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\INCLUDE\xutility
Note: including file:        c:\mozbuild\mozilla-central\objdir-fx\dist\stl_wrappers\climits
Note: including file:         c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\INCLUDE\new
Note: including file:          c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\INCLUDE\exception

Comment 19

[Masatoshi Kimura [:emk]]

This bug biten me when I'm working on bug 826231.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: patch

This pragma should safe (unless CRT has a bug) because warning-disabled block only includes a system stl header.

Comment 21

[(no longer active)]

Comment on attachment 697470 [details] [diff] [review]
patch

Review of attachment 697470 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/msvc-stl-wrapper.template.h
@@ +62,5 @@
>  
>  // We know that code won't be able to catch exceptions, but that's OK
>  // because we're not throwing them.
>  #pragma warning( push )
> +#pragma warning( disable : 4275 4530 )

Again, please add a comment explaining warning C4275.

Comment 22

[Masatoshi Kimura [:emk]]

Pushed with adding the comment.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e003bb70f9c

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2e003bb70f9c