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

[Craig Howland <howland at LGSInnovations dot com>]

On 11/07/2017 11:39 AM, Wilco Dijkstra wrote:
> Craig wrote:
>
> > All of this might be moot due to pure2 appearing to not be valid for the general
> > __locale_ctype_ptr case.  (Even if it would be safe for the specific test cases being discussed.)
> > Would you please comment on those concerns?
> > (https://sourceware.org/ml/newlib/2017/msg01055.html in case it did not make it to you originally.)
> I don't understand your concern. This is how ctype works, GLIBC uses the same attribute.
> You inline the ctype lookup but (for various reasons) may want to hide the ctype pointer
> behind an expensive function call. The call must be marked in such a way that it can be CSEd
> and lifted out of loops. If you didn't want this to happen there would be no point in providing
> complex headers with inline functions for ctype.
>
> Wilco
     The concerns are exactly as spelled out in 
https://sourceware.org/ml/newlib/2017/msg01055.html. ; I won't re-quote, but to 
rephrase the concern a little:  The GCC definition has some precise requirements 
on when const (Newlib's "pure2") may be applied, and putting it on 
__locale_ctype_ptr() and __locale_ctype_ptr_l() both look like clear violations 
of those requirements.  Given the violation of the requirements, it means that 
in general operation might not be correct.  There might also be cases where 
operation would be correct--and this is perhaps even the expected case, but any 
failure case is too many to be OK.  (I have not followed a call entirely through 
to the bitter end of what a ctype function looks up.  If that cannot 
change--despite a different pointer possibly being returned by 
__locale_ctype_ptr(), for example, then perhaps it is OK after all.  But that 
does not appear to be the case from an intermediate examination (see the next 
paragraph).  If you think that I have somehow misread or misapplied the rules 
against these functions, please explain how you think they don't apply.)
     I agree that a reason for ctype to be a global table is to help speed 
things up, but that does not mean all implementations are safe for CSE.  
Newlib's pre-locale implementation was, but not the present with-locale one.  
For example, __locale_ctype_ptr() returns __get_current_locale()->ctype_ptr, and 
__get_current_locale() returns _REENT->_locale or __get_global_locale().  This 
means that the pointer can change--unsafe for the function calls to be moved out 
of a general loop.  If the call is hoisted out of a loop, the loop will start 
being wrong after the value changes (if it changes).  That is, it is possible 
for the ctype table pointer to be changed during the loop, so that if the 
compiler has moved the pointer fetch out of the loop failure will result 
following a locale change inside the loop.
     All of this discussion is for the most general case with locale and 
multibyte, meaning the pointer can change.  I think that when multibyte support 
is off the locale cannot actually be changed, so then the pure2 would actually 
be safe, but that consideration is not part of the present proposed patch.  This 
needs some more checking, but might be allow the pure2 to be conditionally 
applied so that the improvement can be had for some configurations.
     Saying that GLIBC does it might be a proof of concept, but really is 
meaningless when discussing a Newlib patch because their implementation is not 
the same as ours.  The question at hand is the Newlib implementation.  (Given 
that ours clearly fails the tests for GCC const, the GLIBC implementation is 
apparently fundamentally different than ours.)  I have not looked at their 
implementation except for a quick glance at the header files.  It certainly is 
possible that it is different in such a manner that pure2/const is OK.  For 
example, perhaps GLIBC alters the contents of the table, itself, when changing 
locales, rather than changing the pointer to the table.  The comments in their 
ctype.h do talk about pointing into "arrays of 384".  Given a fixed length on 
all locale arrays, it would allow a locale change to change the array instead of 
the pointer.  Sure, it would make the locale change, itself, more expensive, but 
it then could allow the ctype calls to be the least expensive.  (This is a 
possible change that could be made to allow for best performance in all 
configurations, I suppose, but would be a lot more work.)
                Craig