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

RESOLVED FIXED in mozilla20

Status

()

Core
Widget
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: WG9s, Assigned: masayuki)

Tracking

({regression})

Trunk
mozilla20
x86
Windows 7
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 693660 [details]
relevant portion of log
(Reporter)

Updated

6 years ago
Severity: normal → major
(Reporter)

Comment 2

6 years ago
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.
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.
(Reporter)

Updated

6 years ago
Assignee: nobody → bill
(Reporter)

Comment 4

6 years ago
Created attachment 695243 [details] [diff] [review]
Patch

This seems to workaround the compiler error for me.
(Reporter)

Comment 5

6 years ago
Created attachment 695245 [details] [diff] [review]
Patch
Attachment #695243 - Attachment is obsolete: true
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?
(Reporter)

Comment 7

6 years ago
(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 ;-)
(Reporter)

Comment 8

6 years ago
Created attachment 695252 [details] [diff] [review]
Patch
Attachment #695245 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #695252 - Flags: review?(roc)
(Reporter)

Updated

6 years ago
Attachment #695252 - Flags: review?(ted)
(Reporter)

Comment 9

6 years ago
Asking for review from roc as widget module owner, and ted for correctness of testing for compiler version.
(Reporter)

Comment 10

6 years ago
(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.
(Reporter)

Updated

6 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 11

6 years ago
Comment on attachment 695252 [details] [diff] [review]
Patch

Cancelling review requests.

Builds with this patch complete, but the resultant builds do not work correctly.
Attachment #695252 - Flags: review?(ted)
Attachment #695252 - Flags: review?(roc)
(Reporter)

Updated

6 years ago
Attachment #695252 - Attachment is obsolete: true
(Reporter)

Comment 12

6 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
(Reporter)

Comment 13

6 years ago
Reopening with corrected summary now that I find this issue is not restricted to VC9.  I get the same failure under VC10.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: Can't build using VC9 with default optimization since landing of bug 813445 → Can't build using MSVC with enable=optimize=-O2 since landing of bug 813445
(Reporter)

Updated

6 years ago
Assignee: bill → nobody
Status: REOPENED → NEW
(Reporter)

Comment 14

6 years ago
RE-TAKING this.  I expect to have a working patch by sometime tomorrow.
Status: NEW → ASSIGNED
QA Contact: bill
(Reporter)

Updated

6 years ago
Summary: Can't build using MSVC with enable=optimize=-O2 since landing of bug 813445 → Can't build using MSVC with enable-optimize=-O2 since landing of bug 813445
(Reporter)

Comment 15

6 years ago
Oops.
Assignee: nobody → bill
(Reporter)

Updated

6 years ago
QA Contact: bill
(Reporter)

Comment 16

6 years ago
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 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.
(Reporter)

Comment 17

6 years ago
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.
Assignee: bill → nobody
(Reporter)

Updated

6 years ago
Status: ASSIGNED → NEW
Whiteboard: see workaround in comment 17
(Reporter)

Comment 18

6 years ago
(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.
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.
(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.
(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.
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Er, Kimura-san, do you know if the ParamTraits<EventFlags> is safe?
If it's not safe due to non-POD class, we should remove the constructor and make it a POD class.
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.
Of course, making EventFlags POD-class would also work.
Created attachment 695733 [details] [diff] [review]
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
Attachment #695733 - Flags: feedback?(bill)
Flags: needinfo?(bill)
(Reporter)

Comment 28

6 years ago
Comment on attachment 695733 [details] [diff] [review]
Patch

I tried building under VC10 with this and got the same compiler error as before.
Attachment #695733 - Flags: feedback?(bill) → feedback-
Flags: needinfo?(bill)
(Reporter)

Comment 29

6 years ago
Created attachment 695791 [details]
log snippet with new patch

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

Comment 30

6 years ago
>  // 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.
(Reporter)

Comment 31

6 years ago
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.
Thank you for your test. But it's odd why the #558 causes failing to build...
Created attachment 695928 [details] [diff] [review]
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.
Attachment #695733 - Attachment is obsolete: true
Attachment #695928 - Flags: feedback?(bill)
(Reporter)

Updated

6 years ago
Attachment #695513 - Attachment is obsolete: true
(Reporter)

Comment 34

6 years ago
Comment on attachment 695928 [details] [diff] [review]
Patch

This both compiles and seems to work correctly under both VC9 and VC10.  Great job!
Attachment #695928 - Flags: feedback?(bill) → feedback+
(Reporter)

Comment 35

6 years ago
My nightly builds at http://www.wg9s.com/mozilla/firefox/ now include this patch.
Comment on attachment 695928 [details] [diff] [review]
Patch

Bill, thank you for your test!
Attachment #695928 - Flags: review?(bugs)
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.
Attachment #695928 - Flags: review?(bugs) → review-
(Reporter)

Updated

6 years ago
Whiteboard: see workaround in comment 17
Created attachment 696176 [details] [diff] [review]
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.
Attachment #695928 - Attachment is obsolete: true
Attachment #696176 - Flags: review?(bugs)
hmm. Would it work if you had
BaseEventFlags without ctor/dtor, but with Clear()
and then EventFlags : public BaseEventFlags
which had ctor/dtor
Created attachment 696204 [details] [diff] [review]
Patch

Testing on tryserver...
Attachment #696176 - Attachment is obsolete: true
Attachment #696176 - Flags: review?(bugs)
Created attachment 696210 [details] [diff] [review]
Patch
Attachment #696204 - Attachment is obsolete: true
Attachment #696210 - Flags: review?(bugs)
Comment on attachment 696210 [details] [diff] [review]
Patch

I wonder why operator |= wouldn't have worked just fine.
But Union is ok too
Attachment #696210 - Flags: review?(bugs) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/1faf3072fb8b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.