596737 - [OS/2] [JAEGER] linking problems of MethodJIT

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; Warp 4.5; rv:2.0b6pre) Gecko/20100914 Firefox/4.0b6pre
Build Identifier: 

we need to add some if defined(XP_OS2) to be able to link against the MethodJIT object. Practically we need the prepending underscore and like MinGW the symbols created by the asm have to be properly decorated to be recognized as fastcall symbols (see bug595199).

Reproducible: Always

Comment 1

[Walter Meinl]

Attachment: do it like windows

Comment 2

[Walter Meinl]

Comment on attachment 475632 [details] [diff] [review]
do it like windows

going for additional r from someone of the js crew.

Comment 3

[Walter Meinl]

Attachment: patch for checkin

added a comment for check-in to the patch

Comment 4

[Jonathan Watt [:jwatt]]

Why does this have checkin-needed? Please remove that keyword until you have approval.

Comment 5

[Walter Meinl]

Comment on attachment 477714 [details] [diff] [review]
patch for checkin

beautified version of the patch r+ed by OS/2 and JS guys. Very low risk adding ifdef XP_OS2 to unbreak the build

Comment 6

[Jonathan Watt [:jwatt]]

Thank you.

Comment 7

[Walter Meinl]

(In reply to comment #6)
> Thank you.

Yeah, sorry, I wasn't aware that js checkins have to be approved, too.

Comment 8

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/43dd46547be6

Comment 9

[Walter Meinl]

(In reply to comment #8)
> Pushed http://hg.mozilla.org/mozilla-central/rev/43dd46547be6

Thanks for the push. Sorry if the question is stupid this checkin won't get lost next time when tracemonkey is merged?

Comment 10

[Jonathan Watt [:jwatt]]

I'd be shocked if it was! :)

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

We try to push js/src changes to tracemonkey because, yes, it can create merge headaches.

Comment 12

[Jonathan Watt [:jwatt]]

To be clear, I should have pushed to tracemonkey, not m-c?

Comment 13

[Nicholas Nethercote [inactive]]

(In reply to comment #11)
> We try to push js/src changes to tracemonkey because, yes, it can create merge
> headaches.

To expand on this:  we *always* push js/src changes to the tracemonkey repository, and then mark the bug whiteboard field as "fixed-in-tracemonkey".  Then rsayre periodically does merges from tracemonkey to mozilla-central and vice versa.  So your push won't get lost but in the future please push to the tracemonkey repo.

Comment 14

[Jonathan Watt [:jwatt]]

Ok, noted, apologies for any merge problems I've caused. :(

For future, it's worth noting when checkin-needed is used the assumption is that the landing is for m-c unless the Whiteboard/Version/Target Milestone fields say otherwise. Although I now know better for js/src, it may be worth your while to make sure the whiteboard is set in future in case there are others looking for patches to push that don't yet know of this exception.

Comment 15

[Walter Meinl]

(In reply to comment #14)
Sorry, I had written to the whiteboard TraceMonkey when I set the checkin-needed flag before asking for approval.
When asking for approval I cleared the whiteboard and after getting approval I have forgotten to mention tracemonkey again in the whiteboard.