822866 - Can't build using MSVC with enable-optimize=-O2 since landing of bug 813445

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Bill Gianopoulos [:WG9s]]

I cannot build using VC9 since the landing of bug 823445 without reverting to O1 optimization.

This is obviously a compiler bug though and unlike most of my other such bugs which are trying to use features not supported by VC9.

Since compiling with O1 is a workaround, I am not setting the severity to blocker as is my normal custom for such bugs.

Comment 1

[Bill Gianopoulos [:WG9s]]

Attachment: relevant portion of log

Comment 2

[Bill Gianopoulos [:WG9s]]

Oh BTW I am fine with the answer being that starting with version 20, VC9 is no longer a supported build platform.  I plan to redo my builds to use VC10 over XMAS vacation form work.

Comment 3

[(not currently active) Ted Mielczarek]

Can you try putting a #pragma optimize in the file that changed, perhaps #ifdef'ed to target MSVC9 or lower? I'd hate to unsupport VC2008 for something like this, but I also don't want to jump through hoops to support it.

Comment 4

[Bill Gianopoulos [:WG9s]]

Attachment: Patch

This seems to workaround the compiler error for me.

Comment 5

[Bill Gianopoulos [:WG9s]]

Attachment: Patch

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 695245 [details] [diff] [review]
Patch

>+// Workaround Microsoft VC9 internal compiler error.  See bug 822866.
>+// #if defined (_MSC_VER) && _MSC_VER == 1500
>+// #pragma optimize("", off)
>+// #endif
Why commented out?

Comment 7

[Bill Gianopoulos [:WG9s]]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> Comment on attachment 695245 [details] [diff] [review]
> Patch
> 
> >+// Workaround Microsoft VC9 internal compiler error.  See bug 822866.
> >+// #if defined (_MSC_VER) && _MSC_VER == 1500
> >+// #pragma optimize("", off)
> >+// #endif
> Why commented out?

I really hate these text editors that think they are smarter than I am ;-)

Comment 8

[Bill Gianopoulos [:WG9s]]

Attachment: Patch

Comment 9

[Bill Gianopoulos [:WG9s]]

Asking for review from roc as widget module owner, and ted for correctness of testing for compiler version.

Comment 10

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #7)
> (In reply to Masatoshi Kimura [:emk] from comment #6)
> > Comment on attachment 695245 [details] [diff] [review]
> > Patch
> > 
> > >+// Workaround Microsoft VC9 internal compiler error.  See bug 822866.
> > >+// #if defined (_MSC_VER) && _MSC_VER == 1500
> > >+// #pragma optimize("", off)
> > >+// #endif
> > Why commented out?
> 
> I really hate these text editors that think they are smarter than I am ;-)

That was a cut and paste.  I did not notice that since the first pasted line was a comment the lame thing decided to make the whole paste a comment.  I need to figure out how to turn this stupid feature off.

Comment 11

[Bill Gianopoulos [:WG9s]]

Comment on attachment 695252 [details] [diff] [review]
Patch

Cancelling review requests.

Builds with this patch complete, but the resultant builds do not work correctly.

Comment 12

[Bill Gianopoulos [:WG9s]]

THis bug as submitted is actaully invalid.  I had been building for so long with -O2 that I had forgotten it is not the default.  Since the current source builds correctly under VC9 using the default optimization I am resoving this as INVALID.

Comment 13

[Bill Gianopoulos [:WG9s]]

Reopening with corrected summary now that I find this issue is not restricted to VC9.  I get the same failure under VC10.

Comment 14

[Bill Gianopoulos [:WG9s]]

RE-TAKING this.  I expect to have a working patch by sometime tomorrow.

Comment 15

[Bill Gianopoulos [:WG9s]]

Oops.

Comment 16

[Bill Gianopoulos [:WG9s]]

Attachment: For reference only

This patch avoids the compiler error, but is unacceptable for the following reasons:

1.  What is really required here is to turn off only the "i" optimization (generates intrinsic functions for appropriate function calls) which is the optimization that triggers this issue.  Since this cannot be turned off individually, there is no way to make O@ builds work without de-optimizing the release builds.

2.  With this patch, the build completes and avoids the fatal compiler error, however the resultant build immediately exits upon any keypress.  I have no idea how to debug this or if debugging this is actually useful.  depends on if fixing whatever the compiler bug is fixes this issue as well or if this is a separate issue.

Comment 17

[Bill Gianopoulos [:WG9s]]

A workaround for this issue is to replace:

  ac_add_options --enable-optimize=-O2

wit:

  ac_add_options --enable-optimize=-O1t

in .mozconfig.

This enables all of the O2 optimizations (with the exception of Oi) which is the one that triggers the compiler bug.

Comment 18

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #16)
> Created attachment 695513 [details] [diff] [review]
> For reference only
> 
> This patch avoids the compiler error, but is unacceptable for the following
> reasons:
> 
> 1.  What is really required here is to turn off only the "i" optimization
> (generates intrinsic functions for appropriate function calls) which is the
> optimization that triggers this issue.  Since this cannot be turned off
> individually, there is no way to make O@ builds work without de-optimizing
                                        ^^
                                        O2
> the release builds.

Comment 19

[Masatoshi Kimura [:emk]]

Hm. In C++03, EventFlags is not a POD class because it has a non-trivial constructor. memcpy()'ing non-POD class is an undefined behavior.
(In C++11, EventFlags is a trivially-copyable class.)
Given that MSVC9 (and even MSVC10) is not a C++11 compiler, we shouldn't blame the compilers. That is our fault.
Perhaps we will have to abandon the idea using bitfields.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masatoshi Kimura [:emk] from comment #19)
> Perhaps we will have to abandon the idea using bitfields.

I don't think so if the rawFlags approach is the cause. That is only used for the trick for better performance and easier maintaining. If it's important to be able to build opt build with VC9, we should rewrite the methods in EventFlags.

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> (In reply to Masatoshi Kimura [:emk] from comment #19)
> > Perhaps we will have to abandon the idea using bitfields.
> 
> I don't think so if the rawFlags approach is the cause. That is only used
> for the trick for better performance and easier maintaining.

I'm saying not because it doesn't compile on MSVC9. It's very fragile to depend on the undefined behavior. It may be broken by the compiler update. It may be even broken by the changing options. (and according to comment #13, it's actually broken.)

> If it's
> important to be able to build opt build with VC9, we should rewrite the
> methods in EventFlags.

If the compiler is enough smart to optimize such new methods, that's fine.

An alternative option would be using mozilla::EnumSet.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The | operator's performance isn't important due to the usage count.

On the other hand, the Clear() method is shared with constructor. So, this performance is important. But I think that if we make a static instance whose all bits are already cleared and copy with default copy constructor, then, it must be fast.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Er, Kimura-san, do you know if the ParamTraits<EventFlags> is safe?

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If it's not safe due to non-POD class, we should remove the constructor and make it a POD class.

Comment 25

[Masatoshi Kimura [:emk]]

ParamTraits could be rewritten to something like:
  static void Write(Message* aMsg, const paramType& aParam)
  {
    WriteParam(aMsg, aParam.GetRawFlags());
  }

  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
  {
    uint32_t rawFlags;
    if (!ReadParam(aMsg, aIter, &rawFlags) {
      return false;
    }
    aResult->SetRawFlags(rawFlags);
    return true;
  }
Sorry for my inappropriate comments in the previous bug.

Comment 26

[Masatoshi Kimura [:emk]]

Of course, making EventFlags POD-class would also work.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Could you check this patch to fix this bug?

I don't see performance difference.

patched build     unpatched build

win
7098              7461
7138              7501
7582              7794
7430              7269
7849              7258
8048              7668
7936              7776
7407              7309
7607              7608
8014              7211

Mac
4179              4273
4083              4113
4647              4077
4200              4127
4164              4092
4083              4046
4097              4111
4078              4065
4164              4032
4171              4106

Comment 28

[Bill Gianopoulos [:WG9s]]

Comment on attachment 695733 [details] [diff] [review]
Patch

I tried building under VC10 with this and got the same compiler error as before.

Comment 29

[Bill Gianopoulos [:WG9s]]

Attachment: log snippet with new patch

This is the log snippet of relevant portion under VC10 with this patch applied.

Comment 30

[Bill Gianopoulos [:WG9s]]

>  // WARNING: For VC compilers, this class must have neither constructor,
>  // destructor, virtual methods and operators due to safe for memcpy.  I.e.,
>  // this has to be a non-POD class in C++03.
>  inline void Clear()
>  {
>    SetRawFlags(0);
>  }
>  // Get if either the instance's bit or the aOther's bit is true, the
>  // instance's bit becomes true.  In other words, this works like:
>  // eventFlags |= aOther;
>  inline void Union(const EventFlags& aOther)
>  {
    SetRawFlags(GetRawFlags() | aOther.GetRawFlags());


The above line is line 558 where the error occurs.

Comment 31

[Bill Gianopoulos [:WG9s]]

I ran another build and applying both your patch and my previous patch to not optimize in a portion of nsGUIEvent.h still results in avoiding the internal compiler error, but a browser that exits unceremoniously upon the first keypress.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for your test. But it's odd why the #558 causes failing to build...

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

This patch works on my environment. Could you check it?

It seems that VC compiler fails to handle inline methods and if it's used for reference argument.

Comment 34

[Bill Gianopoulos [:WG9s]]

Comment on attachment 695928 [details] [diff] [review]
Patch

This both compiles and seems to work correctly under both VC9 and VC10.  Great job!

Comment 35

[Bill Gianopoulos [:WG9s]]

My nightly builds at http://www.wg9s.com/mozilla/firefox/ now include this patch.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 695928 [details] [diff] [review]
Patch

Bill, thank you for your test!

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 695928 [details] [diff] [review]
Patch

Uh, requirement to have Clear() everywhere is ugly and _very_ error prone.
Is that really the only option?
(Union() is ok, IMO.)

If nothing else works, perhaps the current EventFlags could be
EventFlagsInternal and used only by nsEvent and a new EventFlags.
EventFlags would be a wrapper around EventFlagsInternal and call
Clear() automatically.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

How about this?

I don't like the dispatcher method callers can change *any* flags. So, I think that it makes sense to make new ExtraEventFlags struct for them.

Note that this patch looks we can get red of the memcpy and make the constructor. However, we still need to make EventFlags a POD class for ParamTraits.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

hmm. Would it work if you had
BaseEventFlags without ctor/dtor, but with Clear()
and then EventFlags : public BaseEventFlags
which had ctor/dtor

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Testing on tryserver...

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 696210 [details] [diff] [review]
Patch

I wonder why operator |= wouldn't have worked just fine.
But Union is ok too

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #42)
> Comment on attachment 696210 [details] [diff] [review]
> Patch
> 
> I wonder why operator |= wouldn't have worked just fine.
> But Union is ok too

I understand as POD class cannot have |= operator.
http://en.wikipedia.org/wiki/Plain_Old_Data_Structures

If I misunderstood, let me know. I'll file follow up patch. For now, I'll land the patch soon.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1faf3072fb8b

Comment 45

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/1faf3072fb8b