llvm / llvm/llvm-project

[libc++][bug] double-precision formatter allocates on heap with default precision

Open
#211,909 3 comments 0 reactions 0 assignees View on GitHub
format libc++ performance
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.