947235 - linker errors from LIR-Common.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Ali Ak]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Steps to reproduce:

Pulled, ./mach clobber, ./mach build


Actual results:

Linker errors, undefined references to LCallDirectEvalS::ThisValue in Lowering.cpp:601


Expected results:

Clean link

Comment 1

[Ali Ak]

Attachment: Fix linker errors in patch for bug 942549

The patch for bug 942549 introduced const static integral
members that do not have a definition and are used where an
integral constant expression is not required. Standard seems
a bit fuzzy on initialization and definition of static const
integral memebr variables :/

Comment 2

[Ali Ak]

Attachment: Fix linker errors in patch for bug 942549

The patch for bug 942549 introduced const static integral
members that do not have a definition and are used where an
integral constant expression is not required. Standard seems
to indicate that a definition is required (section 9.4.2
paragraph 3 - c++0x final draft n3092), so some compilers
could potentially not link

Comment 3

[Shu-yu Guo [:shu]]

How comes this isn't needed for all the other LIRs with |static const size_t foo|?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #3)
> How comes this isn't needed for all the other LIRs with |static const size_t
> foo|?

The problem is the ternary here:

    if (!useBoxAtStart(lir, (string->type() == MIRType_String
                             ? LCallDirectEvalS::ThisValue
                             : LCallDirectEvalV::ThisValue),
                       thisValue))

I had the same problem in bug 906040 and Luke wrote in comment 6 there:

> I'm pretty sure the problem is this long-standing GCC quirk that it emits
> references to static const varaibles when they are used in a ternary
> operator (which you write "static const X = 1", this is a declaration and
> initialization, but not a *definition* which means, should a symbol ever be
> emitted for X, it'll be a link error, but mostly symbols never get emitted
> since the whole point is that they're inlined).  The usual fix is either to
> add a definition or take out the ternary operator.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8343778 [details] [diff] [review]
Fix linker errors in patch for bug 942549

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

::: js/src/jit/LIR.cpp
@@ +19,5 @@
>  
> +// From LIR-Common.h
> +const size_t LCallDirectEvalS::ThisValue;
> +const size_t LCallDirectEvalV::ThisValue;
> +const size_t LCallDirectEvalV::Argument;

Per Jan's comment #4, could you just change the ternary operator in |LIRGenerator::visitCallDirectEval| that uses if?

Thanks!

Comment 6

[Shu-yu Guo [:shu]]

Err, I meant to say, change the ternary operator *to* use if.

Comment 7

[(no longer active)]

If we used an enum instead of a static const, we would probably future proof it.

Comment 8

[Shu-yu Guo [:shu]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7)
> If we used an enum instead of a static const, we would probably future proof
> it.

Not a bad idea, I'll change all the LIRs wholesale.

Comment 9

[Nigel Stewart]

Observed this same link failure for both Android and Linux builds of mozilla-central master branch on Ubuntu 12.04.
Release-mode builds don't seem affected.
The proposed patch looks good to me, on face value.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My build is broken too. Can we please just unbreak the build before we start talking about wholesale conversions?

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm borked too.

Comment 12

[Jan de Mooij [:jandem]]

Pushed a simple fix. Let me know if this doesn't work:

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

(Yes, GGC should've been GCC...)

Comment 13

[Ali Ak]

Yep an enum will of course work too, so will an if, but for future reference I'd advise against an if for two reasons:

1) Using an if does not guarantee you won't run in to the same problem again; the standard does not say the compiler must not generate a symbol - so a standard conformant compiler is still free to generate a symbol for any static const integral that's used where a constant expression is not required (mandatory insertion of AFAIK :p)

2) By using an if we would have to introduce an extra variable - not a big deal, but I guess not necessary - or we would have similar calling code within both branches of the if (also ok but not as clean as the ternary)

So that being said, and with the other patch in. What's the process now? Close this? - I'm very new to mozilla so I'm not very familiar with procedure.

Comment 14

[Shu-yu Guo [:shu]]

Jan, thanks for pushing the workaround.

(In reply to Ali Ak from comment #13)

> So that being said, and with the other patch in. What's the process now?
> Close this? - I'm very new to mozilla so I'm not very familiar with
> procedure.

No, don't close this (see the [leave open] whiteboard). Jan pushed a temporary fix so that people can compile on gcc without problems until the more long-term fix of using enums is ready.

Would you like to work on this patch? Should be straightforward to convert all the |static const size_t foo|s in the various LIR headers to be anonymous enums. If so, assign the bug to yourself and r? me afterwards.

Thanks!

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[Nigel Stewart]

Thumbs up for the workaround in mozilla-central.
Works for me - both Android and desktop Linux 64-bit.
Thanks!

- Nigel

Comment 17

[Ali Ak]

(In reply to Shu-yu Guo [:shu] from comment #14)
> Would you like to work on this patch? Should be straightforward to convert
> all the |static const size_t foo|s in the various LIR headers to be
> anonymous enums. If so, assign the bug to yourself and r? me afterwards.
> 
> Thanks!

No probs, understood. No not at the moment. I could take it up when I have some extra time though. In the meantime if anyone else feels like doing this then go for it.

Comment 19

[Hannes Verschore [:h4writer]]

Opened bug 1307733 for the long-term fix of using enums and closing this bug.