605421 - libffi: VFP hard-float calling convention support

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Sudarsana Nagineni]

Attachment: Patch against m-c

There is a patch for VFP hard-float calling convention support available at 
http://sourceware.org/ml/libffi-discuss/2010/msg00153.html

I think it would be good to push this patch to m-c.

Looks like the patch is still not present in git repo for libffi, but it has already been reviewed.
http://sourceware.org/ml/libffi-discuss/2010/msg00167.html

Comment 1

[[e-mail]]

Do we need this for a Mozilla product, i.e. Fennec? If so, I'd take it once it's committed upstream -- easy enough to do by pinging Anthony.

Comment 2

[timeless]

Yes.

Comment 3

[Doug Turner (:dougt)]

yes, it will be needed for hardfp.

Comment 4

[[e-mail]]

Pinged libffi-discuss about the commit.

Comment 5

[[e-mail]]

You guys want this for b3?

Comment 6

[Doug Turner (:dougt)]

sounds reasonable.  Sounds like the meego toolchain requires it.

Comment 7

[Doug Turner (:dougt)]

dan, i am going to assign to you since you are doing the leg work.

Comment 8

[Doug Turner (:dougt)]

dwitte, is this patch ready to when the tree opens?

Comment 9

[[e-mail]]

Well, I wanted to pull from upstream, but apparently it hasn't been committed there yet. I did ping the list. So I can take this patch.

Comment 10

[[e-mail]]

Comment on attachment 484259 [details] [diff] [review]
Patch against m-c

r=dwitte, I'll land this soonish.

Comment 11

[Doug Turner (:dougt)]

dwitte, checkin-needed?

Comment 12

[[e-mail]]

Anthony rebased the libffi git tree over the weekend. There's a merge mistake that needs to be corrected but hopefully that'll get done today, and I'll push. If not, I'll fix it myself and push anyway. :)

Comment 13

[[e-mail]]

... except the ARM patch doesn't actually build. I've pushed a fix to try and will push to Anthony today if it's good.

Comment 14

[[e-mail]]

I don't think this has been answered yet, so: would we build with this patch on any platforms as it stands? If no, when will we? If yes, what are those platforms, what are we doing on them now, and how do we test that they're working as expected with this patch?

(The answer to the last one is "run ctypes tests", but how do we get to that point?)

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

dwitte, who were you expecting to answer your questions?

Comment 16

[[e-mail]]

Well, dougt said we need it, so he may be able to elaborate.

Comment 17

[Doug Turner (:dougt)]

timeless can explain.

Comment 18

[timeless]

http://trac.tspre.org/meetbot/meego-meeting/2010/meego-meeting.2010-12-01-20.02.html

MeeGo 1.2 as a "platform" allows for hard fp.

"Allows" is a bit loose, it's quite likely that no ARM platforms built around MeeGo 1.2 will do !hard fp. The internal numbers seem to show enough of a win such that vendors pushed hard and were willing to disrupt schedules for it.

Certain vendors were pushing hard and intend to ship with hard fp as their platform. If anyone wants their browser (or whatever) to be able to be delivered in any way to such MeeGo platforms, they need to support hard fp.

Comment 19

[[e-mail]]

OK. So does that mean we're going to run unit tests on it? Or that someone will?

I ask because we can take this patch, and theoretically get a working libffi, which will theoretically get us a working ctypes. But history shows that this will not be the case, at all, unless it's unit tested and proven.

Comment 20

[Doug Turner (:dougt)]

timeless, can you verify that this works for hard fp toolchains + devices that you might have access to?

Comment 21

[timeless]

Sudarsana can do that - we're neighbors.

As for unit tests, I'm confident someone will.

I'm fairly certain we're currently using this patch internally on Fennec builds.

If there's a specific command you'd like run in order to get a report that can be attached or sent to you, please provide it and someone will run it...

Comment 22

[[e-mail]]

'make xpcshell-tests' in toolkit/components/ctypes would satisfy me. Basically, as long as you run unit tests on it, and they pass, then I'm OK with taking this...

Do they? ;)

Comment 23

[timeless]

i spoke w/ babu and asked him to provide output for three runs:
1. unpatched
2. with [meego] system libffi
3. patched

Comment 24

[Sudarsana Nagineni]

Attachment: log with patch

> 1. unpatched
> 2. with [meego] system libffi
> 3. patched

I was able to run the tests on device finally. Unit test[test_jsctypes.js] failed in all above cases and the results are same. Please check the results attached.

Comment 25

[Sudarsana Nagineni]

Attachment: log without patch

Comment 26

[Sudarsana Nagineni]

Attachment: log with system libffi

Comment 27

[[e-mail]]

OK. That means the float-conversion overflow behavior of int64's is different than expected. That's not too hard to fix, but belongs in a separate bug...

Comment 28

[timeless]

oh, minor note, the platform team for meego has decided to break floating point (roughly -ffast-math or something), it's possible that we'd run into this somewhere nearby, that change happened "recently".

dwitte: so you'll push for whatever flags are needed and get this pushed?

Comment 29

[Oleg Romashin (:romaxa)]

Comment on attachment 484259 [details] [diff] [review]
Patch against m-c

Can we get this patch landed? I guess it is just sync with upstream, also this seems works fine (test works)

Comment 30

[Oleg Romashin (:romaxa)]

Comment on attachment 484259 [details] [diff] [review]
Patch against m-c

ok, patch need to be tested on try first

Comment 31

[Oleg Romashin (:romaxa)]

Try build looks pretty green
http://hg.mozilla.org/try/rev/ae39c832348e

Comment 32

[Oleg Romashin (:romaxa)]

Comment on attachment 484259 [details] [diff] [review]
Patch against m-c

try build pretty green, can we get an approval here?

Comment 33

[[e-mail]]

Comment 24 indicates that tests are broken on the platform Nokia is testing. Which, to my understanding, is not covered by anything on tryserver -- which is presumably why we're seeing it pass there. In fact, we don't run any unit tests on Meego/Maemo platforms on try, no? Just Talos. Which means, I bet, that ctypes is broken on Meego/Maemo without VFP hard-float anyway.

I think we should land this, since it's not making anything worse; but we should get followup bugs to actually make ctypes tests pass. :)

Comment 34

[Oleg Romashin (:romaxa)]

(In reply to comment #33)
> Comment 24 indicates that tests are broken on the platform Nokia is testing.

ok, now I see, I'll ask Sudarsana Nagineni

Comment 35

[Oleg Romashin (:romaxa)]

(In reply to comment #27)
> OK. That means the float-conversion overflow behavior of int64's is different
> than expected. That's not too hard to fix, but belongs in a separate bug...

yep, this should be in separate bug... and we cannot probably fix that bug without ability to run tests at all...

Comment 36

[Doug Turner (:dougt)]

Comment on attachment 484259 [details] [diff] [review]
Patch against m-c

this must not break anything; please run through try before landing.

Comment 37

[Oleg Romashin (:romaxa)]

> this must not break anything; please run through try before landing.
already did..  see comment#c32 ;)

Comment 38

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/22dda43b07c2