Status

defect
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: martynas, Assigned: wtc)

Tracking

other
x86
OpenBSD

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Reporter

Description

13 years ago
This bug report is for nspr changes to OpenBSD. Attaching some patches now; more to come (e.g. use native __dtoa() and __freedtoa which we have have in libc, instead the implemented in nspr ones). Regression tests pass on OpenBSD/arm.
Reporter

Comment 1

13 years ago
Enable arm configuration for OpenBSD.
Reporter

Comment 2

13 years ago
Enable arm configuration for OpenBSD.

#define PR_DLL_SUFFIX ".so":
Don't append nspr's shared lib version in PR_GetLibraryName. This fixes
missing 'Builtin Roots Module'/Authorities problem in mozilla* apps.
Reporter

Comment 3

13 years ago
-lpthread for regression tests
Reporter

Comment 4

13 years ago
Posted patch prdtoa-native.diff (obsolete) — Splinter Review
Use native __prdtoa and __freedtoa we have implemented in libc.
Eliminates 3400 lines of the code.
I think more platforms might have this; so feel free to change __OpenBSD__ and propagate it in cfg file.
Reporter

Comment 5

13 years ago
Posted patch configure.in.diff (obsolete) — Splinter Review
define _PR_HAVE_GETPROTO_R and _PR_HAVE_GETPROTO_R_INT. Also, link libc after __dtoa and __freedtoa change.
Reporter

Updated

12 years ago
Attachment #256020 - Flags: review?(wtchang)
Reporter

Updated

12 years ago
Attachment #256021 - Flags: review?(wtchang)
Reporter

Updated

12 years ago
Attachment #256022 - Flags: review?(wtchang)
Reporter

Updated

12 years ago
Attachment #256274 - Flags: review?(wtchang)
Reporter

Updated

12 years ago
Attachment #256275 - Flags: review?(wtchang)
Reporter

Comment 6

12 years ago
ANY PROGRESS?
Status: NEW → ASSIGNED
martynas,  
When submitting a patch, it should be a single file containing a cvs diff 
of all the files that need to be changed.  Then you should ask for review
of that one file, rather than asking for review individually on each file
to be patched.  

I have taken the liberty of making such a single patch file on your behalf.
This is not something I will do repeatedly.  It is just here to show the 
expected format.  I produced this by applying your individual patch files
and then running the command "cvs diff -pu5" in the directory mozilla/nsprpub.
Maybe this will hasten the review you seek.

Here are some other comments on the patch.
1. in prdtoa.c, tather than these:

+#ifdef __OpenBSD__
+    result = __dtoa(d, mode, ndigits, decpt, sign, rve);
+#else
     result = dtoa(d, mode, ndigits, decpt, sign, rve);
+#endif

+#ifdef __OpenBSD__
+    __freedtoa(result);
+#else
     freedtoa(result);
+#endif

It might have been better to code:

+#ifdef __OpenBSD__
+#define dtoa __dtoa
+#define freedtoa __freedtoa
+#endif

And rather than 

+char *__dtoa(double d, int mode, int ndigits, int *decpt, int *sign, char **rve
);
+void __freedtoa(char *s);

It might be better to #include a system header that defines those, inside of 
#ifdef __OpenBSD__

Rather than 

+#ifdef __OpenBSD__
+PR_cnvtf(char *buf,int bufsz, int prcsn,double _fval)
+#else
 PR_cnvtf(char *buf,int bufsz, int prcsn,double fval)
+#endif

+#ifdef __OpenBSD__
+    _double fval;
+    value(fval) = _fval;
+#endif

Could you simply have coded:

+#ifdef __OpenBSD__
+    value(fval) = fval;
+#endif

And is value(fval) a legal c lvalue?  
Would this work as well?
     fval = value(fval);
Attachment #256020 - Attachment is obsolete: true
Attachment #256021 - Attachment is obsolete: true
Attachment #256022 - Attachment is obsolete: true
Attachment #256274 - Attachment is obsolete: true
Attachment #256275 - Attachment is obsolete: true
Attachment #260964 - Flags: review?(wtc)
Attachment #256020 - Flags: review?(wtc)
Attachment #256021 - Flags: review?(wtc)
Attachment #256022 - Flags: review?(wtc)
Attachment #256274 - Flags: review?(wtc)
Attachment #256275 - Flags: review?(wtc)
Reporter

Comment 8

12 years ago
(In reply to comment #7)
> 1. in prdtoa.c, tather than these:
> 
> +#ifdef __OpenBSD__
> +    result = __dtoa(d, mode, ndigits, decpt, sign, rve);
> +#else
>      result = dtoa(d, mode, ndigits, decpt, sign, rve);
> +#endif
> 
> +#ifdef __OpenBSD__
> +    __freedtoa(result);
> +#else
>      freedtoa(result);
> +#endif
> 
> It might have been better to code:
> 
> +#ifdef __OpenBSD__
> +#define dtoa __dtoa
> +#define freedtoa __freedtoa
> +#endif

Agreed.

> And rather than 
> 
> +char *__dtoa(double d, int mode, int ndigits, int *decpt, int *sign, char
> **rve
> );
> +void __freedtoa(char *s);
> 
> It might be better to #include a system header that defines those, inside of 
> #ifdef __OpenBSD__

There isn't any.

> Rather than 
> 
> +#ifdef __OpenBSD__
> +PR_cnvtf(char *buf,int bufsz, int prcsn,double _fval)
> +#else
>  PR_cnvtf(char *buf,int bufsz, int prcsn,double fval)
> +#endif
> 
> +#ifdef __OpenBSD__
> +    _double fval;
> +    value(fval) = _fval;
> +#endif
> 
> Could you simply have coded:
> 
> +#ifdef __OpenBSD__
> +    value(fval) = fval;
> +#endif

No because they are completely different structures.
As you see from the code:

typedef union {
        double d;
        ULong ul[2];
} _double;

#define value(x) ((x).d)

(_fval is double)

value(fval) = _fval; makes fval available for further using with our functions.

> And is value(fval) a legal c lvalue?  
> Would this work as well?
>      fval = value(fval);
> 

No.

Thx for your feedback, will submit the new diff soon.
Reporter

Comment 9

12 years ago
Posted patch nspr-openbsd.diff, v2 (obsolete) — Splinter Review
Also included patch-mozilla_nsprpub_pr_include_private_primpl_h.
Attachment #260964 - Attachment is obsolete: true
Attachment #261067 - Flags: superreview?(wtc)
Attachment #261067 - Flags: review?(nelson)
Attachment #260964 - Flags: review?(wtc)
Comment on attachment 261067 [details] [diff] [review]
nspr-openbsd.diff, v2

Martyas,  For what branch is your patch prepared? 
I *think* (not certain, wtc can clarify in his SR comments) that 
we are taking patches for the trunk and the NSPR_4_6_BRANCH right now.  
That is why I prepared my patch for the trunk.  
You patch appears to be some something else.  Please clarify.

Since your patch appears to apply cleanly to the trunk, r=nelson 
for the trunk.  Pls wait for wtc's SR.
Attachment #261067 - Flags: review?(nelson) → review+
Reporter

Comment 11

12 years ago
(In reply to comment #10)
> (From update of attachment 261067 [details] [diff] [review])
> Martyas,  For what branch is your patch prepared? 
> I *think* (not certain, wtc can clarify in his SR comments) that 
> we are taking patches for the trunk and the NSPR_4_6_BRANCH right now.  
> That is why I prepared my patch for the trunk.  
> You patch appears to be some something else.  Please clarify.

The diff was made against MOZILLA_1_8_BRANCH mozilla/nsprpub.

I would like the diff committed for trunk and NSPR_4_6_BRANCH, because i don't think it even works on OpenBSD as it is now.
Assignee

Comment 12

12 years ago
Comment on attachment 261067 [details] [diff] [review]
nspr-openbsd.diff, v2

Does OpenBSD have pthreads?  If so, you should build NSPR
with pthreads (configure NSPR with the --with-pthread option
if necessary).  Your change to primpl.h is not necessary
if NSPR is built with pthreads.

Are __dtoa and __freedtoa public functions?  Why do their names
have the double underscore __ prefix?  Are they declared in a
system header file?
Reporter

Comment 13

12 years ago
(In reply to comment #12)
> (From update of attachment 261067 [details] [diff] [review])
> Does OpenBSD have pthreads?  If so, you should build NSPR
> with pthreads (configure NSPR with the --with-pthread option
> if necessary).  Your change to primpl.h is not necessary
> if NSPR is built with pthreads.

Yea, seems you are right. The primpl.h diff is useless (it was for an older issue).

> Are __dtoa and __freedtoa public functions?  Why do their names
> have the double underscore __ prefix?  Are they declared in a
> system header file?

They are in our libc, i think they are for the internal usage (and no header file declaring it), but were always there (since 1.1.1.1, 1995), the reason for using it was the pain we had, we always had lots of issues with your dtoa, esp. on other platforms like arm, i think it's one of the most unportable code and using our __dtoa is a perfect solution which eliminates 3400 lines of your code.

The __dtoa(), __freedtoa() are declared here:
http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdlib/strtod.c?rev=1.30
Reporter

Comment 14

12 years ago
wtc, if something is preventing to incorporate the dtoa diff, or you are against it, just let me know. It's better to incorporate the rest of the changes than nothing.
Assignee

Comment 15

12 years ago
Sorry about the delay.  I forgot this bug.

I checked in most of your changes.  I did not check in your
changes to primpl.h and prdtoa.c.  I moved the definitions
of the _PR_HAVE_GETPROTO_R and _PR_HAVE_GETPROTO_R_INT macros
from configure.in to prnetdb.c.  I removed NetBSD and FreeBSD
from your change to lib/tests/Makefile.in.  Did you test this
change on NetBSD and FreeBSD?  I understand that FreeBSD uses
-lc_r instead of -lpthread.
Attachment #261067 - Attachment is obsolete: true
Attachment #261067 - Flags: superreview?(wtc)
Assignee

Comment 16

12 years ago
The patch was checked in on the NSPR trunk (NSPR 4.7) and the
NSPRPUB_PRE_4_2_CLIENT_BRANCH for the Mozilla trunk (Gecko 1.9
pre-release).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
You need to log in before you can comment on or make changes to this bug.