NOTE: Please always add static_assert on datatype before using FmtCore::format

[kirk0830]

Describe the Code Quality Issue

After a very long time debugging, @jinzx10, @caic99 and I find the bug reported by issue #4540.
See the following code piece:

abacus-develop/source/module_cell/klist.cpp

Line 776 in db90f92

std::vector<double> ibz2bz(this->nkstot);

what will happen if:

abacus-develop/source/module_cell/klist.cpp

Lines 944 to 953 in db90f92

	for (int ik = 0; ik < nkstot_ibz; ik++)
	{
	table += FmtCore::format("%8d%12.8f%12.8f%12.8f%8.4f%8d\n",
	ik + 1,
	kvec_d_ibz[ik].x,
	kvec_d_ibz[ik].y,
	kvec_d_ibz[ik].z,
	wk_ibz[ik],
	ibz2bz[ik]);
	}

?
It will trigger an undefined behavior because the double is parsed like int by formatter (the ABACUS in-built library for formatting strings, implemented in module_base/formatter.h), that cases recent failure in integrated test.

The implementation of function FmtCore::format is quite simple:

abacus-develop/source/module_base/formatter.h

Lines 40 to 48 in db90f92

	template<typename... Ts>
	static inline std::string format(const char* fmt, const Ts&... args)
	{
	const int size = snprintf(nullptr, 0, fmt, FmtCore::filter(args)...) + 1;
	std::string dst(size, ' ');
	const int size_filled = snprintf(&dst[0], size, fmt, FmtCore::filter(args)...);
	dst.resize(size_filled);
	return dst;
	}

But the c-style function snprintf itself cannot identify the datatype. So I would suggest to use something like static_assert before using this function.

Additional Context

No response

Task list for Issue attackers (only for developers)

• Identify the specific code file or section with the code quality issue.
• Investigate the issue and determine the root cause.
• Research best practices and potential solutions for the identified issue.
• Refactor the code to improve code quality, following the suggested solution.
• Ensure the refactored code adheres to the project's coding standards.
• Test the refactored code to ensure it functions as expected.
• Update any relevant documentation, if necessary.
• Submit a pull request with the refactored code and a description of the changes made.

[kirk0830]

@WHUweiqingzhou FYI

[jinzx10]

Most compilers provide a "-Wformat" option for basic type checks for printf/sprintf/..., which would emit a warning message when there is a type mismatch. For example, compiling the following code

#include <cstdio>

int main() {
    double d = 3;
    std::snprintf(nullptr, 0, "%4i", d);
    return 0;
}

will get something like

hw.cpp: In function ‘int main()’:
hw.cpp:41:34: warning: format ‘%i’ expects argument of type ‘int’, but argument 4 has type ‘double’ [-Wformat=]
   41 |     std::snprintf(nullptr, 0, "%4i", d);
      |                                ~~^   ~
      |                                  |   |
      |                                  int double
      |                                %4f

I'm not sure if "-Wformat" is enabled by default [at least it is so on my everyday compiler gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04)], but even it is enabled, the real question is, "-Wformat" is merely a compile-time check that is unable to be carried out for argument types agains a format string passed via function argument, as it is in FmtCore. That is to say, "-Wformat" would fail to detect bugs like

#include <cstdio>

void foobar(const char* fmt, double i) {
    std::snprintf(nullptr, 0, fmt, i);
}

int main() {
    foobar("%4i", 3.0);
    return 0;
}

It might, however, be possible to let snprintf knows the format string during compilation, as long as the format string is a const expression (e.g., making "foobar" a function template and fmt a template parameter). Unfortunately, using string literals as template parameters is a c++20 feature; achieving this within c++11, if possible, might need some metaprogramming.

[caic99]

@jinzx10 IMHO using macro definition would be a good replacement

#include <cstdio>

#define foobar_safe(fmt, i) std::snprintf(nullptr, 0, fmt, i)

int main() {
    foobar_safe("%4i", 3.0);
}

This grammar generates warning correctly.

[kirk0830]

@caic99
I will use the following version:

#include <cstdio>

#define __FMTCORE_DATA_CHECK__(fmt, ...) std::snprintf(nullptr, 0, fmt, __VA_ARGS__)

int main() {
    const int right = 3;
    const double wrong = 3.0;

    __FMTCORE_DATA_CHECK__("%4d%4d", right, wrong);
}

[mohanchen]

I think in the beginning of the issue, there should be a link to explain what is 'formatter' @kirk0830

[kirk0830]

I think in the beginning of the issue, there should be a link to explain what is 'formatter' @kirk0830

Thanks, I have added a short explanation about formatter in issue descrption

[WHUweiqingzhou]

I move this issue to discussion Note for Developer.