827032 - Enable FAIL_ON_WARNINGS on MSVC in netwerk/

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 1

[Masatoshi Kimura [:emk]]

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

Sorry, this patch caused a link error.

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Added explicit template instantiation.

Comment 3

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=c3c714b3a50d

Comment 4

[Jason Duell]

Comment on attachment 698385 [details] [diff] [review]
patch v2

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

This is more of a quick drive-by than a formal review for now, but I've got some questions.

::: netwerk/base/src/DashboardTypes.h
@@ +21,5 @@
>  struct DNSCacheEntries
>  {
>      nsCString hostname;
>      nsTArray<nsCString> hostaddr;
> +    uint16_t family;

Why do we need to move from 8 bit to 16 here?

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +695,5 @@
>      , mFD(nullptr)
>      , mFDref(0)
>      , mFDconnected(false)
> +    , mInput(This())
> +    , mOutput(This())

What's the warning here?  Seems very weird we need a method to return 'this' instead of just passing it.

::: netwerk/protocol/http/SpdySession3.cpp
@@ +495,5 @@
>    mInputFrameDataStream = nullptr;
>  }
>  
> +template<typename T> void
> +SpdySession3::EnsureBuffer(nsAutoArrayPtr<T> &buf,

What's the issue with ensureBuffer that needs to be fixed? I.e. what's the warning here?

::: netwerk/protocol/http/SpdyStream3.cpp
@@ +64,5 @@
>  
>    LOG3(("SpdyStream3::SpdyStream3 %p", this));
>  
>    mRemoteWindow = spdySession->GetServerInitialWindow();
> +  mTxInlineFrame = new uint8_t[mTxInlineFrameSize];

I'll want :mcmanus to look over this change.  Maybe make a patch that splits out the SPDY stuff and assign him that review.

::: netwerk/protocol/http/nsHttpAuthCache.cpp
@@ +53,2 @@
>  {
> +    mObserver = new AppDataClearObserver(this);

I guess this is the same weird issue with msvc not liking constructors in construction lists?

Comment 5

[Masatoshi Kimura [:emk]]

(In reply to Jason Duell (:jduell) from comment #4)
> ::: netwerk/base/src/DashboardTypes.h
> @@ +21,5 @@
> >  struct DNSCacheEntries
> >  {
> >      nsCString hostname;
> >      nsTArray<nsCString> hostaddr;
> > +    uint16_t family;
> 
> Why do we need to move from 8 bit to 16 here?

Because nsHostRecord (and every other places in the tree) defines address family as uint_16. MSVC complains about the discrepancy when compiling CacheEntryEnumerator in nsHostResolver.cpp.
This change doesn't increase the structure size because DNSCacheEntries has very inefficient layout :)

> ::: netwerk/base/src/nsSocketTransport2.cpp
> @@ +695,5 @@
> >      , mFD(nullptr)
> >      , mFDref(0)
> >      , mFDconnected(false)
> > +    , mInput(This())
> > +    , mOutput(This())
> 
> What's the warning here?  Seems very weird we need a method to return 'this'
> instead of just passing it.

MSVC warns C4355 if "this" pointer is used in the initializer list.
http://msdn.microsoft.com/en-us/library/3c594ae3%28v=vs.80%29.aspx
Another workaround is #pragma's written in bug 826959.

> ::: netwerk/protocol/http/SpdySession3.cpp
> @@ +495,5 @@
> >    mInputFrameDataStream = nullptr;
> >  }
> >  
> > +template<typename T> void
> > +SpdySession3::EnsureBuffer(nsAutoArrayPtr<T> &buf,
> 
> What's the issue with ensureBuffer that needs to be fixed? I.e. what's the
> warning here?

MSVC warns at the following line in SpdyStream3::ParseHttpRequestHeaders if mTxInlineFrame is the array of (signed) char:
>    mTxInlineFrame[16] = 7 << 5;
Indeed 7 << 5 (= 0xF0) is out of the range of (signed) char.
I couldn't suppress the warning using the cast such as:
    mTxInlineFrame[16] = static_cast<char>(7 << 5);

> ::: netwerk/protocol/http/SpdyStream3.cpp
> @@ +64,5 @@
> >  
> >    LOG3(("SpdyStream3::SpdyStream3 %p", this));
> >  
> >    mRemoteWindow = spdySession->GetServerInitialWindow();
> > +  mTxInlineFrame = new uint8_t[mTxInlineFrameSize];
> 
> I'll want :mcmanus to look over this change.  Maybe make a patch that splits
> out the SPDY stuff and assign him that review.

If I could somehow suppress the above warning, SPDY changes would not be needed.

> ::: netwerk/protocol/http/nsHttpAuthCache.cpp
> @@ +53,2 @@
> >  {
> > +    mObserver = new AppDataClearObserver(this);
> 
> I guess this is the same weird issue with msvc not liking constructors in
> construction lists?

Yes. the difference is mObserver had a default constructor, so I could move the initialization in the function body.

Comment 6

[Jason Duell]

> Because nsHostRecord (and every other places in the tree) defines address 
> family as uint_16

OK, fine.

> MSVC warns C4355 if "this" pointer is used in the initializer list.

We should simply disable this warning.  Ideally we should do it for all of Mozilla, using a compiler flag (sounds like '/ignore:4355' might do it).  I suggest we open a separate bug just for that--I'm not sure who to ask for review on msvc compiler flag changes (ask on #developers).   If for some reason that doesn't get accepted (it ought to: apparently the msvc team is disabling this warning anyway for newer vc++ versions:  see http://connect.microsoft.com/VisualStudio/feedback/details/718050 ), then let's use this pragma in each file instead of a This() method:

  #pragma warning(disable : 4355)

> MSVC warns at the following line in SpdyStream3::ParseHttpRequestHeaders...
> Indeed 7 << 5 (= 0xF0) is out of the range of (signed) char.

Sounds like we should run this by :mcmanus then--might be a bug.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Jason Duell (:jduell) from comment #6)
> We should simply disable this warning.  Ideally we should do it for all of
> Mozilla, using a compiler flag (sounds like '/ignore:4355' might do it).  I
> suggest we open a separate bug just for that--I'm not sure who to ask for
> review on msvc compiler flag changes (ask on #developers).   If for some
> reason that doesn't get accepted (it ought to: apparently the msvc team is
> disabling this warning anyway for newer vc++ versions:  see
> http://connect.microsoft.com/VisualStudio/feedback/details/718050 ), then
> let's use this pragma in each file instead of a This() method:
> 
>   #pragma warning(disable : 4355)

OK, I have repurposed bug 826959.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jason Duell (:jduell) from comment #6)
> > MSVC warns C4355 if "this" pointer is used in the initializer list.
> 
> We should simply disable this warning.

I still think this is a useful warning. And, it is easy to work around when it is spurious:

#include "base/compiler_specific.h"
[...]
Whatever::Whatever()
  : ALLOW_THIS_IN_INITIALIZER_LIST(this)
  {
  }

>   #pragma warning(disable : 4355)

Please use the ALLOW_THIS_IN_INITIALIZER_LIST macro instead of the pragma, as it is more self-documenting.

> > MSVC warns at the following line in SpdyStream3::ParseHttpRequestHeaders...
> > Indeed 7 << 5 (= 0xF0) is out of the range of (signed) char.
> 
> Sounds like we should run this by :mcmanus then--might be a bug.

Yep. Please file a bug.

Comment 9

[Masatoshi Kimura [:emk]]

(In reply to Brian Smith (:bsmith) from comment #8)
> (In reply to Jason Duell (:jduell) from comment #6)
> > Sounds like we should run this by :mcmanus then--might be a bug.
> 
> Yep. Please file a bug.

Filed bug 827824.

Comment 10

[Masatoshi Kimura [:emk]]

(In reply to Brian Smith (:bsmith) from comment #8)
> #include "base/compiler_specific.h"
> [...]
> Whatever::Whatever()
>   : ALLOW_THIS_IN_INITIALIZER_LIST(this)
>   {
>   }
> 
> >   #pragma warning(disable : 4355)
> 
> Please use the ALLOW_THIS_IN_INITIALIZER_LIST macro instead of the pragma,
> as it is more self-documenting.

How do I define ALLOW_THIS_IN_INITIALIZER_LIST()?
#define ALLOW_THIS_IN_INITIALIZER_LIST(...) \
#pragma warning(push) \
#pragma warning(disable : 4355) \
  ...     \
#pragma warning(pop)
is not possible.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Masatoshi Kimura [:emk] from comment #10)
> (In reply to Brian Smith (:bsmith) from comment #8)
> > #include "base/compiler_specific.h"
> > [...]
> > Whatever::Whatever()
> >   : ALLOW_THIS_IN_INITIALIZER_LIST(this)
> >   {
> >   }
> > 
> > >   #pragma warning(disable : 4355)
> > 
> > Please use the ALLOW_THIS_IN_INITIALIZER_LIST macro instead of the pragma,
> > as it is more self-documenting.
> 
> How do I define ALLOW_THIS_IN_INITIALIZER_LIST()?

just #include "base/compiler_specific.h" like in my example. (This macro actually come from Chromium. We should probably put a copy in MFBT.)

Comment 12

[Jason Duell]

> Please use the ALLOW_THIS_IN_INITIALIZER_LIST macro instead of the pragma

OK, I'm fine with that as a workaround should bug 826959 get shot down.

Comment 13

[Jason Duell]

Actually go ahead and use ALLOW_THIS_IN_INITIALIZER_LIST so we can get this landed, and bug 826959 can remove all uses of it in the tree if we go that route.

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Brian Smith (:bsmith) from comment #11)
> just #include "base/compiler_specific.h" like in my example. (This macro
> actually come from Chromium. We should probably put a copy in MFBT.)

Oh, I'd never thought "base/compiler_specific.h" would be the real header name. Thanks.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: patch v3

Comment 16

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=e70ce4bfab29

Comment 17

[Ed Morley [:emorley]]

Congrats on top 3!
http://people.mozilla.org/~catlee/highscores/highscores.html

(Bug 829932)

Comment 18

[Ed Morley [:emorley]]

:-)

Comment 19

[Jason Duell]

Comment on attachment 699260 [details] [diff] [review]
patch v3

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

Thanks Masatoshi!

Comment 20

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee3745570f7

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ee3745570f7