RFC: Locale-independent case conversion #7506

tstarling · 2021-09-22T12:44:47Z

Implementation of RFC https://wiki.php.net/rfc/strtolower-ascii

Commit 1: Use ASCII case conversion consistently. See the RFC for consequences.
Commit 2: Add some tests since if I changed the magic numbers in the SSE implementation of strtoupper, nothing failed
Commit 3: Add ctype_tolower() and ctype_toupper() as proposed in the RFC.

I wanted to reuse the existing zend_string_tolower(), but there was no zend_string_toupper to go with it. So I added a family of toupper functions to zend_operators.c to go with the tolower functions.

Instead of splitting out a non-inline zend_tolower_ascii(), I realise now that I could have added extern declarations for tolower_map and toupper_map. I think I will try that tomorrow.

I replaced php_strtolower() with zend_str_tolower() in most places for performance reasons. They don't quite do the same thing -- php_strtolower() returns its argument. So it's not just a matter of #define php_strtolower zend_str_tolower.

zend_str_toupper_impl() is not called by anything and so is untested. But the similar SSE code in zend_string_toupper_ex is tested.

Zend/zend_operators.c

TysonAndre · 2021-09-23T13:48:18Z

main/rfc1867.c

@@ -714,7 +713,7 @@ SAPI_API SAPI_POST_HANDLER_FUNC(rfc1867_post_handler) /* {{{ */
 		int content_type_len = (int)strlen(content_type_dup);
 		char *content_type_lcase = estrndup(content_type_dup, content_type_len);

-		php_strtolower(content_type_lcase, content_type_len);
+		zend_str_tolower(content_type_lcase, content_type_len);


Good catch - I think this may have made sense regardless for http headers but outside of the scope of this RFC

e.g. this is done in https://golang.org/src/net/http/internal/ascii/print.go - the rfc from 1995 https://datatracker.ietf.org/doc/html/rfc1867 makes no mention of locale so I think it was assuming everything was using the english locale

More of an issue in ext/standard/http_fopen_wrapper.c for parsing request headers than here

EDIT: Though "boundary" would be the same no matter what locale for any locale I know of, so not a bug in this file

RFC 1867 says "Note that mime headers are generally required to consist only of 7-bit data in the US-ASCII character set."

ext/intl/grapheme/grapheme_string.c

Headers should not be processed in a locale-depdendent fashion. Switch from upper to lowercasing because that's the standard for PHP and we provide an ASCII implementation of this operation. This is adapted from GH-7506.

nikic · 2021-09-23T15:24:39Z

All of the changes to "miscellaneous" extensions you've made here look like plain bug fixes to me -- we should land these independently of this RFC, and I think those should also go into PHP-8.1. Would you mind submitting a PR with just those changes?

I landed the mbstring change that was using toupper in 46315de, but the rest looks like it should be directly applicable without the new toupper functionality.

This will help reduce the scope of the RFC itself.

tstarling · 2021-09-24T01:18:37Z

All of the changes to "miscellaneous" extensions you've made here look like plain bug fixes to me -- we should land these independently of this RFC, and I think those should also go into PHP-8.1. Would you mind submitting a PR with just those changes?

Sure, will do.

tstarling · 2021-09-24T01:45:22Z

The changes to ext/pdo_dblib and ext/xml require zend_str_toupper(). So I'll leave them out of the misc PR. Or I can put the introduction of zend_str_toupper() in the misc PR if you prefer.

tstarling · 2021-09-24T02:16:42Z

The force push removed the misc bug fixes from the first commit so that it won't conflict with #7511.

morrisonlevi · 2021-09-24T06:10:58Z

Zend/zend_operators.c

+		const __m128i _a = _mm_set1_epi8('a' - 1);
+		const __m128i z_ = _mm_set1_epi8('z' + 1);
+		const __m128i delta = _mm_set1_epi8('a' - 'A');
+		do {
+			__m128i op = _mm_loadu_si128((__m128i*)p);
+			__m128i gt = _mm_cmpgt_epi8(op, _a);
+			__m128i lt = _mm_cmplt_epi8(op, z_);
+			__m128i mingle = _mm_and_si128(gt, lt);
+			__m128i sub = _mm_and_si128(mingle, delta);
+			__m128i upper = _mm_sub_epi8(op, sub);
+			_mm_storeu_si128((__m128i *)q, upper);
+			p += 16;
+			q += 16;
+		} while (p + 16 <= end);


We can improve slightly on the p + 16 <= end by defining z to be end rounded down to a multiple of 16 and then simply compare p != z. Like this, if I haven't made a mistake (I'm quite tired, so likely):

Suggested change

const __m128i _a = _mm_set1_epi8('a' - 1);

const __m128i z_ = _mm_set1_epi8('z' + 1);

const __m128i delta = _mm_set1_epi8('a' - 'A');

do {

__m128i op = _mm_loadu_si128((__m128i*)p);

__m128i gt = _mm_cmpgt_epi8(op, _a);

__m128i lt = _mm_cmplt_epi8(op, z_);

__m128i mingle = _mm_and_si128(gt, lt);

__m128i sub = _mm_and_si128(mingle, delta);

__m128i upper = _mm_sub_epi8(op, sub);

_mm_storeu_si128((__m128i *)q, upper);

p += 16;

q += 16;

} while (p + 16 <= end);

// round end down to multiple of 16

static const uintptr_t mask = 15u;

unsigned char *z = (unsigned char*)(((uintptr_t)end) & ~mask);

const __m128i _a = _mm_set1_epi8('a' - 1);

const __m128i z_ = _mm_set1_epi8('z' + 1);

const __m128i delta = _mm_set1_epi8('a' - 'A');

do {

__m128i op = _mm_loadu_si128((__m128i*)p);

__m128i gt = _mm_cmpgt_epi8(op, _a);

__m128i lt = _mm_cmplt_epi8(op, z_);

__m128i mingle = _mm_and_si128(gt, lt);

__m128i sub = _mm_and_si128(mingle, delta);

__m128i upper = _mm_sub_epi8(op, sub);

_mm_storeu_si128((__m128i *)q, upper);

p += 16;

q += 16;

} while (p != z);

The assembly of the inner loop at -O2 is just

.L535: movdqu -16(%rbx), %xmm1 addq $16, %rbx addq $16, %rax movdqa %xmm1, %xmm2 movdqa %xmm1, %xmm0 pcmpgtb %xmm4, %xmm2 pcmpgtb %xmm5, %xmm0 pandn %xmm6, %xmm2 pand %xmm2, %xmm0 pand %xmm3, %xmm0 paddb %xmm1, %xmm0 movups %xmm0, -16(%rax) cmpq %rbx, %rbp jnb .L535

%rbx holds p+16 which it uses for the loop comparison, and then it loads from %rbx-16. I don't think there's any overhead associated with the -16 offset -- my understanding is that the addressing modes all happen in the same clock cycle.

It's not correct to mask out the lower bits of the end pointer as you suggest, because the input is not aligned, hence the use of the loadu builtin.

It's not correct to mask out the lower bits of the end pointer as you suggest, because the input is not aligned, hence the use of the loadu builtin.

Right, sorry, was tired. The mask can be done on length instead, I think? Anyway, the ASM is not as clean as you make it out to be on any compiler I've chosen on godbolt; not sure what's different. Here's an A/B comparison using the latest GCC: https://godbolt.org/z/PM5zT9aTE. Whether I did the optimization correctly or not, you can still see that the ASM isn't as clean as what you posted above and can tinker with flags/compilers to figure out the difference.

nikic · 2021-09-24T07:29:10Z

The changes to ext/pdo_dblib and ext/xml require zend_str_toupper(). So I'll leave them out of the misc PR. Or I can put the introduction of zend_str_toupper() in the misc PR if you prefer.

That's what I was going to suggest next :) We can land the new toupper implementations separately, both for internal use and as an optimization in strtoupper(), same as we currently do in strtolower().

tstarling · 2021-09-28T00:08:00Z

That's what I was going to suggest next :) We can land the new toupper implementations separately, both for internal use and as an optimization in strtoupper(), same as we currently do in strtolower().

This is #7521.

nikic · 2021-09-29T11:06:57Z

This should become a lot simpler after a rebase :)

TysonAndre · 2021-12-11T17:49:39Z

. strtolower() and strtoupper() are no longer locale-sensitive. They now
perform ASCII case conversion. Use mb_strtolower() if you want localized
case conversion. Similarly, stristr, stripos, strripos, lcfirst, ucfirst,
ucwords, str_ireplace, array_change_key_case and sorting with SORT_FLAG_CASE
use ASCII case conversion.

It may be clearer to qualify that with ASCII case conversion as if the locale were "C". Or ASCII case conversion without localization.
As a followup that can be done outside this PR, Zend/Optimizer/sccp.c can also be changed for the functions that now use ASCII case conversion, for can_ct_eval_func_call

static bool can_ct_eval_func_call(zend_string *name, uint32_t num_args, zval **args) {
	/* Functions in this list must always produce the same result for the same arguments,
	 * and have no dependence on global state (such as locales). It is okay if they throw
	 * or warn on invalid arguments, as we detect this and will discard the evaluation result. */
	if (false
		|| zend_string_equals_literal(name, "array_diff")
		|| zend_string_equals_literal(name, "array_diff_assoc")
		|| zend_string_equals_literal(name, "array_diff_key")

The earlier build failure was from running out of CI minutes, which was fixed. This can be rebased and tested again

Implement RFC https://wiki.php.net/rfc/strtolower-ascii

tstarling · 2021-12-14T00:47:36Z

It may be clearer to qualify that with ASCII case conversion as if the locale were "C". Or ASCII case conversion without localization.

Done.

TysonAndre · 2021-12-14T14:11:23Z

Is this ready to be merged?

dstogov

I don't see any problems

https://wiki.php.net/rfc/strtolower-ascii means that these functions no longer depend on the current locale in php 8.2. Before that, this was unsafe to evaluate at compile time. Followup to phpGH-7506

…uation. https://wiki.php.net/rfc/strtolower-ascii means that these functions no longer depend on the current locale in php 8.2. Before that, this was unsafe to evaluate at compile time. Followup to phpGH-7506 Add strcmp/strcasecmp/strtolower/strtoupper functions Add bin2hex/hex2bin and related functions Add array_key_first/array_key_last Update test of garbage collection using strtolower - Make sure that this tests the right thing when opcache is enabled. Update this as an example for future tests.

…uation, add functions. (#7780) https://wiki.php.net/rfc/strtolower-ascii means that these functions no longer depend on the current locale in php 8.2. Before that, this was unsafe to evaluate at compile time. Followup to GH-7506 Add strcmp/strcasecmp/strtolower/strtoupper functions Add bin2hex/hex2bin and related functions Update test of garbage collection using strtolower to use something else to create a refcounted string

cmb69 added the RFC label Sep 22, 2021

tstarling force-pushed the tolower-ascii branch from 123025d to d072b91 Compare September 23, 2021 01:35

dstogov reviewed Sep 23, 2021

View reviewed changes

Zend/zend_operators.c Outdated Show resolved Hide resolved

TysonAndre reviewed Sep 23, 2021

View reviewed changes

ext/intl/grapheme/grapheme_string.c Outdated Show resolved Hide resolved

tstarling mentioned this pull request Sep 24, 2021

Use ASCII lower case for misc case folding #7511

Closed

tstarling force-pushed the tolower-ascii branch from c00f865 to fdc8ff3 Compare September 24, 2021 02:15

morrisonlevi reviewed Sep 24, 2021

View reviewed changes

tstarling force-pushed the tolower-ascii branch from b1883a1 to 585d34c Compare October 11, 2021 01:04

tstarling force-pushed the tolower-ascii branch from 585d34c to 641927c Compare November 25, 2021 04:07

Make strtolower() and strtoupper() do ASCII case conversion

6fdf447

Implement RFC https://wiki.php.net/rfc/strtolower-ascii

tstarling force-pushed the tolower-ascii branch from 641927c to 6fdf447 Compare December 14, 2021 00:47

TysonAndre approved these changes Dec 14, 2021

View reviewed changes

dstogov approved these changes Dec 15, 2021

View reviewed changes

TysonAndre merged commit 8eee0d6 into php:master Dec 15, 2021

TysonAndre mentioned this pull request Dec 15, 2021

Allow internal functions to declare if they support compile-time evaluation. #7780

Merged

RFC: Locale-independent case conversion #7506

RFC: Locale-independent case conversion #7506

Uh oh!

Conversation

tstarling commented Sep 22, 2021

Uh oh!

Uh oh!

TysonAndre Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstarling Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic commented Sep 23, 2021

Uh oh!

tstarling commented Sep 24, 2021

Uh oh!

tstarling commented Sep 24, 2021

Uh oh!

tstarling commented Sep 24, 2021

Uh oh!

morrisonlevi Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

tstarling Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

morrisonlevi Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

nikic commented Sep 24, 2021

Uh oh!

tstarling commented Sep 28, 2021

Uh oh!

nikic commented Sep 29, 2021

Uh oh!

TysonAndre commented Dec 11, 2021

Uh oh!

tstarling commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TysonAndre commented Dec 14, 2021

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TysonAndre Sep 23, 2021 •

edited

Loading

tstarling commented Dec 14, 2021 •

edited

Loading