[libc++][bug] double-precision formatter allocates on heap with default precision
- Dominant language
- LLVM
- Stars
- 40.5k
- Forks
- 18.7k
- PR merge metrics
- PR metrics pending
Description
While migrating from fmt to std::format, I stumbled upon this apparent bug when formatting a double with default precision (aka no precision spec) where it unexpectedly allocates memory on the heap. Based on the comments in the code and the initial review (https://reviews.llvm.org/D114001), that does not seem to be intentional, and thus, a bug.
The relevant code is in `libcxx/include/__format/formatter_floating_point.h` (https://github.com/llvm/llvm-project/blob/main/libcxx/include/__format/formatter_floating_point.h). First, a traits class lists the relevant numbers:
```c++
template <>
struct __traits {
static constexpr int __max_integral = 308;
static constexpr int __max_fractional = 1074;
static constexpr int __max_fractional_value = 4;
static constexpr size_t __stack_buffer_size = 1024;
static constexpr int __hex_precision_digits = 4;
};
```
Then, the constructor of the float_buffer helper class works as follows, in relevant parts:
```c++
// When using a scientific formatting with a precision of 6 a stack buffer
// will always suffice. At the moment that isn't important since floats and
// doubles use a stack buffer, unless the precision used in the format string
// is large.
// When supporting long doubles the __max_integral part becomes 4932 which
// may be too much for some platforms. For these cases a better estimate is
// required.
explicit _LIBCPP_HIDE_FROM_ABI __float_buffer(int __precision)
: __precision_(__precision != -1 ? __precision : _Traits::__max_fractional) {
// ...
__size_ = __formatter::__float_buffer_size<_Fp>(__precision_);
if (__size_ > _Traits::__stack_buffer_size)
// The allocated buffer's contents don't need initialization.
__begin_ = allocator{}.allocate(__size_);
else
__begin_ = __buffer_;
}
```
Without a precision spec, `__precision == -1` is passed into the constructor, which causes `__precision_` to have the value `__max_fractional` (1074). Then, the `__size_` is computed with:
```c++
template
_LIBCPP_HIDE_FROM_ABI constexpr size_t __float_buffer_size(int __precision) {
using _Traits = __traits<_Fp>;
return 4 + _Traits::__max_integral + __precision + _Traits::__max_fractional_value;
}
```
That results in 1390, which is greater than the `__stack_buffer_size` trait (1024). This mismatch appears to have been done by mistake, given the comments and review comments.
I'm happy to submit a PR, but I'd like to know what should be the proper fix. It seems to me like two choices, either (1) increase `__stack_buffer_size` to more than 1390, or (2) set the default precision to `__stack_buffer_size - 4 - __max_integral - __max_fractional_value`. Both seem fairly low impact and an easy fix.
Contributor guide
Research direction
Start in libcxx/include/__format/formatter_floating_point.h, reading the __traits values and __float_buffer constructor, then compare the intent in review D114001. Reproduce default-precision double formatting and inspect the buffer-size calculation. Done means the default path no longer allocates unexpectedly while preserving correct std::format output.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 67/100