If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Enable FAIL_ON_WARNINGS on MSVC in netwerk/

RESOLVED FIXED in mozilla21

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 698306 [details] [diff] [review]
patch
Attachment #698306 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Blocks: 187528
(Assignee)

Comment 1

5 years ago
Comment on attachment 698306 [details] [diff] [review]
patch

Sorry, this patch caused a link error.
Attachment #698306 - Attachment is obsolete: true
Attachment #698306 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 2

5 years ago
Created attachment 698385 [details] [diff] [review]
patch v2

Added explicit template instantiation.
Attachment #698385 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 3

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c3c714b3a50d
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?
Attachment #698385 - Flags: review?(jduell.mcbugs) → feedback+
(Assignee)

Comment 5

5 years ago
(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.
> 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.
(Assignee)

Comment 7

5 years ago
(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.
(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.
(Assignee)

Updated

5 years ago
Depends on: 827824
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 826959
(Assignee)

Comment 10

5 years ago
(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.
(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.)
> 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.
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.
No longer depends on: 826959
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
Created attachment 699260 [details] [diff] [review]
patch v3
Attachment #698385 - Attachment is obsolete: true
Attachment #699260 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 16

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=e70ce4bfab29
Congrats on top 3!
http://people.mozilla.org/~catlee/highscores/highscores.html

(Bug 829932)
:-)
Comment on attachment 699260 [details] [diff] [review]
patch v3

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

Thanks Masatoshi!
Attachment #699260 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee3745570f7
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/8ee3745570f7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.