Mutable flag isn't clear on VC++ x64

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
x86_64
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 396935 [details] [diff] [review]
patch v1

By bug 501324, we added a workaround for VC++ x64.  But Mutable flag isn't clear well even if calling flatClearMutable().

We seem to need cast code for VC++ x64.
(Assignee)

Comment 1

9 years ago
Created attachment 397791 [details] [diff] [review]
patch v1.1
Assignee: general → m_kato
Attachment #396935 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #397791 - Flags: review?(jwalden+bmo)

Comment 2

9 years ago
Scary. Why is that cast needed?

We specifically have some ugly code in there to tell msvc to use an size_t enum. Maybe we shouldn't be using an enum here?

    enum
#if defined(_MSC_VER) && defined(_WIN64)
    : size_t /* VC++ 64-bit incorrectly defaults this enum's size to int. */
#endif
Comment on attachment 397791 [details] [diff] [review]
patch v1.1

Style should be ~size_t(MUTABLE) if needed, gal asks the right question.

/be
(Assignee)

Comment 4

9 years ago
(In reply to comment #2)
> Scary. Why is that cast needed?

compiler bug.  Even if enum : size_t, it sometimes recognize as int type on VC++.

Comment 5

9 years ago
Luke, save us. What else could we use here instead of an enum and still make it look pretty and not increase the object size. Would static const size_t members work? (instead of the enum).

Comment 6

9 years ago
I think you're right about static const size_t, especially since the enum-y features of the enum aren't being used.

Also, its technically not a bug that 64-bit MSVC picks that enums are ints.  It even makes a bit of sense since its not fun when all your enum-using data structures (which clearly already fit in 32-bits) suddenly double in size.  As usual, and of no help to us here, C++0x standardizes the solution (http://www.research.att.com/~bs/C++0xFAQ.html#enum) by letting one declare the underlying type rather like the MSVC hack is attempting to do.
(Assignee)

Comment 7

9 years ago
Created attachment 398101 [details] [diff] [review]
patch v2

update by comments
Attachment #397791 - Attachment is obsolete: true
Attachment #398101 - Flags: review?(jwalden+bmo)
Attachment #397791 - Flags: review?(jwalden+bmo)

Comment 8

9 years ago
Comment on attachment 398101 [details] [diff] [review]
patch v2

I would really much prefer a proper fix (no longer (ab-)using an enum).
Attachment #398101 - Flags: review-
Comment on attachment 398101 [details] [diff] [review]
patch v2

I think I agree with Andreas.  We've whacked a handful of moles already in this code to work around MSVC++64 suckage, and they just won't stay down -- strategic reassessment recommends static const size_t.
Attachment #398101 - Flags: review?(jwalden+bmo) → review-
Created attachment 401603 [details] [diff] [review]
get rid of enums
Attachment #398101 - Attachment is obsolete: true
Attachment #401603 - Flags: review?
Attachment #401603 - Flags: review? → review?(jwalden+bmo)
Attachment #401603 - Flags: review?(jwalden+bmo) → review+
Well I'd like to land this, but uh... it won't build on Apple gcc-4.2.

Undefined symbols:
  "JSString::LENGTH_MASK", referenced from:
      JSString::dependentLength() const in jsapi.o
      JSString::dependentLength() const in jsarray.o
      JSString::dependentLength() const in jsatom.o
      JSString::dependentLength() const in jsbool.o
      JSString::dependentLength() const in jsdate.o
      JSString::dependentLength() const in jsexn.o
      JSString::dependentLength() const in jsfun.o
      JSString::dependentLength() const in jsinterp.o
      JSString::dependentLength() const in jsnum.o
      JSString::dependentLength() const in jsobj.o
      JSString::dependentLength() const in json.o
      JSString::dependentLength() const in jsopcode.o
      JSString::dependentLength() const in jsparse.o
      JSString::dependentLength() const in jsregexp.o
      JSString::dependentLength() const in jsscan.o
      JSString::dependentLength() const in jsstr.o
      JSString::dependentLength() const in jsxdrapi.o
      JSString::dependentLength() const in jsxml.o
      JSString::dependentLength() const in jstracer.o
      JSString::dependentLength() const in jsbuiltins.o
  "JSString::DEPENDENT_LENGTH_MASK", referenced from:
      JSString::dependentLength() const in jsapi.o
      JSString::dependentLength() const in jsarray.o
      JSString::dependentLength() const in jsatom.o
      JSString::dependentLength() const in jsbool.o
      JSString::dependentLength() const in jsdate.o
      JSString::dependentLength() const in jsexn.o
      JSString::dependentLength() const in jsfun.o
      JSString::dependentLength() const in jsinterp.o
      JSString::dependentLength() const in jsnum.o
      JSString::dependentLength() const in jsobj.o
      JSString::dependentLength() const in json.o
      JSString::dependentLength() const in jsopcode.o
      JSString::dependentLength() const in jsparse.o
      JSString::dependentLength() const in jsregexp.o
      JSString::dependentLength() const in jsscan.o
      JSString::dependentLength() const in jsstr.o
      JSString::dependentLength() const in jsxdrapi.o
      JSString::dependentLength() const in jsxml.o
      JSString::dependentLength() const in jstracer.o
      JSString::dependentLength() const in jsbuiltins.o

Luke and I played with this for 10-15 minutes and could find neither rhyme nor reason. It's not a terribly important patch so I'll play with it more when I have time.
Have you tried rebuilding (incl. configure) from scratch?  I think I've seen such errors go away by doing that.
Unfortunately that didn't work.
(Assignee)

Updated

8 years ago
Blocks: 471090
(Assignee)

Comment 14

8 years ago
Humm, linux gcc-4.3.3 on try server seems to throw unexpected error during linking process...

/tools/gcc-4.3.3/installed/bin/g++  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Wno-long-long -pedantic -gstabs+ -fno-strict-aliasing -pthread -pipe  -DNDEBUG -DTRIMMED -O3 -fstrict-aliasing  -fPIC -shared -Wl,-z,defs -Wl,-h,libmozjs.so -o libmozjs.so  jsapi.o jsarena.o jsarray.o jsatom.o jsbool.o jscntxt.o jsdate.o jsdbgapi.o jsdhash.o jsdtoa.o jsemit.o jsexn.o jsfun.o jsgc.o jshash.o jsinterp.o jsinvoke.o jsiter.o jslock.o jslog2.o jsmath.o jsnum.o jsobj.o json.o jsopcode.o jsparse.o jsprf.o jsregexp.o jsscan.o jsscope.o jsscript.o jsstr.o jstask.o jsutil.o jsxdrapi.o jsxml.o prmjtime.o jstracer.o Assembler.o Allocator.o CodeAlloc.o Containers.o Fragmento.o LIR.o RegAlloc.o avmplus.o Nativei386.o jsbuiltins.o VMPI.o     -lpthread   -Wl,-rpath-link,/bin -Wl,-rpath-link,/lib  -L/builds/slave/sendchange-linux-hg/build/objdir/dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl -ldl -lm  -lm -ldl  
jsinterp.o: In function `js_Interpret':
/builds/slave/sendchange-linux-hg/build/js/src/jsstr.h:326: undefined reference to `JSString::LENGTH_MASK'
/builds/slave/sendchange-linux-hg/build/js/src/jsstr.h:326: undefined reference to `JSString::LENGTH_MASK'
/builds/slave/sendchange-linux-hg/build/js/src/jsstr.h:326: undefined reference to `JSString::DEPENDENT_LENGTH_MASK'
/builds/slave/sendchange-linux-hg/build/js/src/jsstr.h:326: undefined reference to `JSString::DEPENDENT_LENGTH_MASK'
/usr/bin/ld: libmozjs.so: hidden symbol `JSString::LENGTH_MASK' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output

Comment 15

8 years ago
I got the same linker error on linux with gcc 4.4.1, but only on debug; opt compiles fine.  And get this, the patch compiles fine with this change to dependentLength:

-        return mLength & (dependentIsPrefix() ? LENGTH_MASK : DEPENDENT_LENGTH_MASK);
+        if (dependentIsPrefix())
+            return mLength & LENGTH_MASK;
+        return mLength & DEPENDENT_LENGTH_MASK

I suspect this is a GCC bug that has to do with the external linkage of LENGTH_MASK and use in the ternary operator.
(In reply to comment #15)

Whoa, thanks! That makes me sad, but great find. Now I can land this.
Anyone up for filing a gcc.gnu.org bug report?

/be

Comment 19

8 years ago
I gnu someone would say that...

Comment 21

8 years ago
So, as pointed out in the bug:

class C {
  static const size_t T = 1;
};

is declaration and an initialization, but *not* a definition.  Hence, the static value of T can be used in constant expressions, but should it ever *not* be inlined and an external symbol generated (as apparently was happending with dependentLength), there will be no definition to link to, hence the linker error.  The fix is to insert a *non-initializing* out-of-body definition in jsstr.cpp.  Fun!

Comment 22

8 years ago
http://hg.mozilla.org/mozilla-central/rev/dd207825c164
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Further fix coming?

/be

Comment 24

8 years ago
Created attachment 413388 [details] [diff] [review]
define the undefined constants

This fixes the problem, keeping the expression the same.  It's still weird that a symbol reference is being emitted at all.
You need to log in before you can comment on or make changes to this bug.