From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/13490 Path: news.gmane.org!.POSTED!not-for-mail From: Xan Phung Newsgroups: gmane.linux.lib.musl.general Subject: Re: stdio glitch & questions Date: Sat, 1 Dec 2018 13:42:30 +1100 Message-ID: References: <20181130160951.GS23599@brightrain.aerifal.cx> <20181201000229.GT23599@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="0000000000003203c4057bece0d6" X-Trace: blaine.gmane.org 1543632078 9475 195.159.176.226 (1 Dec 2018 02:41:18 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 1 Dec 2018 02:41:18 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-13506-gllmg-musl=m.gmane.org@lists.openwall.com Sat Dec 01 03:41:13 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1gSvDT-0002M0-Hc for gllmg-musl@m.gmane.org; Sat, 01 Dec 2018 03:41:11 +0100 Original-Received: (qmail 25721 invoked by uid 550); 1 Dec 2018 02:43:20 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 25700 invoked from network); 1 Dec 2018 02:43:19 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=5LXQxbZPQcKlZWd9Q50XUXUiV9BrwRlFmTHz/QGd+To=; b=o9xZEKfXKWS3ZJlRcHt9EUCSHjymoeMPl9oDeWuDHvtYdb1vjC8vxBOuo+tv+p1y90 gnFW+srzMcmPU90kqqkZgQtI6UoAmnSEQw0m0qAI7wiTnUWGOAry1PewXiJDDkIvjJMd 6sZrijT9ftppp9WsyFIry3sX9iHvT2UlolvyWSpnV23o8RLskGEhzyPzrBxFV9ck+bDR oy+5G6SFKpiAXjOhB2AinQFASs1sMUdeKXsfsfZJsgM1UqVOAO4dI5z9irUKMWvVSrUE qZFvOVdoaMcxAQmyYKQgaQVuYUlBmpvbZcYx88G0l8AG7TjmBPF4A/4picz7pRUw9744 x3IA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=5LXQxbZPQcKlZWd9Q50XUXUiV9BrwRlFmTHz/QGd+To=; b=JbFAM4jcBxP91PR6RRPaPNbEYIEbCG2FA9vU0Wg3lXC3AvIRoD6xAPcq509xAK1uob 5Yx7/Fw6h9BaCN6tLnl9GgM3ogdCxPH+Kw2yb/ydjhLUGuGY3yYB38xzR9nnvSrK33fz ZJAanH0Dopxjf4b26j203nQP6njAVEanH2Cldt7No4qWws53Wuv1DNLD89pCL3l3PevA jo942rhA/T1I2pXYMxkUGFhfIgQ6HdqaeRkxe17sGMl/3aihZMAx/AnzzL6eZS9u0+Em c9RSrmqZVdZsk1aGZjOrbfDqW9FhzCE5YLraeZVBwp7GsLerljfKIbfjBvKYuOwnxvzM YYfg== X-Gm-Message-State: AA+aEWZHWGoFW6L/L4LeFXqSvCP/xaPUtEIcwCwWo+dTvNoV6u3Rhl3H txkkOeIOG5smGSebAyGehY9vCeX+JIkcUebjuxgDXZ9k X-Google-Smtp-Source: AFSGD/U/yEN4S4nfoWkGfvYU2XD128purLscEvDA5tXFFMGa//Tlcb6+9fMKucxpMEadYEqPei8sdNpRqY/yghU+Yoo= X-Received: by 2002:a9f:314c:: with SMTP id n12mr3697392uab.33.1543632187424; Fri, 30 Nov 2018 18:43:07 -0800 (PST) In-Reply-To: <20181201000229.GT23599@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:13490 Archived-At: --0000000000003203c4057bece0d6 Content-Type: text/plain; charset="UTF-8" On Sat, 1 Dec 2018 at 11:02, Rich Felker wrote: > > I've been trying to understand what you're trying to do. It seems you > chose to work at the point of line-buffered flush logic, since that > happens to be the only case where f->write is called with an argument > that might fit in the remaining buffer space. > > Yes that's correct... Also, the other reason I chose __fwrite.c is it's the only place where the search for '\n' is done. An additional objective I had was to split the loop searching for '\n' into two stages: (i) Stage 1: search for '\n' word by word ie: 8 bytes at a time ... if '\n' found at >~16 bytes before start of "s" array then go straight to f->write without copying bytes (I don't show it in my code, but the algorithm to do stage 1 would be similar to memchr.c) (ii) Stage 2: in final 9~16 bytes, drop down into byte-by-byte searching for '\n' (also doing copy of residual bytes into buffer when '\n' found) > As written the alignment logic and pointer arithmetic is invalid; the > sums/differences are out of bounds of the array, and i<=0 is not > meaningful since i has an unsigned type (and so does l+s-t). But even > if it could be made correct, it's all completely unnecessary and just > making the code slower and less readable. If __fwritex were the right > place for this code, all you would need to > do is check whether i<16 (or whatever threshold) before calling > f->write, and if so, memcpy'ing it to the buffer then calling f->write > with a length of 0. I agree, the check for i <= X is a simpler way of expressing the algorithm. [X is a value between 9 and 16 that guarantees (s + X) will be word aligned] In my version, I introduced a new array base "t" and rebase i to index into "t" because "t" is a guaranteed word aligned pointer. However, this word alignment of "t" is not exploited in the current byte-at-a-time searching for '\n', so yes at present it's unnecessary. > However, then you could not use the return value > of f->write to determine if it succeeded (see how fflush and fseek > have to deal with this case). Contrary to what your code assumes, > f->write does not (and cannot, since the type is unsigned) return a > negative value on error. > > Ok, noted, the expression to check for error should be (!f->wpos) and not n < 0 Instead, I think it probably makes more sense to put the logic in > __stdio_write, but this will also be somewhat nontrivial to work in. > At least the "iovcnt == 2 ? ..." logic needs to be adapted to > something like "rem > len ? ...". Before the loop should probably be > something like "if (len < f->wend-f->wpos && len <= 16) ..." to > conditionally copy the new data into the buffer. > > Do you see any reason to prefer doing it in __fwritex? > > Wouldn't putting check for i < X in __fwrite.c better because it: (a) facilitates word-by-word searching objective outlined above (b) check for space in buffer is already done in line 10 of __fwrite.c, hence avoids need for any more buffer length checks (c) ?? speeds up writes which doesn't use __stdio_write --0000000000003203c4057bece0d6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Sat, 1 Dec 2018 at 11:02, Rich Felker <dalias@libc.org> wrote:

I've been trying to understand what you're trying to do. It seems y= ou
chose to work at the point of line-buffered flush logic, since that
happens to be the only case where f->write is called with an argument that might fit in the remaining buffer space.


Yes that's correct... Also, the ot= her reason I chose __fwrite.c is it's the only place where the search f= or '\n' is done.

An additional objective I= had was to split the loop searching for '\n' into two stages:
(i) Stage 1: search for '\n' word by word ie: 8 bytes at a ti= me ... if '\n' found at >~16 bytes before start of "s"= array then go straight to f->write without copying bytes
(I d= on't show it in my code, but the algorithm to do stage 1 would be simil= ar to memchr.c)
(ii) Stage 2: in final 9~16 bytes, drop down into= byte-by-byte searching for=C2=A0 '\n' (also doing copy of residual= bytes into buffer when '\n' found)
=C2=A0
As written the alignment logic and pointer arithmetic is invalid; the
sums/differences are out of bounds of the array, and i<=3D0 is not
meaningful since i has an unsigned type (and so does l+s-t). But even
if it could be made correct, it's all completely unnecessary and just making the code slower and less readable. If __fwritex were the right place= for this code, all you would need to
do is check whether i<16 (or whatever threshold) before calling
f->write, and if so, memcpy'ing it to the buffer then calling f->= write
with a length of 0.

I agree, the check for = i <=3D X is a simpler way of expressing the algorithm.
[X is a= value between 9 and 16 that guarantees (s + X) will be word aligned]
=

In my version, I introduced a new array base "t&qu= ot; and rebase i to index into "t" because "t" is a gua= ranteed word aligned pointer.
However, this word alignment of &qu= ot;t" is not exploited in the current=C2=A0 byte-at-a-time searching f= or '\n', so yes at present it's unnecessary.
=C2=A0
However, then you= could not use the return value
of f->write to determine if it succeeded (see how fflush and fseek
have to deal with this case). Contrary to what your code assumes,
f->write does not (and cannot, since the type is unsigned) return a
negative value on error.


Ok, noted, the expression to check for= error should be (!f->wpos) and not n < 0

Instead, I think it probably makes more sense to put the logic in
__stdio_write, but this will also be somewhat nontrivial to work in.
At least the "iovcnt =3D=3D 2 ? ..." logic needs to be adapted to=
something like "rem > len ? ...". Before the loop should proba= bly be
something like "if (len < f->wend-f->wpos && len <= =3D 16) ..." to
conditionally copy the new data into the buffer.

Do you see any reason to prefer doing it in __fwritex?

=
Wouldn't putting check for i < X in __fwrite.c better because i= t:
(a) facilitates word-by-word searching objective outlined abov= e
(b) check for space in buffer is already done in line 10 of= __fwrite.c, hence avoids need for any more buffer length checks
<= div>
(c) ?? speeds up writes which doesn't use __stdio_write
<= br class=3D"gmail-Apple-interchange-newline">
=C2=A0
<= /div>
--0000000000003203c4057bece0d6--