124536 - Eliminate nsCRT::str* to use str* from libC

Status: NEW

(Core :: XPCOM, defect, P5)

Description

[Cathleen]

removes levels of indirections in code that we don't need, to improve performance.

similar to bug 118135

Comment 1

[Roland Mainz]

cathleen:
nsCRT::strtok() should be replaced with PL_strtok_r() (same function as
strtok_r()) ...

Comment 2

[Brendan Eich [:brendan]]

Why do we need to use PL_strtok_r?  Does any platform lack strtok_r?

/be

Comment 3

[Wan-Teh Chang]

Windows lacks strtok_r.

Comment 4

[Cathleen]

Attachment: eliminate nsCRT::strlen() for char * strings

eliminated 473 callers to nsCRT::strlen(char* s) and function itself.

Comment 5

[Suresh Duddi (gone)]

Comment on attachment 69775 [details] [diff] [review]
eliminate nsCRT::strlen() for char * strings

r=dp

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 69775 [details] [diff] [review]
eliminate nsCRT::strlen() for char * strings

Go strong!  Don't let any more nsCRT::strlen on char * data go in while you
blink!

/be

Comment 7

[Bradley Baetz (:bbaetz)]

This caused mac/beos bustage because of bug 102980 - if size_t is unsigned long,
rather than unsigned int, then the overloading between the two versions of
nsCRT::strncmp we have is ambiguous.

I imagine that that bug will be fixed for char* when you use the native strncmp,
though, although it won't fix the prunichar* case.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

nsCRT.h still includes a comment for the now-removed nsCRT::memcpy function,
maybe you can remove it while you're at the file.
( http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.h#111 )

Comment 9

[Cathleen]

Comment on attachment 69775 [details] [diff] [review]
eliminate nsCRT::strlen() for char * strings

I landed all changes in this patch (for last 2 days), except removing actual
nsCRT::strlen(char* s) function itself.

A new patch to flow up for diff left to land.

Comment 10

[Cathleen]

Attachment: follow up on removal of nsCRT::strlen(char* s)

remove nsCRT::strlen(char* s)and fixed up comments in nsCRT.h
added back changes that was erased by taka's check-ins in mailnews.

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 70658 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s) 

>+   ***  In addition, the follwoing char* string utilities
>+   ***  are no longer supported either.  Plse use lib C also.

Typo for 'following' and 'please'

Comment 12

[timeless]

just a reminder that you can't whack strcasecmp/strncasecmp or strdup.

   /** Compute the string length of s
    @param s the string in question
    @return the length of s
    */
-  static PRUint32 strlen(const char* s) {
-    return PRUint32(::strlen(s));
-  }
 
also, please don't remove a function without removing its comment. doing so 
will mess up things like 
http://unstable.elemental.com/mozilla/build/latest/mozilla/xpcom/dox/classnsCRT
.html (or humans trying to read the code...)

Comment 13

[Cathleen]

I will remove this comment, when the last str* function is removed.  Right now,
I plan to leave it in till then.  :-)


 /** Compute the string length of s
    @param s the string in question
    @return the length of s
    */
-  static PRUint32 strlen(const char* s) {
-    return PRUint32(::strlen(s));
-  }
 

Comment 14

[Cathleen]

Attachment: follow up on removal of nsCRT::strlen(char* s)

same as attachment 70658 [details] [diff] [review], fixed typos in comments :-)

remove nsCRT::strlen(char* s) and updated comments in nsCRT.h
added back changes that was erased by taka's check-ins in mailnews.

Comment 15

[Suresh Duddi (gone)]

Comment on attachment 70750 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s)

r=dp

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 70750 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s)

sr=[e-mail]

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 70750 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s)

a=roc+moz for 0.9.9

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 70750 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s)

removing approval. if this still needs to land please re-request approval from
drivers. thanks.

Comment 19

[Henry Jia]

Attachment: example to show the crash when using strlen with the parameter string null

After examing the usage of 'strlen' for some time, I think it is a better way
to encapsulate the function. Because if we pass a parameter of NULL to
'strlen', the application will crash. The attachment I give today(04/01/2002)
is an example to show the crash. I've verified this on both Win2k and Solaris.
And I find that in mozilla a lot of people doesn't check the parameter before
passing it in. So this is a very serious implied problem.
I'm not sure if I should file a new bug or keep on working on this bug, so I
just discuss this issue here first. If you think I should file a new bug, just
tell me.
BTW: I'll try to give a patch for the encapsulation with almost no performance
lost soon, so this issue may be discussed more clearly and quickly.

Comment 20

[Henry Jia]

It is a foundation stability problem to encapsulat the C function 'strlen'. If
we do this, we can make mozilla more stable.

Comment 21

[Jerry]

I agree with henry, just like NSPR
we use NSPR, will lose some perf, but at the same time, 
we can use it more safely.

Comment 22

[Henry Jia]

Attachment: patch to encapsulate the C function 'strlen'

This is the patch for encapsulating the C function 'strlen'. Using the
encapsulating function 'nsCRT::strlen' instead of directly using the C function
'strlen' will cause almost no performance lose in average. Although it will add
a comparing operation, the operation will use very little time and when the
parameter string is NULL, there is no need to call the C function and directly
return 0.
If this patch can be r= and sr=, we need to go back to reuse the original
nsCRT::strlen instead of direct strlen. So although I strongly suggest to
encapsulating 'strlen', we must think this issue clearly. If we need to reuse
nsCRT::strlen, I volunteer to do this work.
BTW: Other similliar functions, such as str*, printf, etc, also need to be
taken into account.

Comment 23

[Henry Jia]

By using NSPR, we can not only make mozilla cross-platform, but also give out a
set of APIs which are more stable and more easy to use.

Comment 24

[Brendan Eich [:brendan]]

NSPR does not wrap ("encapsulate") all Standard C and C++ functions, nor should
it.  Besides defeating optimizations where the compiler can disambiguate strlen
and inline it appropriately, such wrapping adds cost.  We do not know how high
that cost is without measuring real-world benchmarks with and without wrapping.

Then there is the question of null-testing char * or const char * in parameters.
 If we had been null-testing the parameter to nsCRT::strlen(const char* s) for
years, then I would agree that we can't stop checking for null.  However,
nsCRT::strlen has *never* null-tested s.  The standard C function strlen does
not null-test either.  So I claim we should not keep nsCRT::strlen, because
(1) we do not want any overhead of wrapping, however small it appears without
measurement;
(2) we do not want any overhead of null-testing, same reason;
(3) we do not need null-testing for nsCRT.h "API compatibility", and now is not
the time to be changing the API semantics.

Let's not actually encourage callers to be sloppy about null vs. a valid char
pointer.  A C string is referenced via a (const or not) char *, which is never
null.  The C standard doesn't allow null as a valid parameter to strlen, and
neither should Mozilla allow it in any strlen-like wrapper, or pass null into
anything named strlen.

/be

Comment 25

[Brendan Eich [:brendan]]

What's more, all this talk about "stability" and "portability" is conjecture. 
We have code that is highly portable already.  Until someone can demonstrate a
real world portability problem that requires wrapping strlen or other standard C
library functions, we should do nothing.  To do the opposite in the name of some
abstract good such as stability or portability is to bloat code and add cycle
costs without reason.

/be

Comment 26

[Roland Mainz]

Henry Jia:
If someone hits the "|strlen(NULL)|-causes-crash"-issue in a production build
he/she can use |% export LD_PRELOAD=/usr/lib/[e-mail].1; ./mozilla| in Solaris to
work around these crashes ([e-mail].1 maps a zero-filled page at the address 0 to
make it a valid address) ...

Comment 27

[Bradley Baetz (:bbaetz)]

If someone finds strlen(NULL) being called, then they can file a bug so that the
code cam be fixed.

Comment 28

[Henry Jia]

Anyway, I still stick to my opinion for the following reasons.
1. PL_strlen wrap strlen with the null parameter checking
(http://lxr.mozilla.org/seamonkey/source/nsprpub/lib/libc/src/strlen.c#40). And
it still be used in the trunk code. Since we're trying to eliminate it, we
should pay attention to the places in which it is used.
2. I still think by wraping strlen, we can make mozilla programming more easy
and safe.

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

I'm against using PR_strlen, it's much slower than simply using strlen(),
because a) it's not inlined and b) it does the null check

For several years we have not null checked strings passed into strlen, why
should we start now?

Comment 30

[Roland Mainz]

I think we should file a bug to get PL_strlen() removed from NSPR code.
|strlen()| is part of ANSI-C and there is AFAIK no need to keep it around...

brendan ?

Comment 31

[Brendan Eich [:brendan]]

As wtc wrote elsewhere, NSPR has been around for many years, and it keeps
compatibility according to some strict rules.  It's not about to remove the
PL_str* wrappers, on that account, but they are deprectated, and there is *no
good reason* to use them where standard C provides working counterparts, and
where no one passes in null.

/be

Comment 32

[Roland Mainz]

Brendan wrote:
> and there is *no good reason* to use them where standard C provides 
> working counterparts, and where no one passes in null.

Did you ever make a search how many people use PL_strlen() ? I think we should
put a LARGE comment (like "do not use - deprectated") into the header file and
source because some people still think that PL_strlen() is mandatory... ;-(

Comment 33

[Brendan Eich [:brendan]]

Roland: sorry I misspelled "deprecated" (a typo), please don't repeat my
mistake.  You might file a bug on NSPR asking for deprecation comments, but wtc
is cc'd here, so let's ask him first.

wtc, what do you say to deprecation comments?

/be

Comment 34

[Wan-Teh Chang]

Sure.  We should go one step further and implement PL_strxxx as a call
to strxxx if strxxx is a standard C library function.  This will reduce
the performance penalty of PL_strxxx to function call overhead and the
inability of inlining by compilers.

Comment 35

[Henry Jia]

Mmmmm... I agree with you guys. If you decide to do something more on this,
don't forget to tell me. I volunteer.

Comment 36

[Cathleen]

Attachment: final patch, remove all nsCRT::strlen(char* s)

attatchment 70750 had landed back in Feb (for 0.9.9).

This new patch cleans up all final traces and creep backs, over last 3 mo.

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

Shouldn't the patch also remove the nsCRT::strlen function itself, to ensure
that it's not used anymore?

Comment 38

[Cathleen]

yes, of course. removing nsCRT::strlen(char* s) is part of the latest patch.

Comment 39

[Brendan Eich [:brendan]]

Comment on attachment 86137 [details] [diff] [review]
final patch, remove all nsCRT::strlen(char* s)

sr=[e-mail]

/be

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

oh indeed.
but it leaves the comment about the function's usage in, shouldn't that be
removed as well?

Comment 41

[Suresh Duddi (gone)]

Comment on attachment 86137 [details] [diff] [review]
final patch, remove all nsCRT::strlen(char* s)

r=dp

Comment 42

[Cathleen]

Attachment: clean up of str* functions in nsCRT

this patch eliminates nsCRT::str* call instructions, and only native str*
appears in generated assembly code.

Comment 43

[Darin Fisher]

Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

sr=darin

Comment 44

[Darin Fisher]

Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

alec: can you review this.  thx!

Comment 45

[Bradley Baetz (:bbaetz)]

The nspr strcmp is null-safe, whilst the libc ones (and thus these replacements)
aren't. Hopefully noone tries to pass a null string in, though.

Comment 46

[Alec Flett]

Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

can't we use configure to check for stricmp and friends? I'm concerned that 
XP_UNIX won't be enough... a simple one or two line check in configure.in,
something like 
AC_CHECK_FUNCS(stricmp, strnicmp)

Comment 47

[Darin Fisher]

alec:

i originally wrote some configure.in checks, but they weren't being executed on
windows... asked seawood... and he said, use an #ifdef in the header file.
turns out most of the checks we run in configure.in are disabled for WIN32.

Comment 48

[Alec Flett]

Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

ah ok. r=alecf

Comment 49

[Cathleen]

yeah, darin said it.  :-) thanks guys! :-)  

Comment 50

[Brendan Eich [:brendan]]

bbaetz: you mean null pointer, not null string -- the latter is ambiguous, often
meaning an empty string.  Null-the-pointer (0 in a pointer context) is not a
valid C string (char * or const char *) value.  I hope we can safely assert this
in all of our code!

/be

Comment 51

[hacker formerly known as [e-mail]]

Just for clarification, I said that there's no need to add a configure check for
strlen because it's in the ANSI C standard and we're already using it (w/o
nsCRT:: or PL_) in numerous places in the code.  Unless we have reason to
believe strcasecmp isn't available on some platform, I wouldn't bother with a
check there either.  I'm guessing from the patch that it's not available on
win32 & os2 so we probably should add a check for it. (Not that we should hold
the patch for it.)

Comment 52

[Darin Fisher]

so, there are some hangups with this patch.  namely, linux (RH7.3) doesn't
provide strcasecmp or strncasecmp unless __USE_BSD is defined above #include
<string.h>.  that means we are going to need some configure.in magic... but i
worry that setting such defines (maybe _BSD_SOURCE is equivalent) globably would
have some side effect?

also, bbaetz's point is very good... i don't know if we want to bite the bullet
and risk unprotected strcmp.  we could just implement nsCRT::strcmp like this:

  PRInt32 strcmp(const char *s1, const char *s2)
  {
    if (!s1 || !s2)
      return PRInt32(s1 - s2);

    return ::strcmp(s1, s2);
  }

but now this might end up increasing code size :(

thoughts?

Comment 53

[Bradley Baetz (:bbaetz)]

brendan: Yeah, thats what I meant. I hope we can assert that too, but given how
long these have tested their arguments....

The compielr should be bale to optimise the inlined null check out when it can
prove that the result isn't null. I'm happy with it as is, but we could assert
and test for a few weeks if others aren't, and see if anyone gets caught. I jsut
wanted to mention it...

Comment 54

[hacker formerly known as [e-mail]]

I think the only adverse side-effect -D_BSD_SOURCE would have is to enable a
bunch of annoying warnings.  That was the reason we started using _POSIX_SOURCE
way back when.  We could just remove the *_SOURCE defines from configure.in and
have all of the default glibc API (which includes bsd routines) available.

Comment 55

[Darin Fisher]

seawood: yeah, there are several places where we have to add -D_BSD_SOURCE to
build specific files: xpcom/io, netwerk/base/src, netwerk/cache/src.  ftruncate
being one such system call that we use in necko.  if you are ok eliminating
_POSIX_SOURCE, then i think we should just do that.  it would greatly simplify
using the extended functions.

Comment 56

[Aaron Lehmann]

Bug 209499 is similar to this one. I have started to remove uses of PL_str*
functions. Seeking review.

Comment 57

[[e-mail]]

Attachment: ansify a few more nsCRT functions

this removes/cleans up a few methods to use ANSI C functions.

Comment 58

[[e-mail]]

oh, regarding the comment in strncasecmp - i checked all the usages in the tree,
and no one's abusing the return value. it seems that comment was added at the
same time as an identical one in xpcom/string, so it probably has no significance.

i also don't see what it means by "very negative numbers". PL_strncasecmp will
return the difference between two |char| values when the strings differ, so it
could at most be -128.

Comment 59

[Alec Flett]

Comment on attachment 135654 [details] [diff] [review]
ansify a few more nsCRT functions

did you try compiling with this? I feel like we're using some of the
nsCRT::strncmp stuff in templates..i.e. so you can apply it in

template<T>
int MyCompare(T* a, T* b) {
return nsCRT::strncmp(a, b, 100);
}

I tried to remove these once before and I think it was darin who objected for
that reason

anyway, sr=alecf on all the non-inlined stuff

for inline stuff like nsCRT::strncmp() which is just a direct forward to the
libc version, I don't feel like they're doing much harm (But maybe we still
need to update the comment there!)

Comment 60

[[e-mail]]

yup, i did build with this... but egads, looking at it now, there's a pretty
nice mistake(?) in the current nsCRT.h, and in my patch...

http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.h#144
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.h#164

so we have two of the same method, with different types of the integer arg! that
can't be right...

anyway, what would eventually be nice to do is make nsCRT a home for libc
wrappers that aren't available in ANSI C (like cathleen tried to do for
strcasecmp here). then we could start removing usages of the (inherently much
slower) PL_* functions that these replace, such as PL_strcasecmp.

eliminating the ANSI C wrappers we have now would also be nice, especially since
they call PL_str* versions which are inherently slower. those consumers that
explicitly need nullsafety could either add the checks themselves, or use the
PL_str* versions. but that's not too big of a deal.

anyway, which comment were you referring to alec? ;)

Comment 61

[[e-mail]]

Attachment: ansify a few more nsCRT functions v2

remove the duplicate fn... took the safe route and removed the non-nullsafe
one.

i'll fix the callers of this method to do the right thing at some other point.

Comment 62

[[e-mail]]

Comment on attachment 135660 [details] [diff] [review]
ansify a few more nsCRT functions v2

alec, could i get you to sr this again? :/

Comment 63

[Bradley Baetz (:bbaetz)]

Be very careful. Thats not a mistake, its on purpose, it appears. See bug 102980.

Comment 64

[[e-mail]]

hmm... nice. thanks bbaetz. so, my build compiles fine with the duplicate
function removed... are there any platforms that might break (i.e. if a consumer
was trying to pass a PRInt32, and the compiler decides the remaining PRUint32
prototype isn't compatible)?

otherwise i think we *should* be able to remove it... so my sr request still stands.

Comment 65

[Dan Mosedale (:dmosedale, :dmose)]

Since we no longer need to worry about Mac OS9, you should also be able to ditch
PL_strdup in favor of strdup, I think.

Comment 66

[Aaron Lehmann]

Have you checked that every platform has strdup? It is not part of the C
standard, at least not C90. I'd expect that some of the older UNIX platforms
lack it. strcat, strcmp, etc are good functions to use directly, but in general
strdup is not.

Comment 67

[[e-mail]]

i'm not too fussed about strdup... my patch breaks PL_strdup down into a
strlen/malloc/memcpy anyway, and the malloc is slow, so it's not perf-critical :)

if we wanted to do something like that, we'd probably have to go for a configure
test.

Comment 68

[Dan Mosedale (:dmosedale, :dmose)]

Aaron: as I remember it, when people used to check in code with strdup() in it,
Mac OS9 tinderboxen were the only ones that broke.  It's possible that I'm
misremembering, however.  As dwitte points out, an autoconf test would be ideal.

Comment 69

[Darin Fisher]

>i'm not too fussed about strdup... my patch breaks PL_strdup down into a
>strlen/malloc/memcpy anyway, and the malloc is slow, so it's not perf-critical :)

incidentally, dbaron mentioned that he compared GLIBC's strdup against
strlen+malloc+memcpy and found the latter to be faster!  the reason, he said,
had to do with strdup copying the data byte-by-byte.  memcpy on the other hand
copies data one WORD at a time (modulo special handling at the beginning and
end).  of course, this may have been fixed in more recent GLIBC versions :)

Comment 70

[[e-mail]]

in cvs head glibc sources, strdup does exactly that - strlen/malloc/memcpy. so i
doubt this is something to worry much about :)

Comment 71

[Alec Flett]

Comment on attachment 135660 [details] [diff] [review]
ansify a few more nsCRT functions v2

well then, sr=alecf

Comment 72

[Alfred Kayser]

*** Bug 102980 has been marked as a duplicate of this bug. ***

Comment 73

[Alfred Kayser]

in /netwerk/cache still nsCRT:: strdup, and ::strcmp are used, can these also be
replaced with PL_strdup and libc:strcmp?

Comment 74

[Alfred Kayser]

*** Bug 105482 has been marked as a duplicate of this bug. ***

Comment 75

[Christian :Biesinger (don't email me, ping me on IRC)]

> replaced with PL_strdup

be careful with allocators

Comment 76

[Alfred Kayser]

See bug 181295 for discussion on PL_strdup (that needs to match PL_strfree).

Comment 77

[Shriram (irc: Mavericks)]

Filing a blocking bug to replace the nsCRT::strcmp with the libc::strmcp