This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] vfprintf: Remove alloca from string formatting with conversion
On 06/28/2017 08:46 PM, Adhemerval Zanella wrote:
>
>
> On 28/06/2017 15:13, Florian Weimer wrote:
>> On 06/27/2017 10:32 PM, Adhemerval Zanella wrote:
>>
>>> Wouldn't be better to add a wrapper on malloc/malloc-internal for overflow
>>> check? Something like
>>>
>>> static inline bool
>>> check_add_overflow_int_t (int left, int right, int *result)
>>> {
>>> #if __GNUC__ >= 5
>>> return __builtin_add_overflow (left, right, result);
>>> #else
>>> if (INT_MAX - left <= right)
>>> return true;
>>> *result = left + right;
>>> return false;
>>> #endif
>>> }
>>>
>>> As we already do for multiplication and for unsigned (from my char_array)?
>>
>> I'd prefer to wait until we can require GCC 5 as the baseline compiler
>> and then use the built-ins directly. Less cognitive load for the
>> maintainers.
>
> This would take at least one or two more releases and I do not see why
> add and follow an idea already in place for malloc.
GCC 4.9 is already out of support upstream, so I expect we will switch
to GCC 5 during the next cycle.
>>>> + /* Use a small buffer to combine processing of multiple characters.
>>>> + CONVERT_FROM_OTHER_STRING expects the buffer size in (wide)
>>>> + characters, and buf_length counts that. */
>>>> +#ifdef COMPILE_WPRINTF
>>>> + enum { buf_length = 64 };
>>>> + wchar_t buf[buf_length];
>>>> +#else
>>>> + enum { buf_length = 256 };
>>>> + char buf[buf_length];
>>>> + _Static_assert (sizeof (buf) > MB_LEN_MAX,
>>>> + "buffer is large enough for a single multi-byte character");
>>>> +#endif
>>>
>>> Why different 'buf_length' sizes if you using the required type for array
>>> creation?
>>
>> I don't understand the question. I want to avoid a large stack frame.
>> Regarding the different constants, see the comment above.
>
> If the buffer must be computed as wide characters, why not defined it
> using wchar_t regardless:
>
> wchar_t wbuf[64];
> const size_t buf_length = sizeof (wbuf) / sizeof (CHAR_T);
>
> ?
Well, it would have to be like this:
enum { buf_length = 256 / sizeof (CHAR_T) };
CHAR_T buf[buf_length];
I think this should work.
(I've already posted a test which adds coverage for this code and would
check that in first.)
Thanks,
Florian