User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:22.214.171.124) Gecko/20090715 Firefox/3.5.1 Build Identifier: Preprocessor instructions assume that all WIN32 and WIN64 code is compiled using MSVC. Reproducible: Always
Created attachment 389320 [details] [diff] [review] Use GCC asm syntax when building on MinGW.
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 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?)
(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.
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.
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?
Yes, please commit it. Thanks.
I notice that the patches are based on TM codebase, should this be moved out of the "tamarin" product?
Created attachment 392969 [details] [diff] [review] Alternate patch Here's an alternate patch that doesn't move around the code.
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?
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.
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.
With the only reviewed patch marked as obsolete, it's not at all clear what's "checkin-needed" here.
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.
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.
Can this be patch landed, please?
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...