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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Jacek Caban, Assigned: Jacek Caban)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

9 years ago
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
(Assignee)

Comment 1

9 years ago
Created attachment 389320 [details] [diff] [review]
Use GCC asm syntax when building on MinGW.
Attachment #389320 - Flags: review?(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
Attachment #389320 - Flags: review?(brendan) → review?(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?)
Attachment #389320 - Flags: review?(graydon) → review-
(Assignee)

Comment 4

8 years ago
(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.
(Assignee)

Comment 5

8 years ago
Created attachment 390444 [details] [diff] [review]
Fix 2

This is an alternative solution with exactly the same effect. This patch reorders #ifs instead of adding new ones.
Attachment #390444 - Flags: review?(graydon)

Updated

8 years ago
Attachment #390444 - Flags: review?(graydon) → review+
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?
(Assignee)

Comment 7

8 years ago
Yes, please commit it. Thanks.
Keywords: checkin-needed

Comment 8

8 years ago
I notice that the patches are based on TM codebase, should this be moved out of
the "tamarin" product?
Flags: flashplayer-triage+

Comment 9

8 years ago
Created attachment 392969 [details] [diff] [review]
Alternate patch

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

Updated

8 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 10

8 years ago
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?
(Assignee)

Comment 11

8 years ago
Created attachment 405463 [details] [diff] [review]
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.
Attachment #389320 - Attachment is obsolete: true
Attachment #390444 - Attachment is obsolete: true
Attachment #405463 - Flags: review?(graydon)
(Assignee)

Comment 12

8 years ago
Created attachment 405464 [details] [diff] [review]
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.
Attachment #405464 - Flags: review?(graydon)
With the only reviewed patch marked as obsolete, it's not at all clear what's "checkin-needed" here.
Assignee: nobody → jacek
Component: JIT Compiler (NanoJIT) → JavaScript Engine
Flags: flashplayer-triage+
Keywords: checkin-needed
Product: Tamarin → Core
QA Contact: nanojit → general
(Assignee)

Comment 14

8 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #405464 - Attachment is obsolete: true
Attachment #405464 - Flags: review?(graydon)
(Assignee)

Comment 15

8 years ago
Created attachment 418573 [details] [diff] [review]
rebased fix

This is the patch rebased against current version. I assume that a new review is not needed.
Attachment #392969 - Attachment is obsolete: true
Attachment #405463 - Attachment is obsolete: true
Attachment #405463 - Flags: review?(graydon)
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Blocks: 421095
(Assignee)

Updated

8 years ago
Attachment #390444 - Attachment is obsolete: false
(Assignee)

Comment 16

8 years ago
Can this be patch landed, please?
Whiteboard: [c-n: tracemonkey]
http://hg.mozilla.org/tracemonkey/rev/af515d48bdcf
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [c-n: tracemonkey] → fixed-in-tracemonkey
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.
Oh blah, I didn't consciously notice the change was to nanojit.  Sec, I'll back out and push to nanojit-central...
Repush:

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

Backout:

http://hg.mozilla.org/tracemonkey/rev/2b79013c369a
Whiteboard: fixed-in-tracemonkey → fixed-in-nanojit
http://hg.mozilla.org/tracemonkey/rev/8d898c57337c
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey

Updated

8 years ago
Duplicate of this bug: 523177

Comment 23

8 years ago
http://hg.mozilla.org/mozilla-central/rev/8d898c57337c
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.