505034 - Compiling Nativei386.cpp on MinGW fails because of different syntax of inline asm.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacek Caban]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
Build Identifier: 

Preprocessor instructions assume that all WIN32 and WIN64 code is compiled using MSVC.

Reproducible: Always

Comment 1

[Jacek Caban]

Attachment: Use GCC asm syntax when building on MinGW.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 389320 [details] [diff] [review]
Use GCC asm syntax when building on MinGW.

Please solicit review from nanojit hackers (hg annotate when in doubt).

/be

Comment 3

[Graydon Hoare :graydon]

Comment on attachment 389320 [details] [diff] [review]
Use GCC asm syntax when building on MinGW.

I'm not sure adding another ifdef here makes sense; maybe change the WIN32 ifdef to _MSC_VER? I mean ... unless you can think of any scenario in which _MSC_VER is defined, and WIN32 isn't. Doesn't seem to fit to me, as written.

(Also, wouldn't we wind up with an empty function in that case, if it existed?)

Comment 4

[Jacek Caban]

(In reply to comment #3)
> (From update of attachment 389320 [details] [diff] [review])
> I'm not sure adding another ifdef here makes sense; maybe change the WIN32
> ifdef to _MSC_VER? I mean ... unless you can think of any scenario in which
> _MSC_VER is defined, and WIN32 isn't. Doesn't seem to fit to me, as written.

Yes, compiling via MinGW is such case. This way we use gcc to compile Windows version, so WIN32 is defined, but _MSV_VER is not. In this case we want to use GCC asm syntax. It's not an official way of compilation but it works except for a few problems like this one and it's supported by building environment. It's not a hypothetical situation, it's a bug I've found when compiling with MinGW and after fixing it and a few other bug I was able to get working Firefox build.

> (Also, wouldn't we wind up with an empty function in that case, if it existed?)

No, we'd use gcc asm syntax.

Comment 5

[Jacek Caban]

Attachment: Fix 2

This is an alternative solution with exactly the same effect. This patch reorders #ifs instead of adding new ones.

Comment 6

[Graydon Hoare :graydon]

Comment on attachment 390444 [details] [diff] [review]
Fix 2

Oh! Terribly sorry, I misunderstood the original patch as there *is* a case of _MSC_VER && !WIN32, namely WIN64. I didn't notice that as it was missing from the first patch context, and I (lazily) did not read the source file in question to see.

In that case either variant is fine. I don't really have a preference; I was just not seeing the 3rd case in the 3-way alternative presented.

Thanks for your patience. Don't suppose you have commit rights, mm? Shall I commit this for you then?

Comment 7

[Jacek Caban]

Yes, please commit it. Thanks.

Comment 8

[Brent Baker]

I notice that the patches are based on TM codebase, should this be moved out of
the "tamarin" product?

Comment 9

[cls]

Attachment: Alternate patch

Here's an alternate patch that doesn't move around the code.

Comment 10

[Jacek Caban]

That's what my first patch did, except that I think it's safer to use _MSC_VER around MSVC assembly syntax (it's not a big deal IMO). Could someone check in any of these patches, please?

Comment 11

[Jacek Caban]

Attachment: Updated patch to current version.

Now that there is no WIN64 code in Nativei386.cpp, the patch is even easier and does exactly the same as similar code in NativeX64.cpp.

Comment 12

[Jacek Caban]

Attachment: Don't use __rdtsc on mingw.

This patch fixes similar problem in avmplus.h. We should use the same implementation of rdtsc on mingw as we use for other GCCs.

Comment 13

[Phil Ringnalda (:philor)]

With the only reviewed patch marked as obsolete, it's not at all clear what's "checkin-needed" here.

Comment 14

[Jacek Caban]

This attachment has identical effect as the reviewed patch, but I wasn't sure if it requires a new review or not:
https://bug505034.bugzilla.mozilla.org/attachment.cgi?id=405463
Graydon, could you please review it?

Also the rdtsc patch is no longer needed.

Comment 15

[Jacek Caban]

Attachment: rebased fix

This is the patch rebased against current version. I assume that a new review is not needed.

Comment 16

[Jacek Caban]

Can this be patch landed, please?

Comment 17

[Jeff Walden [:Waldo]]

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

Comment 18

[Nicholas Nethercote [inactive]]

Waldo: you should have committed this to nanojit-central.  Next time someone does a NJ-to-TM merge it'll be overwritten.  See https://developer.mozilla.org/en/NanojitMerge.  Can you backout the change and recommit it to nanojit-central?

Graydon, is there some way to automatically check against people making this mistake?  Vlad and Waldo have both done it, it's easy to do, esp. for people involved with TM but who don't usually do make NJ changes.

Comment 19

[Jeff Walden [:Waldo]]

Oh blah, I didn't consciously notice the change was to nanojit.  Sec, I'll back out and push to nanojit-central...

Comment 20

[Jeff Walden [:Waldo]]

Repush:

http://hg.mozilla.org/projects/nanojit-central/rev/998ded176a21

Backout:

http://hg.mozilla.org/tracemonkey/rev/2b79013c369a

Comment 21

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/8d898c57337c

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8d898c57337c