From 0a52742008090787024003d6e523e8ac37a0bb10 Mon Sep 17 00:00:00 2001 From: Kenneth Weiss Date: Fri, 30 Dec 2022 16:37:11 -0800 Subject: [PATCH 1/8] Bugfix for fmt::printf on Power9 architecture with the XL compiler --- include/fmt/format.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/fmt/format.h b/include/fmt/format.h index 8f05c7d92cf9..e78409483434 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -3583,6 +3583,10 @@ template struct arg_formatter { const format_specs& specs; locale_ref locale; + arg_formatter(buffer_appender it, const format_specs& s, + locale_ref l) + : out{it}, specs{s}, locale{l} {} + template FMT_CONSTEXPR FMT_INLINE auto operator()(T value) -> iterator { return detail::write(out, value, specs, locale); From ed37406f956c564f3b5b8ecdf35a6b0b6803bda0 Mon Sep 17 00:00:00 2001 From: Kenneth Weiss Date: Tue, 3 Jan 2023 12:32:51 -0800 Subject: [PATCH 2/8] Adds arg_formatter contructor using default locale This avoids the need to pass a temporary (default) locale_ref. --- include/fmt/format.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index e78409483434..23f6c8a7f240 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -3583,9 +3583,12 @@ template struct arg_formatter { const format_specs& specs; locale_ref locale; + arg_formatter(buffer_appender it, const format_specs& s) + : out(it), specs(s) {} + arg_formatter(buffer_appender it, const format_specs& s, locale_ref l) - : out{it}, specs{s}, locale{l} {} + : out(it), specs(s), locale(l) {} template FMT_CONSTEXPR FMT_INLINE auto operator()(T value) -> iterator { From 6cfc4c5310c51f5db8453e94903eea56ea9354da Mon Sep 17 00:00:00 2001 From: Kenneth Weiss Date: Tue, 3 Jan 2023 12:34:00 -0800 Subject: [PATCH 3/8] Don't pass locale_ref into arg_formatter constructor in printf_arg_formatter --- include/fmt/printf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/fmt/printf.h b/include/fmt/printf.h index f091cc8039a8..8fbec12ad62c 100644 --- a/include/fmt/printf.h +++ b/include/fmt/printf.h @@ -235,7 +235,7 @@ class printf_arg_formatter : public arg_formatter { public: printf_arg_formatter(OutputIt iter, format_specs& s, context_type& ctx) - : base{iter, s, locale_ref()}, context_(ctx) {} + : base{iter, s}, context_(ctx) {} OutputIt operator()(monostate value) { return base::operator()(value); } From 10e187dff6c5295befb19c3afe38774076895519 Mon Sep 17 00:00:00 2001 From: Kenneth Weiss Date: Mon, 9 Jan 2023 11:59:04 -0800 Subject: [PATCH 4/8] Use a pointer instead of a reference for specs in arg_formatter Per PR suggestion. --- include/fmt/format.h | 10 +++++----- include/fmt/printf.h | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index 23f6c8a7f240..2b2837055d2b 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -3580,19 +3580,19 @@ template struct arg_formatter { using context = buffer_context; iterator out; - const format_specs& specs; + const format_specs* specs; locale_ref locale; - arg_formatter(buffer_appender it, const format_specs& s) + arg_formatter(buffer_appender it, const format_specs* s) : out(it), specs(s) {} - arg_formatter(buffer_appender it, const format_specs& s, + arg_formatter(buffer_appender it, const format_specs* s, locale_ref l) : out(it), specs(s), locale(l) {} template FMT_CONSTEXPR FMT_INLINE auto operator()(T value) -> iterator { - return detail::write(out, value, specs, locale); + return detail::write(out, value, *specs, locale); } auto operator()(typename basic_format_arg::handle) -> iterator { // User-defined types are handled separately because they require access @@ -4220,7 +4220,7 @@ void vformat_to(buffer& buf, basic_string_view fmt, specs.precision, specs.precision_ref, context); if (begin == end || *begin != '}') on_error("missing '}' in format string"); - auto f = arg_formatter{context.out(), specs, context.locale()}; + auto f = arg_formatter{context.out(), &specs, context.locale()}; context.advance_to(visit_format_arg(f, arg)); return begin; } diff --git a/include/fmt/printf.h b/include/fmt/printf.h index 8fbec12ad62c..409e0b1faeca 100644 --- a/include/fmt/printf.h +++ b/include/fmt/printf.h @@ -228,13 +228,13 @@ class printf_arg_formatter : public arg_formatter { context_type& context_; OutputIt write_null_pointer(bool is_string = false) { - auto s = this->specs; + auto s = *this->specs; s.type = presentation_type::none; return write_bytes(this->out, is_string ? "(null)" : "(nil)", s); } public: - printf_arg_formatter(OutputIt iter, format_specs& s, context_type& ctx) + printf_arg_formatter(OutputIt iter, format_specs* s, context_type& ctx) : base{iter, s}, context_(ctx) {} OutputIt operator()(monostate value) { return base::operator()(value); } @@ -244,7 +244,7 @@ class printf_arg_formatter : public arg_formatter { // MSVC2013 fails to compile separate overloads for bool and Char so use // std::is_same instead. if (std::is_same::value) { - format_specs fmt_specs = this->specs; + format_specs fmt_specs = *this->specs; if (fmt_specs.type != presentation_type::none && fmt_specs.type != presentation_type::chr) { return (*this)(static_cast(value)); @@ -269,13 +269,13 @@ class printf_arg_formatter : public arg_formatter { /** Formats a null-terminated C string. */ OutputIt operator()(const char* value) { if (value) return base::operator()(value); - return write_null_pointer(this->specs.type != presentation_type::pointer); + return write_null_pointer(this->specs->type != presentation_type::pointer); } /** Formats a null-terminated wide C string. */ OutputIt operator()(const wchar_t* value) { if (value) return base::operator()(value); - return write_null_pointer(this->specs.type != presentation_type::pointer); + return write_null_pointer(this->specs->type != presentation_type::pointer); } OutputIt operator()(basic_string_view value) { @@ -545,7 +545,7 @@ void vprintf(buffer& buf, basic_string_view format, // Format argument. out = visit_format_arg( - printf_arg_formatter(out, specs, context), arg); + printf_arg_formatter(out, &specs, context), arg); } write(out, basic_string_view(start, to_unsigned(it - start))); } From af9a54ac9345b26b39a3ff76d5b2b8134a0c6669 Mon Sep 17 00:00:00 2001 From: Kenneth Weiss Date: Mon, 9 Jan 2023 12:34:31 -0800 Subject: [PATCH 5/8] Remove constructors for arg_formatter struct --- include/fmt/format.h | 7 ------- 1 file changed, 7 deletions(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index 2b2837055d2b..0d42bd9aaefd 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -3583,13 +3583,6 @@ template struct arg_formatter { const format_specs* specs; locale_ref locale; - arg_formatter(buffer_appender it, const format_specs* s) - : out(it), specs(s) {} - - arg_formatter(buffer_appender it, const format_specs* s, - locale_ref l) - : out(it), specs(s), locale(l) {} - template FMT_CONSTEXPR FMT_INLINE auto operator()(T value) -> iterator { return detail::write(out, value, *specs, locale); From e4645d2867f01667d619885de59940233d834874 Mon Sep 17 00:00:00 2001 From: Kenneth Weiss Date: Mon, 9 Jan 2023 12:35:05 -0800 Subject: [PATCH 6/8] Apply XL compiler bugfix within printf_arg_formatter constructor --- include/fmt/printf.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/include/fmt/printf.h b/include/fmt/printf.h index 409e0b1faeca..8eecf0b44c5f 100644 --- a/include/fmt/printf.h +++ b/include/fmt/printf.h @@ -235,7 +235,13 @@ class printf_arg_formatter : public arg_formatter { public: printf_arg_formatter(OutputIt iter, format_specs* s, context_type& ctx) - : base{iter, s}, context_(ctx) {} + : base{iter, s, locale_ref()}, context_(ctx) { +#if defined(__ibmxl__) + // Bugfix: XL compiler optimizes out initializer for base + this->out = iter; + this->specs = s; +#endif + } OutputIt operator()(monostate value) { return base::operator()(value); } From 4097a242a9c1c3a2418528b9cf1596d127513be6 Mon Sep 17 00:00:00 2001 From: Kenneth Weiss Date: Tue, 10 Jan 2023 18:17:19 -0800 Subject: [PATCH 7/8] Adds a factory function for `arg_formatter` w/ default `locale_ref` Use this within printf_arg_formatter constructor as workaround for XL compiler bug that optimizes away base class initializer. Also: Reverts conversion of internal `specs` variable back to a const ref Per PR suggestion. --- include/fmt/format.h | 11 ++++++++--- include/fmt/printf.h | 20 +++++++------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index 0d42bd9aaefd..70764bff4e77 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -3580,18 +3580,23 @@ template struct arg_formatter { using context = buffer_context; iterator out; - const format_specs* specs; + const format_specs& specs; locale_ref locale; template FMT_CONSTEXPR FMT_INLINE auto operator()(T value) -> iterator { - return detail::write(out, value, *specs, locale); + return detail::write(out, value, specs, locale); } auto operator()(typename basic_format_arg::handle) -> iterator { // User-defined types are handled separately because they require access // to the parse context. return out; } + + static auto make_arg_formatter(iterator iter, format_specs& s) + -> arg_formatter { + return {iter, s, locale_ref()}; + } }; template struct custom_formatter { @@ -4213,7 +4218,7 @@ void vformat_to(buffer& buf, basic_string_view fmt, specs.precision, specs.precision_ref, context); if (begin == end || *begin != '}') on_error("missing '}' in format string"); - auto f = arg_formatter{context.out(), &specs, context.locale()}; + auto f = arg_formatter{context.out(), specs, context.locale()}; context.advance_to(visit_format_arg(f, arg)); return begin; } diff --git a/include/fmt/printf.h b/include/fmt/printf.h index 8eecf0b44c5f..40ebede416d7 100644 --- a/include/fmt/printf.h +++ b/include/fmt/printf.h @@ -228,20 +228,14 @@ class printf_arg_formatter : public arg_formatter { context_type& context_; OutputIt write_null_pointer(bool is_string = false) { - auto s = *this->specs; + auto s = this->specs; s.type = presentation_type::none; return write_bytes(this->out, is_string ? "(null)" : "(nil)", s); } public: - printf_arg_formatter(OutputIt iter, format_specs* s, context_type& ctx) - : base{iter, s, locale_ref()}, context_(ctx) { -#if defined(__ibmxl__) - // Bugfix: XL compiler optimizes out initializer for base - this->out = iter; - this->specs = s; -#endif - } + printf_arg_formatter(OutputIt iter, format_specs& s, context_type& ctx) + : base(base::make_arg_formatter(iter, s)), context_(ctx) {} OutputIt operator()(monostate value) { return base::operator()(value); } @@ -250,7 +244,7 @@ class printf_arg_formatter : public arg_formatter { // MSVC2013 fails to compile separate overloads for bool and Char so use // std::is_same instead. if (std::is_same::value) { - format_specs fmt_specs = *this->specs; + format_specs fmt_specs = this->specs; if (fmt_specs.type != presentation_type::none && fmt_specs.type != presentation_type::chr) { return (*this)(static_cast(value)); @@ -275,13 +269,13 @@ class printf_arg_formatter : public arg_formatter { /** Formats a null-terminated C string. */ OutputIt operator()(const char* value) { if (value) return base::operator()(value); - return write_null_pointer(this->specs->type != presentation_type::pointer); + return write_null_pointer(this->specs.type != presentation_type::pointer); } /** Formats a null-terminated wide C string. */ OutputIt operator()(const wchar_t* value) { if (value) return base::operator()(value); - return write_null_pointer(this->specs->type != presentation_type::pointer); + return write_null_pointer(this->specs.type != presentation_type::pointer); } OutputIt operator()(basic_string_view value) { @@ -551,7 +545,7 @@ void vprintf(buffer& buf, basic_string_view format, // Format argument. out = visit_format_arg( - printf_arg_formatter(out, &specs, context), arg); + printf_arg_formatter(out, specs, context), arg); } write(out, basic_string_view(start, to_unsigned(it - start))); } From ad7f0c22863938d382e17659561c7a55a058c6bd Mon Sep 17 00:00:00 2001 From: Kenneth Weiss Date: Thu, 12 Jan 2023 19:11:30 -0800 Subject: [PATCH 8/8] Moved make_arg_formatter to printf.h Per PR suggestion. --- include/fmt/format.h | 5 ----- include/fmt/printf.h | 10 +++++++++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/include/fmt/format.h b/include/fmt/format.h index 70764bff4e77..8f05c7d92cf9 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -3592,11 +3592,6 @@ template struct arg_formatter { // to the parse context. return out; } - - static auto make_arg_formatter(iterator iter, format_specs& s) - -> arg_formatter { - return {iter, s, locale_ref()}; - } }; template struct custom_formatter { diff --git a/include/fmt/printf.h b/include/fmt/printf.h index 40ebede416d7..cae051d7b711 100644 --- a/include/fmt/printf.h +++ b/include/fmt/printf.h @@ -218,6 +218,14 @@ template class printf_width_handler { } }; +// Workaround for a bug with the XL compiler when initializing +// printf_arg_formatter's base class. +template +auto make_arg_formatter(buffer_appender iter, format_specs& s) + -> arg_formatter { + return {iter, s, locale_ref()}; +} + // The ``printf`` argument formatter. template class printf_arg_formatter : public arg_formatter { @@ -235,7 +243,7 @@ class printf_arg_formatter : public arg_formatter { public: printf_arg_formatter(OutputIt iter, format_specs& s, context_type& ctx) - : base(base::make_arg_formatter(iter, s)), context_(ctx) {} + : base(make_arg_formatter(iter, s)), context_(ctx) {} OutputIt operator()(monostate value) { return base::operator()(value); }