On Thu, 2011-09-01 at 00:33 +0400, Dmitry V. Levin wrote:
> On Wed, Aug 31, 2011 at 10:32:55AM +0000, Denys Vlasenko wrote:
> > commit 2fb4db3e7aa1d6ac6b4b43f47597197492a846dd
> > Author: Denys Vlasenko <[email protected]>
> > Date: Wed Aug 31 12:26:03 2011 +0200
> >
> > Optimization: eliminate all remaining usages of strcat()
> >
> > After this change, we don't use strcat() anywhere.
> >
> > * defs.h: Change sprinttv() return type to char *.
> > * time.c (sprinttv): Return pointer past last stored char.
> > * desc.c (decode_select): Change printing logic in order to eliminate
> > usage of strcat() - use stpcpy(), *outptr++ = ch, sprintf() instead.
> > Also reduce usage of strlen().
> > * stream.c (decode_poll): Likewise.
>
> The idea behind this change is quite clear, but some hunks are questionable.
>
> > @@ -544,41 +545,42 @@ decode_select(struct tcb *tcp, long *args, enum
> > bitness_t bitness)
> > return RVAL_STR;
> > }
> >
> > - fds = (fd_set *) malloc(fdsize);
> > + fds = malloc(fdsize);
> > if (fds == NULL)
> > fprintf(stderr, "out of memory\n");
> >
> > - outstr[0] = '\0';
> > + tcp->auxstr = outstr;
>
> I suggest to initialize tcp->auxstr as late as possible (just in case
> there is a branch of code that might fail to initialize it properly),
> better right before returning RVAL_STR.
Good idea, I just committed a change which does exactly this.
> > + outptr = outstr;
> > + sep = "";
> > for (i = 0; i < 3; i++) {
> > int first = 1;
> >
> > - tcp->auxstr = outstr;
> > arg = args[i+1];
> > if (fds == NULL || !arg ||
> > umoven(tcp, arg, fdsize, (char *) fds) < 0)
> > continue;
> > for (j = 0; j < args[0]; j++) {
> > if (FD_ISSET(j, fds)) {
> > - char str[11 + 3 * sizeof(int)];
> > -
> > - if (first) {
> > - sprintf(str, "%s%s [%u", sep,
> > - i == 0 ? "in" :
> > - i == 1 ? "out" :
> > - "except", j);
> > - first = 0;
> > - sep = ", ";
> > + /* +2 chars needed at the end: ']',NUL
> > */
> > + if (outptr < end_outstr - (sizeof(",
> > except [") + sizeof(int)*3 + 2)) {
> > + if (first) {
> > + outptr +=
> > sprintf(outptr, "%s%s [%u",
> > + sep,
> > + i == 0 ? "in" :
> > i == 1 ? "out" : "except",
> > + j
> > + );
> > + first = 0;
> > + sep = ", ";
> > + }
> > + else {
> > + outptr +=
> > sprintf(outptr, " %u", j);
> > + }
> > }
> > - else
> > - sprintf(str, " %u", j);
>
> I'm not sure about this change. The new code is less readable and
> requires more room in outstr[] than the old code.
>
> > - cumlen += strlen(str);
> > - if (cumlen < sizeof(outstr))
> > - strcat(outstr, str);
Yes, it's possible that in a degenerate case old code would manage to
fit, say, a small integer into str via sprintf(str, " %u", j),
and then
cumlen += strlen(str); if (cumlen < sizeof(outstr)) strcat(outstr, str);
would successfully add it to almost full outstr.
But considering that outstr is "static char outstr[1024]", this can be
"fixed" by enlarging 1024 by about 40.
> > if (args[4]) {
> > - char str[128];
> > -
> > - sprintf(str, "%sleft ", sep);
> > - sprinttv(tcp, args[4], bitness, str + strlen(str));
> > - if ((cumlen += strlen(str)) < sizeof(outstr))
> > - strcat(outstr, str);
> > + if (outptr < end_outstr - 128) {
> > + outptr += sprintf(outptr, "%sleft ", sep);
> > + outptr = sprinttv(tcp, args[4], bitness,
> > outptr);
> > + }
>
> The new code requires more room in outstr[] than the old code.
Enlarging outstr would fix this.
How big do you want "static char outstr[]" to be here? 1060? 1100?
> > -void
> > +char *
> > sprinttv(struct tcb *tcp, long addr, enum bitness_t bitness, char *buf)
> > {
> > + int rc;
> > +
> > if (addr == 0)
> > - strcpy(buf, "NULL");
> > - else if (!verbose(tcp))
> > - sprintf(buf, "%#lx", addr);
> > - else {
> > - int rc;
> > + return stpcpy(buf, "NULL");
> >
> > - if (bitness == BITNESS_32
> > + if (!verbose(tcp)) {
> > + buf += sprintf(buf, "%#lx", addr);
> > + return buf;
> > + }
>
> Why not just write it simply
>
> if (!verbose(tcp))
> return buf + sprintf(buf, "%#lx", addr);
>
> > +
> > + if (bitness == BITNESS_32
> > #if defined(LINUX) && SUPPORTED_PERSONALITIES > 1
> > - || personality_wordsize[current_personality] == 4
> > + || personality_wordsize[current_personality] == 4
> > #endif
> > - )
> > - {
> > - struct timeval32 tv;
> > -
> > - rc = umove(tcp, addr, &tv);
> > - if (rc >= 0)
> > - sprintf(buf, "{%u, %u}",
> > - tv.tv_sec, tv.tv_usec);
> > - } else {
> > - struct timeval tv;
> > + )
> > + {
> > + struct timeval32 tv;
> > +
> > + rc = umove(tcp, addr, &tv);
> > + if (rc >= 0)
> > + buf += sprintf(buf, "{%u, %u}",
> > + tv.tv_sec, tv.tv_usec);
>
> Why not just return buf + sprintf here,
>
> > + } else {
> > + struct timeval tv;
> >
> > - rc = umove(tcp, addr, &tv);
> > - if (rc >= 0)
> > - sprintf(buf, "{%lu, %lu}",
> > - (unsigned long) tv.tv_sec,
> > - (unsigned long) tv.tv_usec);
> > - }
> > - if (rc < 0)
> > - strcpy(buf, "{...}");
> > + rc = umove(tcp, addr, &tv);
> > + if (rc >= 0)
> > + buf += sprintf(buf, "{%lu, %lu}",
> > + (unsigned long) tv.tv_sec,
> > + (unsigned long) tv.tv_usec);
>
> here and
>
> > }
> > + if (rc < 0)
> > + buf = stpcpy(buf, "{...}");
>
> here?
>
> > +
> > + return buf;
> > }
Good idea, pushed to git this version:
char *
sprinttv(struct tcb *tcp, long addr, enum bitness_t bitness, char *buf)
{
int rc;
if (addr == 0)
return stpcpy(buf, "NULL");
if (!verbose(tcp))
return buf + sprintf(buf, "%#lx", addr);
if (bitness == BITNESS_32
#if defined(LINUX) && SUPPORTED_PERSONALITIES > 1
|| personality_wordsize[current_personality] == 4
#endif
)
{
struct timeval32 tv;
rc = umove(tcp, addr, &tv);
if (rc >= 0)
return buf + sprintf(buf, "{%u, %u}",
tv.tv_sec, tv.tv_usec);
} else {
struct timeval tv;
rc = umove(tcp, addr, &tv);
if (rc >= 0)
return buf + sprintf(buf, "{%lu, %lu}",
(unsigned long) tv.tv_sec,
(unsigned long) tv.tv_usec);
}
return stpcpy(buf, "{...}");
}
--
vda
------------------------------------------------------------------------------
Special Offer -- Download ArcSight Logger for FREE!
Finally, a world-class log management solution at an even better
price-free! And you'll get a free "Love Thy Logs" t-shirt when you
download Logger. Secure your free ArcSight Logger TODAY!
http://p.sf.net/sfu/arcsisghtdev2dev
_______________________________________________
Strace-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/strace-devel
