[PATCH] posix: Sync gnulib regex implementation

[Adhemerval Zanella]

On 29/06/2018 20:10, Paul Eggert wrote:
> On 06/29/2018 09:00 AM, Adhemerval Zanella wrote:
>> Paul, could you check what I am missing on the overflow check so we can
>> move forward? I really don't want to get this sync stalled because of
>> intprops.h addition as a pre-requisite.
> 
> I can add comments to  INT_ADD_WRAPV, if that would help. Please ask questions about it; that would help me understand what parts need better commenting.

I think we can get back to intprops.h addition on next 2.29 release.

> 
> I'm afraid that Florian's comment about testability is a bit of a red herring, as intprops.h is an internal header, not a published one. Glibc typically doesn't have test cases for macros defined in internal include files, and there's no need to test intprops.h directly. Besides, intprops.h and INT_ADD_WRAPV have been tested widely in commonly-used GNU applications like Coreutils, so it's not like I'm proposing anything flaky here.

We do have internal testing and situation for internal interfaces testing
are improving over the releases. But it is not clear to me what exactly
Florian opposes in intprops.h code and which parts he thinks would be
untestable using glibc testsuite (the header contains some code which
is not directly applicable to glibc, like various non-gnu compiler bugs
support, but I don't think it should prevent its inclusion).

> 
> That being said, this use of INT_ADD_WRAPV is unimportant (as far as I know, no Gnu apps use this two-buffer regex searching code any more), so I guess it's OK here to write a simple substitute that's good enough for regex even if it's not robust or general. The goal should be simplicity, portability, and meeting regex's needs. Proposed further Glibc patch attached (it assumes your patch). I've installed a similar patch into Gnulib.

Thanks, I sync in the patch I intend to push upstream.

> 
> I really don't want to get into the habit of doing this sort of thing, though. There's a lot of hard-won integer overflow expertise in intprops.h and it's a waste of time to continue to maintain poor imitations of it.

I tend to agree, I just did not want to drag the regex sync indefinitely 
due this specific code detail.

> 
> 
>> I tried to check the pos-processed implementation using intprops.h for
>> the snippet ... And its results are pretty much unreadable.
> Trying to audit this by looking at the preprocessed output is a bit like auditing glibc by looking at the generated machine code. It can be done, but you can't expect it to be easy. For your particular compiler I expect that the assembly-language output is more readable than the preprocessor output, so if you want to look at low level stuff I suggest looking at assembly language. But really, you should be better off reading the original source (and if it needs more comments, please say where and why and l'll add them).
> 
> 

I followed your suggestion and reading intprops.h code I indeed couldn't
find anything that should prevents us to include it on GLIBC (also mostly
of the complexity of the pos-processed code came from generic use, so I
take back it is 'unreadable').

Thanks for your review and work on gnulib side Paul, in attachments is 
the patch I indeed to push upstream on GLIBC. Basically it is just new
tests for the referenced bugzillas, and some tests additions from previous
discussions.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-posix-Sync-gnulib-regex-implementation.patch
Type: text/x-patch
Size: 228736 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20180703/3d54d58b/attachment.bin>