Closed Bug 369722 Opened 13 years ago Closed 12 years ago

prdtoa.c jsnum.h txDouble.h not required IEEE_ARM define on Codesourcery EABI gcc3.4.4 compiler

Categories

(Core :: XSLT, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: romaxa, Assigned: romaxa)

Details

Attachments

(2 files)

Codesourcery 3.4.4 compiler use in Maemo platform.

Gecko have special defines in nspr, js, xslt for ARM compiler.
.........
#if defined(__arm) || defined(__arm32__) || defined(__arm26__) || defined(__arm__)
#define CPU_IS_ARM
........
See Bug 106864, Bug 209814, Bug 278981.

But for Codesourcery gcc3.4.4 ARM  and gcc4.x.x that defines should be disabled, because they have EABI...

See patch in attachments

Also there are fix for wrong define name 
CPU_IS_ARM -> IEEE_ARM
Attachment #254407 - Flags: review?(darin.moz)
Summary: prdtoa.c jsnum.h txDouble.h not required CPU_IS_ARM define on Codesourcery EABI gcc3.4.4 compiler → prdtoa.c jsnum.h txDouble.h not required IEEE_ARM define on Codesourcery EABI gcc3.4.4 compiler
Attachment #254407 - Flags: review?(darin.moz) → review+
Attachment #254407 - Flags: review?(shaver)
Attachment #254407 - Flags: review?(peterv)
Attachment #254407 - Flags: review?(shaver) → review+
Attachment #254407 - Flags: review?(jonas)
Any progress?
Have you tried the testcase in bug 53518 to see if it still passes on all affected platforms with this fix?
This my patch will affected only for ARM gnueabi platform.
I have checked last testcase from bug 53518 
https://bugzilla.mozilla.org/attachment.cgi?id=137614
and it pass all cases.

Attachment #254407 - Flags: review?(peterv)
Attachment #254407 - Flags: review?(jonas)
Attachment #254407 - Flags: review+
Comment on attachment 254407 [details] [diff] [review]
Probably this is the fix.

I checked in the NSPR (prdtoa.c) change in this patch on the
NSPR trunk (NSPR 4.7) and the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Mozilla trunk, Gecko 1.9 pre-release).
gavin@picsel.com, am I right in guessing that bug 383607 is a more correct fix for this general problem?
Yes, for JS Part...

There are still XSLT part which not fixed yet.
txDouble.h
sure, but I'd note that if their fix is more correct, then a patch that uses it would be better than me landing a less correct patch.
(In reply to comment #5)
> gavin@picsel.com, am I right in guessing that bug 383607 is a more correct fix
> for this general problem?
> 

I think so. The approach in bug 383607 allows the floating point model to be detected independently of the other aspects of the ABI (some older compilers certainly allow the FP model to be set to either FPA or VFP).

The only slight snag in bug 383607 is that different toolchains tend to use different macro names to indicate the floating point model, so it may be necessary to add additional checks for a particular toolchain (__VFP_FP__ works for gcc-arm and maybe others).

If __ARM_EABI__ always implies VFP (i.e. using FPA is impossible) then it would be OK to add an __ARM_EABI__ check to the existing #ifdef __VFP_FP__ block in jsnum.h. [I haven't had a chance to check this yet.]

That approach should be fine to use throughout the other header files too.

Since ARM supports two floating point models (VFP and FPA), the macro name IEEE_ARM is slightly inaccurate so I wouldn't complain if it was changed.
(In reply to comment #8)
>
> Since ARM supports two floating point models (VFP and FPA), the macro name
> IEEE_ARM is slightly inaccurate so I wouldn't complain if it was changed.

Gavin, could you suggest a better name for the IEEE_ARM macro?
Are we using the VFP or FPA model when IEEE_ARM is defined?
(In reply to comment #9)
> (In reply to comment #8)
> >
> > Since ARM supports two floating point models (VFP and FPA), the macro name
> > IEEE_ARM is slightly inaccurate so I wouldn't complain if it was changed.
> 
> Gavin, could you suggest a better name for the IEEE_ARM macro?
> Are we using the VFP or FPA model when IEEE_ARM is defined?
> 

For the JS engine the name 'FPU_IS_ARM_FPA' is used. IEEE_ARM corresponds to the FPA model.
JS and NSS already have this fix, there only xslt...
Attachment #286146 - Flags: review?(jonas)
Attachment #286146 - Flags: superreview+
Attachment #286146 - Flags: review?(jonas)
Attachment #286146 - Flags: review+
Assignee: wtc → nobody
Component: NSPR → XSLT
Product: NSPR → Core
QA Contact: nspr → xslt
Version: other → Trunk
Assignee: nobody → romaxa
Attachment #286146 - Flags: approval1.9?
Attachment #286146 - Flags: approval1.9? → approval1.9+
Checking in content/xslt/public/txDouble.h;
/cvsroot/mozilla/content/xslt/public/txDouble.h,v  <--  txDouble.h
new revision: 1.3; previous revision: 1.2
done
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
You need to log in before you can comment on or make changes to this bug.