Closed Bug 947235 Opened 9 years ago Closed 6 years ago

linker errors from LIR-Common.h

Categories

(Core :: JavaScript Engine: JIT, defect)

28 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ali, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

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
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 :/
Attachment #8343761 - Flags: review?(shu)
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
Attachment #8343761 - Attachment is obsolete: true
Attachment #8343761 - Flags: review?(shu)
Attachment #8343778 - Flags: review?(shu)
How comes this isn't needed for all the other LIRs with |static const size_t foo|?
(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 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!
Attachment #8343778 - Flags: review?(shu)
Err, I meant to say, change the ternary operator *to* use if.
If we used an enum instead of a static const, we would probably future proof it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
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.
My build is broken too. Can we please just unbreak the build before we start talking about wholesale conversions?
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...)
Whiteboard: [leave open]
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.
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!
Thumbs up for the workaround in mozilla-central.
Works for me - both Android and desktop Linux 64-bit.
Thanks!

- Nigel
(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.
Duplicate of this bug: 947642
Opened bug 1307733 for the long-term fix of using enums and closing this bug.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.