333347 - legacy BSD NAN constants may cause alignement problems with 64 bit

Status: NEW

(Core :: XSLT, defect)

Description

[Martin Husemann]

extensions/transformiix creates NAN an +/- infinity double constants via some dubious bit magic. There is no guarantee that the resulting values are properly aligned for 64 bit archs with strict alignment. The problem might not always strike, depending on random luck during linking.

There probably is a less intrusive way to fix it (like depending on alignement pragmas), but to me this looked like the cleanest way.

Comment 1

[Martin Husemann]

Attachment: primitives.h change

Comment 2

[Martin Husemann]

Attachment: changes to FunctionCall.cpp

Comment 3

[Martin Husemann]

Attachment: changes to MultiplicativeExpr.cpp

Comment 4

[Martin Husemann]

Attachment: changes to txFormatNumberFunctionCall.cpp

Comment 5

[Martin Husemann]

Attachment: changes to txStylesheetCompileHandlers.cpp

Comment 6

[Martin Husemann]

Attachment: changes to txXSLTPatterns.cpp

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Could you attach a |cvs diff -up8| of all changes in one file?

Comment 8

[Martin Husemann]

(In reply to comment #7)
> Could you attach a |cvs diff -up8| of all changes in one file?
> 

Sure (I wasn't even finished with the single files). Give me a few minutes...

Comment 9

[Martin Husemann]

Attachment: all-in-one patch

Sorry, this is not compile tested even, but I guess the intent will be pretty clear and any oversights easily fixable. I created this on a NFIREFOX_1_5_0_1_RELEASE branch (because I happened to have that around). If anything is unclear or you need a (tested) patch against trunk, let me know.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Would the patch in bug 289394 fix this? The patch probably doesn't apply cleanly any more, but that should be pretty easy to fix.

Comment 11

[Martin Husemann]

(In reply to comment #10)
> Would the patch in bug 289394 fix this? The patch probably doesn't apply
> cleanly any more, but that should be pretty easy to fix.

Yes, that is one of the other ways to solve it - but I still think using ISO C features (where available) is better.

Comment 12

[Martin Husemann]

Let me make the consequences of this bug a bit clearer: while on some 64bit archs the unaligned accesses are just slow, on others they need fixup by the kernel (alpha) [i.e. are *much* slower]; they cause (by default) a bus error/core dump on some (like sparc64).

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, I understand the problem, I'm just trying to figure out what the best solution is.

The only thing that I don't like about the patch in this bug is that it creates two levels of indirection for some functions that really should be very simple. And these functions are in some cases somewhat important to performance (we need to call isNaN for many common math operations for example).

Could you instead of creating new functions have a txDouble::Init() function that is called on startup that sets the members to their proper values, either using the current method or using macros. You can look at nsTextFragment::Init for an example.

Oh, and please write a patch that applies to the trunk since it's changed quite a bit since the 1.5 branch (unfortunatly, if you want to get this working in firefox 2 you'll have to write a patch both for the trunk and for the 1.5 branch).

Also, your implementation of isNeg does not work since it won't catch negative 0. You can test using the testcase in bug 53518.

Comment 14

[Axel Hecht]

I think it should suffice to port stricter to jsnum.h again. That's our reference.

Comment 15

[Martin Husemann]

(In reply to comment #13)
> Could you instead of creating new functions have a txDouble::Init() function
> that is called on startup that sets the members to their proper values, either
> using the current method or using macros. You can look at nsTextFragment::Init
> for an example.

How about an making txDouble::isNaN() an inline method that just calls the standard C isnan()? This might need an autoconfig feature test for it's presence (though I'm not aware of supported plattforms that do not have it).

But if you prefer to got the Init route, I can do that too.

Comment 16

[Axel Hecht]

The summary was an insult, and non-precise.
Martin, as you probably don't know the people that wrote that code, I suggest that
you stick with the choice of summary from the XSLT team, in which I am a peer.

Comment 17

[Martin Husemann]

(In reply to comment #16)
> The summary was an insult, and non-precise.

Oh, sorry - bugzilla tricked me, I didn't mean to change it back (nor to be insulting in the first place)

Comment 18

[Martin Husemann]

(In reply to comment #14)
> I think it should suffice to port stricter to jsnum.h again. That's our
> reference.

This sounds like a good idea to me.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Inlining all the way is good when we can, however I'm not sure we can when the standard C functions aren't available. Then we want to stick them implementation in a separate .cpp file so that all the FreeBSD flags only affect that file.

Comment 20

[Martin Husemann]

It seems that we can't go forward in this direction - would an alternative patch that wraps the magically created aggregates into a union to force proper alignement be acceptable? Haven't tried it yet, but it should be doable, minor intrusive (optionally restricted via ifdefs) and portable.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds good to me

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

My main concern really is performance when it comes to this. This code is very unreadable no matter what :(

Comment 23

[Havard Eidnes]

Attachment: Alternative patch using GCC attribute extension to force alignment

This is an alternative patch for this problem, taken from
sunbird 0.9.

Naturally, it fixes the problem only for those who use gcc
as their compiler.

I'd like comments if something along these lines would be
more acceptable?