Closed Bug 371247 Opened 14 years ago Closed 13 years ago

nspr changes for OpenBSD

Categories

(NSPR :: NSPR, defect)

x86
OpenBSD
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: martynas, Assigned: wtc)

Details

Attachments

(1 file, 7 obsolete files)

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.
Enable arm configuration for OpenBSD.
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.
-lpthread for regression tests
Attached 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.
Attached 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.
Attachment #256020 - Flags: review?(wtchang)
Attachment #256021 - Flags: review?(wtchang)
Attachment #256022 - Flags: review?(wtchang)
Attachment #256274 - Flags: review?(wtchang)
Attachment #256275 - Flags: review?(wtchang)
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)
(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.
Attached 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+
(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.
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?
(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
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.
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)
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: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
You need to log in before you can comment on or make changes to this bug.