Paul Eggert wrote:
> The first thing I noticed was this comment about two size_t variables:
>
> if (len > size - 1) /* equivalent to: (size > 0 && len >= size) */
>
> The comment is not correct if size_t promotes to int
This is a border case; you can fix it by adding a cast:
if (len > (size_t)(size - 1))
> and anyway the
> code would be clearer and typically just as fast if it were written if
> (0 < size && size <= len) since compilers typically optimize this into
> a single comparison these days.
No, they don't. Take "gcc-4.1.1 -O2" on this test file:
===================== foo.c =====================
typedef unsigned int size_t;
int single_cond_jump (size_t len, size_t size)
{
if (len > size - 1)
return len;
else
return -1;
}
int two_cond_jumps (size_t len, size_t size)
{
if (size > 0 && len >= size)
return len;
else
return -1;
}
===================== foo.s =======================
.file "foo.c"
.text
.p2align 4,,15
.globl single_cond_jump
.type single_cond_jump, @function
single_cond_jump:
pushl %ebp
movl $-1, %edx
movl %esp, %ebp
movl 12(%ebp), %eax
movl 8(%ebp), %ecx
decl %eax
cmpl %ecx, %eax
jae .L4
movl %ecx, %edx
.L4:
popl %ebp
movl %edx, %eax
ret
.size single_cond_jump, .-single_cond_jump
.p2align 4,,15
.globl two_cond_jumps
.type two_cond_jumps, @function
two_cond_jumps:
pushl %ebp
movl %esp, %ebp
movl 12(%ebp), %edx
testl %edx, %edx
je .L10
movl 8(%ebp), %eax
cmpl %eax, %edx
ja .L10
popl %ebp
ret
.p2align 4,,7
.L10:
popl %ebp
movl $-1, %eax
.p2align 4,,4
ret
.size two_cond_jumps, .-two_cond_jumps
.ident "GCC: (GNU) 4.1.1"
.section .note.GNU-stack,"",@progbits
============================================================
You see the that gcc turns the two comparisons into two conditional jumps.
It is well-known that jumps affect performance a lot on modern CPUs.
gcc converts two comparisons like this to a single conditional jump only
when both bounds are constant. Not when, like here, one of them is a
variable.
> However, when I looked into the surrounding code I noticed that the
> logic was wrong anyway, since it didn't properly copy the generated
> string into the output buffer when the string is too long.
Right, this is a major bug. Thanks for noticing it!
> While we're on the subject, snprintf should
> check for EOVERFLOW, since it's the interface that stupidly tries to
> copy a size_t into an int, so I changed it to do that.
This is not needed. vasnprintf already does this check.
Still one bug remaining: Your new code will crash when passed an str = NULL
argument.
Furthermore I don't like that your memcpy copies more than needed.
The vasnprintf code needs memory in chunks, is not looking for it byte
for byte. Therefore it can happen that output != str and still
0 <= len < size - and then you are copying around more memory than needed.
Simon, ok to apply this?
*** snprintf.c 10 Aug 2006 19:32:38 -0000 1.8
--- snprintf.c 11 Aug 2006 13:43:12 -0000
***************
*** 1,6 ****
/* Formatted output to strings.
Copyright (C) 2004, 2006 Free Software Foundation, Inc.
! Written by Simon Josefsson and Paul Eggert.
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
--- 1,6 ----
/* Formatted output to strings.
Copyright (C) 2004, 2006 Free Software Foundation, Inc.
! Written by Simon Josefsson, Paul Eggert, Bruno Haible.
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
***************
*** 22,40 ****
#include "snprintf.h"
- #include <errno.h>
- #include <limits.h>
#include <stdarg.h>
#include <stdlib.h>
#include <string.h>
#include "vasnprintf.h"
- /* Some systems, like OSF/1 4.0 and Woe32, don't have EOVERFLOW. */
- #ifndef EOVERFLOW
- # define EOVERFLOW E2BIG
- #endif
-
/* Print formatted output to string STR. Similar to sprintf, but
additional length SIZE limit how much is written into STR. Returns
string length of formatted string (which may be larger than SIZE).
--- 22,33 ----
***************
*** 57,76 ****
if (output != str)
{
! if (size)
{
! memcpy (str, output, size - 1);
! str[size - 1] = '\0';
}
free (output);
}
! if (INT_MAX < len)
! {
! errno = EOVERFLOW;
! return -1;
! }
return len;
}
--- 50,71 ----
if (output != str)
{
! /* output was malloc()ed by the vasnprintf function. */
!
! if (str != NULL && size > 0)
{
! /* Copy contents of output to str. */
! size_t pruned_len = (len >= size ? size - 1 : len);
!
! memcpy (str, output, pruned_len);
! str[pruned_len] = '\0';
}
free (output);
}
! /* No need to test against len > INT_MAX. vasnprintf already handled
! this case. */
return len;
}
