512866 - Mutable flag isn't clear on VC++ x64

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Makoto Kato [:m_kato]]

Attachment: 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.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: patch v1.1

Comment 2

[Andreas Gal :gal]

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 3

[Brendan Eich [:brendan]]

Comment on attachment 397791 [details] [diff] [review]
patch v1.1

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

/be

Comment 4

[Makoto Kato [:m_kato]]

(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

[Andreas Gal :gal]

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

[Luke Wagner [:luke]]

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.

Comment 7

[Makoto Kato [:m_kato]]

Attachment: patch v2

update by comments

Comment 8

[Andreas Gal :gal]

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

I would really much prefer a proper fix (no longer (ab-)using an enum).

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: get rid of enums

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 12

[Nicholas Nethercote [inactive]]

Have you tried rebuilding (incl. configure) from scratch?  I think I've seen such errors go away by doing that.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Unfortunately that didn't work.

Comment 14

[Makoto Kato [:m_kato]]

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

[Luke Wagner [:luke]]

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.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #15)

Whoa, thanks! That makes me sad, but great find. Now I can land this.

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/dd207825c164

Comment 18

[Brendan Eich [:brendan]]

Anyone up for filing a gcc.gnu.org bug report?

/be

Comment 19

[Luke Wagner [:luke]]

I gnu someone would say that...

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42101

Comment 21

[Luke Wagner [:luke]]

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

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/dd207825c164

Comment 23

[Brendan Eich [:brendan]]

Further fix coming?

/be

Comment 24

[Luke Wagner [:luke]]

Attachment: 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.