Re: [PATCH v2] Add __pure2 to __locale_ctype_ptr(_l)

[Craig Howland <howland at LGSInnovations dot com>]

On 10/31/2017 09:34 AM, Wilco Dijkstra wrote:
> The newlib ctype functons are extremely inefficient due to repeatedly
> calling __locale_ctype_ptr for every single use of a ctype macro, even
> in a tight loop.  Improve this by adding the missing __pure2 attribute so
> the pointer can be cached just like in GLIBC, resulting in > 2x speedup
> in loops.
>
> I haven't looked in detail at the implementation, but in general you don't
> need a function call - a variable, thread-local if required, would be simpler,
> even faster and cache automatically.
>
> ChangeLog:
> 2017-10-31  Wilco Dijkstra  <wdijkstr@arm.com>
>
>          * newlib/libc/include/ctype.h (__locale_ctype_ptr): Add __pure2.
>          (__locale_ctype_ptr_l): Likewise.
> --
> diff --git a/newlib/libc/include/ctype.h b/newlib/libc/include/ctype.h
> index 06458cbda47abe1294ebe226a16981fc9f38254d..a85e157a9019132fd7f9eceba690dcde07be2f3c 100644
> --- a/newlib/libc/include/ctype.h
> +++ b/newlib/libc/include/ctype.h
> @@ -66,7 +66,7 @@ extern int toascii_l (int __c, locale_t __l);
>   #define _X	0100
>   #define	_B	0200
>   
> -const char *__locale_ctype_ptr (void);
> +const char *__locale_ctype_ptr (void) __pure2;
>   # define __CTYPE_PTR	(__locale_ctype_ptr ())
>   
>   #ifndef __cplusplus
> @@ -100,7 +100,7 @@ const char *__locale_ctype_ptr (void);
>   #endif
>   
>   #if __POSIX_VISIBLE >= 200809
> -const char *__locale_ctype_ptr_l (locale_t);
> +const char *__locale_ctype_ptr_l (locale_t) __pure2;
>   #define __ctype_lookup_l(__c,__l) ((__locale_ctype_ptr_l(__l)+sizeof(""[__c]))[(int)(__c)])
>   
>   #define	isalpha_l(__c,__l)	(__ctype_lookup_l(__c,__l)&(_U|_L))
     The __pure2 attribute really is (maps to) the "const" attribute.  The GCC 
"const" attribute appears to not be allowed in this case.  Quoting from the GCC 
4.7.2 manual,
"Many functions do not examine any values except their arguments, and have
no effects except the return value. Basically this is just slightly more strict
class than the pure attribute below, since function is not allowed to read global
memory.
Note that a function that has pointer arguments and examines the data pointed
to must not be declared const."
     __locale_ctype_ptr() reads global memory. __locale_ctype_ptr_l() has a 
pointer argument and does examine the data pointed to (in that it returns a 
member from the given structure pointer).
     The GCC "pure" attribute is less strict, but even that does not appear to 
be appropriate/safe.  "Many functions have no effects except the return value 
and their return value
depends only on the parameters and/or global variables. Such a function can
be subject to common subexpression elimination and loop optimization just as
an arithmetic operator would be."  The problem is that a pointer is being used, 
not a plain variable.  So

for(i=0; i<strlen(str); i++)  { if(isalpha(str[i])) putchar(str[i]); }

could be safe, but

for(i=0; i<strlen(str); i++)  {
    if(isalpha(str[i])) putchar(str[i]);
    else setlocale(...); }

would probably not be.  This is a contrived example, but there is no way for the 
compiler to know the the locale has been changed since the pointer is still the 
same, being the underlying member which gets edited by the set.
     I think it would be safe when if !defined(_MB_CAPABLE), since the locale 
cannot be changed then, but not in the general case which is being changed by 
the proposed ctype.h edit.  That is, I think the patch can probably be OK if it 
is enhanced to be conditional for _MB_CAPABLE.  (It would be "dangerous" in the 
sense that it requires linkage between the header and the specific 
implementation of _setlocale_r() in a different file, but some comments can help 
with that.)
                Craig