569819 - nanojit compilation failure on mingw-w64

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Jacek Caban]

Attachment: fix v1.0

I get following errors:

js/src/nanojit/Assembler.cpp:1678:29: error: cast from 'nanojit::NIns*' to 'long unsigned int' loses precision
[a few more the same errors]
js/src/nanojit/Assembler.cpp:810:9: error: cast from 'nanojit::NIns*' to 'long unsigned int' loses precision
js/src/nanojit/NativeX64.cpp:73:81: error: too many initializers for 'const nanojit::Register [5]'

First two are easy to fix by casting to intptr_t instead of long (in win64 ABI long is 4-bytes). The 3rd one is caused by guard for win64 ABI based compiler (_MSC_VER) instead of OS (_WIN64). The attached patch fixes the problem.

Comment 1

[Edwin Smith]

Comment on attachment 448990 [details] [diff] [review]
fix v1.0

marking r- in order to request an enhanced patch, although the change i'm requesting is trivial:

The printf format for pointers should just be changed to %p, which is used extensively elsewhere in the code.  Then we don't need any cast.

Do all compilers for x64-windows use the register conventions?  If so, _WIN64 sounds right, we should add a comment to that effect.

Comment 2

[Edwin Smith]

more specifically, %010lx should be changed to %p (not %010p) for consistency with how other addresses are printed.

Comment 3

[Jacek Caban]

Attachment: fix v1.1

Thanks for review, I've attached a fixed version. I had an impression that %p wasn't used intentionally. As for _WIN64, as far as I understand it's defined in ABI, so all compilers have to be compatible with this calling convention.

Comment 4

[Edwin Smith]

Comment on attachment 449046 [details] [diff] [review]
fix v1.1

Looks good and passed regression tests on tamarin-redux.

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/70f2ce619876

Comment 6

[Jacek Caban]

Landed again on TM after accidental revert in commit from bug 571202:

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

Comment 7

[Makoto Kato [:m_kato]]

Jecek, is this backed out by b987f7aaa128?

Comment 8

[Jacek Caban]

Yes, it was. Bug 573341 was backed out as well.

Comment 9

[Jacek Caban]

Landed again on TM:
http://hg.mozilla.org/tracemonkey/rev/6bba024e5fc8

Comment 10

[Jacek Caban]

nanojit-central: http://hg.mozilla.org/projects/nanojit-central/rev/85b2cf98c09b

Comment 11

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/98045ed9a537

Comment 12

[Jacek Caban]

Nicholas, please be more careful. It was reverted again by 0069451274aa.

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

Comment 13

[Robert Sayre]

(In reply to comment #12)
> Nicholas, please be more careful. It was reverted again by 0069451274aa.
> 
> http://hg.mozilla.org/tracemonkey/rev/d65fca375e44

nanojit fixes need to be fixed in http://hg.mozilla.org/projects/nanojit-central/

Comment 14

[Jacek Caban]

They are: http://hg.mozilla.org/projects/nanojit-central/rev/85b2cf98c09b

Comment 15

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/7d974430532a

Jacek, have you read https://developer.mozilla.org/en/NanojitMerge?  Changes to nanojit have to be pushed to nanojit-central and then merged to tracemonkey.  If you push them directly to tracemonkey they'll be overwritten by the next NJ-to-TM merge.  

As for Dão's landing of this patch on mozilla-central before it landed anywhere else... who knows how that's supposed to work.  Hopefully the next TM-to-mozilla-central merge will fix it without any additional work.

Comment 16

[Jacek Caban]

Nicolas, thanks for the link and explanation, that's what I needed. I was confused first by Dao's commit to m-c and later by the fact that the last revert of my patch was after I landed it on nanojit-central. BTW, I asked about landing on #jsapi and I was told I do things right...