-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MDEV-21978 part 2] Enable and Fix GCC -Wformat
Checks on my_vsnprintf
and Users
#3360
base: main
Are you sure you want to change the base?
Conversation
Commits for MariaDB#3360 (MDEV-21978) changes ABIs with their introduction of `__attribute((format(printf, …))` tags. For the CI’s convenience, I have changed `do_abi_check.cmake` to warn rather than to fatally error. I will revert this commit at the end of this PR.
65dceed
to
a488e87
Compare
To confirm, are our Update: Actually, let me see if it can be Update: Nope. Too many |
Commits for MariaDB#3360 (MDEV-21978) changes ABIs with their introduction of `__attribute((format(printf, …))` tags. For the CI’s convenience, I have changed `do_abi_check.cmake` to warn rather than to fatally error. I will revert this commit at the end of this PR.
a488e87
to
4c51a35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recent force-pushes extracted some work for latter commits (i.e., commit split & reörder).
The previous force-push also amends mistakes:
- Use
NullS
overnullptr
- Update assertion count in unit test
Commits for MariaDB#3360 (MDEV-21978) changes ABIs with their introduction of `__attribute((format(printf, …))` tags. For the CI’s convenience, I have changed `do_abi_check.cmake` to warn rather than to fatally error. I will revert this commit at the end of this PR.
2dff369
to
dc990a8
Compare
Commits for MariaDB#3360 (MDEV-21978) changes ABIs with their introduction of `__attribute((format(printf, …))` tags. For the CI’s convenience, I have changed `do_abi_check.cmake` to warn rather than to fatally error. I will revert this commit at the end of this PR.
dc990a8
to
f1425a2
Compare
Commits for MariaDB#3360 (MDEV-21978) changes ABIs with their introduction of `__attribute((format(printf, …))` tags. For the CI’s convenience, I have changed `do_abi_check.cmake` to warn rather than to fatally error. I will revert this commit at the end of this PR.
1207c8d
to
a97e4e5
Compare
Commits for MariaDB#3360 (MDEV-21978) changes ABIs with their introduction of `__attribute((format(printf, …))` tags. For the CI’s convenience, I have changed `do_abi_check.cmake` to warn rather than to fatally error. I will revert this commit at the end of this PR.
a97e4e5
to
dcd8abc
Compare
Commits for MariaDB#3360 (MDEV-21978) changes ABIs with their introduction of `__attribute((format(printf, …))` tags. For the CI’s convenience, I have changed `do_abi_check.cmake` to warn rather than to fatally error. I will revert this commit at the end of this PR.
dcd8abc
to
43c7984
Compare
There is no need to parse a format string if we just need to clone chars over. In fact, when MariaDB#3360 enabled `ATTRIBUTE_FORMAT` on `my_snprintf`, `-Werror=format-security` complained several of these that “format not a string literal and no format arguments”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changeset is getting wide – LMK if you prefer certain commits squashed together.
Regarding client/mysqltest.cc
,
Line 25 in 25b5c63
Please keep the test framework tools identical in all versions! |
What does it mean by this line?
Line 36 in 25b5c63
#define VER "3.5" |
Do I need to increase this version tag… if yes, how?
@@ -2280,7 +2280,7 @@ int spider_db_mbase::fetch_and_print_warnings(struct tm *l_time) | |||
longlong res_num = | |||
(longlong) my_strtoll10(row[1], (char**) NULL, &error_num); | |||
DBUG_PRINT("info",("spider res_num=%lld", res_num)); | |||
my_printf_error((int) res_num, row[2], MYF(0)); | |||
my_printf_error((int) res_num, "%s", MYF(0), row[2]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentor @LinuxJedi says that, while it’s a security issue, the impact should be (relatively) insignificant.
We must address it nonetheless. I’ll probably extract this to a separate PR that follows the bug fix process (base on the oldest version in support).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to increase the version tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll probably extract this to a separate PR that follows the bug fix process (base on the oldest version in support).
[list moved to OP]
@@ -294,7 +294,7 @@ int ha_oqgraph::oqgraph_check_table_structure (TABLE *table_arg) | |||
{ | |||
DBUG_PRINT( "oq-debug", ("Allowing integer no more!")); | |||
badColumn = true; | |||
push_warning_printf( current_thd, Sql_condition::WARN_LEVEL_WARN, HA_WRONG_CREATE_OPTION, "Integer latch is not supported for new tables.", i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git blame
4e66318
my_printf_error(ER_UNKNOWN_ERROR, "XPATH syntax error: '%.*s'", | ||
MYF(0), clen, xpath.lasttok.beg); | ||
else | ||
my_printf_error(ER_UNKNOWN_ERROR, "XPATH syntax error: '%.32T'", | ||
my_printf_error(ER_UNKNOWN_ERROR, "XPATH syntax error: '%.32sT'", | ||
MYF(0), xpath.lasttok.beg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(applies to the former two %sT
changes as well)
%sT
won't add the ...
replacement if the length is <=
the specified width.
But eh, what do I know? Maybe the clen
and others have a purpose.
vsnprintf(buf, MAX_BUF_SIZE - 1, format, args); | ||
|
||
push_warning_printf( | ||
push_warning( | ||
thd, Sql_condition::WARN_LEVEL_WARN, | ||
uint(convert_error_code_to_mysql(error, 0, thd)), buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also applies to the copy below)
It’s understandable for this file to vsnprintf
before push_warning
because push_warning_printf
doesn’t have a vprintf
variant.
But it’s interesting that it preferred using C++ vsnprintf
over beïng consistent with push_warning_printf
and using my_vsnprintf
.
20711ef
to
8f4ee14
Compare
@@ -3106,7 +3106,7 @@ SQL_SELECT::test_quick_select(THD *thd, | |||
group_by_optimization_used= 1; | |||
param.table->set_opt_range_condition_rows(group_trp->records); | |||
DBUG_PRINT("info", ("table_rows: %llu opt_range_condition_rows: %llu " | |||
"group_trp->records: %ull", | |||
"group_trp->records: %llu", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sql/table.cc
Outdated
i, field->null_bit); | ||
my_snprintf(buff, sizeof(buff), | ||
"\ntable->field[%u]->field_name %s\n" | ||
"table->field[%u]->offset = %" PRIuPTR "\n" // `%tu` not available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t
is the size modifier for ptrdiff_t
, similar to z
is for size_t
.
https://en.cppreference.com/w/c/io/fprintf#Parameters
@@ -2643,7 +2643,7 @@ dberr_t fseg_free_page(fseg_header_t *seg_header, fil_space_t *space, | |||
mtr->x_lock_space(space); | |||
|
|||
DBUG_PRINT("fseg_free_page", | |||
("space_id: " ULINTPF ", page_no: %u", space->id, offset)); | |||
("space_id: %" PRIu32 ", page_no: %" PRIu32, space->id, offset)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found ULINTPF
here:
server/storage/innobase/include/univ.i
Lines 290 to 321 in 25b5c63
/** ulint format for the printf() family of functions */ | |
#define ULINTPF "%zu" | |
/** ulint hexadecimal format for the printf() family of functions */ | |
#define ULINTPFx "%zx" | |
#ifdef _WIN32 | |
/* Use the integer types and formatting strings defined in Visual Studio. */ | |
# define UINT32PF "%u" | |
# define UINT64scan "llu" | |
# define UINT64PFx "%016llx" | |
#elif defined __APPLE__ | |
/* Apple prefers to call the 64-bit types 'long long' | |
in both 32-bit and 64-bit environments. */ | |
# define UINT32PF "%" PRIu32 | |
# define UINT64scan "llu" | |
# define UINT64PFx "%016llx" | |
#elif defined _AIX | |
/* Workaround for macros expension trouble */ | |
# define UINT32PF "%u" | |
# define UINT64scan "lu" | |
# define UINT64PFx "%016lx" | |
#else | |
/* Use the integer types and formatting strings defined in the C99 standard. */ | |
# define UINT32PF "%" PRIu32 | |
# define INT64PF "%" PRId64 | |
# define UINT64scan PRIu64 | |
# define UINT64PFx "%016" PRIx64 | |
#endif | |
typedef int64_t ib_int64_t; | |
typedef uint64_t ib_uint64_t; | |
typedef uint32_t ib_uint32_t; |
The file even has platform-detecting macros for
UINT32PF
. Is there a reason not to use PRIu32
from C/C++ (as with my_snprintf
)? If not, I prefer utilizing the standard.(also applies to the
PRIu64
addition on storage/innobase/handler/ha_innodb.cc
)See also Zulip discusson on C/C++
stdint.h
vs. MariaDB my_global.h
It’s also intriguing that innobase
has this misleading #define ULINTPF "%zu"
macro at home – %zu
takes an unsigned size_t
, which is not necessarily an unsigned long
.
Regardless, looks like space->id
is an uint32_t
(not even an ib_uint32_t
from above).
server/storage/innobase/include/fil0fil.h
Line 328 in 25b5c63
uint32_t id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now %zu
has backfired in storage/innobase/dict/dict0load.cc
.
8f4ee14
to
4544a03
Compare
#ifdef WITH_WSREP | ||
#include <inttypes.h> | ||
#ifdef WITH_WSREP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(At least) 3 of the Buildbots (here’s one of them) doesn’t understand PRIuPTR
(now replaced with PRiPTR
) for #3360 (comment).
Let me know if I should #include <inttypes.h>
somewhere else.
There is no need to parse a format string if we just need to clone chars over. In fact, when MariaDB#3360 enabled `ATTRIBUTE_FORMAT` on `my_snprintf`, `-Werror=format-security` complained several of these that “format not a string literal and no format arguments”.
Commits for MariaDB#3360 (MDEV-21978) changes ABIs with their introduction of `__attribute((format(printf, …))` tags. For the CI’s convenience, I have changed `do_abi_check.cmake` to warn rather than to fatally error. I will revert this commit at the end of this PR.
4544a03
to
dd6d9ea
Compare
@@ -1201,6 +1201,7 @@ class Log_event_handler | |||
const char *user_host, size_t user_host_len, ulonglong query_utime, | |||
ulonglong lock_utime, bool is_command, | |||
const char *sql_text, size_t sql_text_len)= 0; | |||
ATTRIBUTE_FORMAT(printf, 3, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn’t a guideline on where to put attributes.
I checked existing entries: most put attributes after the signature, a few put them before.
@@ -191,7 +191,7 @@ bool btr_root_fseg_validate(ulint offset, const buf_block_t &block, | |||
sql_print_error("InnoDB: Index root page " UINT32PF " in %s is corrupted " | |||
"at " ULINTPF, | |||
block.page.id().page_no(), | |||
UT_LIST_GET_FIRST(space.chain)->name); | |||
UT_LIST_GET_FIRST(space.chain)->name, offset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guessed this one as well and could use a contact.
@@ -4439,7 +4439,7 @@ static dberr_t recv_rename_files() | |||
err= space->rename(new_name, false); | |||
if (err != DB_SUCCESS) | |||
sql_print_error("InnoDB: Cannot replay rename of tablespace " | |||
UINT32PF " to '%s: %s", new_name, ut_strerr(err)); | |||
UINT32PF " to '%s': %s", id, new_name, ut_strerr(err)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git blame
28b27b9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The '
is a cosmetic fix.
@@ -2457,7 +2457,7 @@ static void check_and_remove_stale_alter(Relay_log_info *rli) | |||
{ | |||
DBUG_ASSERT(info->state == start_alter_state::REGISTERED); | |||
|
|||
sql_print_warning("ALTER query started at %u-%u-%llu could not " | |||
sql_print_warning("ALTER query started at %llu-%u-%u could not " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use PRIu64
over %llu
for the uint64
here, but somehow it doesn’t work, at least on my (WSL) Ubuntu.
This commit backports two fixes from MariaDB#3360’s vast discovery.
This commit backports two fixes from #3360’s vast discovery.
This commit prepares for MDEV-21978 which wants to migrate extensions from *specifiers* to standard-compatible *suffixes*, complete with boolean local vars. In this new format, branches will land on `%s`/`%u` before processing the suffix. This commit contains non-breaking portions of MDEV-21978 work. This commit also changes an `if`-`else` chain to a `switch` block simply because it’s an eyesore.
This is the first part of MDEV-21978 make my_vsnprintf to use gcc-compatible format extensions, which adds these alternatives to the MySQL extensions. There’s also the escapes `%sS` & `%uU` for any hippies needing them. These suffixes are compatible with the C standard and therefore as well as `printf` tools such as GCC checks. The old extension formats (e.g., `%M`) are now effectively deprecated, although they’re left intact for now. For a more sequential MDEV-21978 process, a separate commit will delete them after we migrate all `my_vsnprintf` usages to the new preferred syntax. The service’s major version bumped nonetheless for the new significance of suffixes. [Breaking] This commit may fail * on places needing the aforementioned escapes * because of the major version bump
Per MariaDB#3309 (comment), it is crucial that services’ headers have reliable and legible docs.
readability improvement (and tiny manual optimisation) MariaDB#3309 (comment) We could also extract `arg_idx` or even the entire `print_arr`; but eh, my job here is not to refactor the whole thing. (And TBH, we should refactor *more than* the whole thing.)
… and delete `%uU` (just use `%d` for that) The follow-up MariaDB#3360 discovered `%M` usages that suggest that it was designed for `errno` and similar **signed** `int` (“errno_t”) variables. Besides convenience, if the old `%M` read a `signed int`, so should the new version to maintain compatibility. I only added `%iE` (no `%dE`) to keep the new suffix mechanics away from the popular `%d`. Beïng synonyms (originally), this decision on preserving `%d` also saves the need for `%iI`/`%dD`.
PR MariaDB#3309 (MDEV-21978) introduced more preferrable alternatives to those extensions. This commit removes test assertions for the above old and deprecated syntax ahead of their removal, so GCC `-Wformat` testing won’t complain about those while their actual applications migrate. I will remove their implementation code in a much later commit. This way, nothing will break (further) as I utilize GCC `-Wformat` to port things from the old format to the new.
Commits for MariaDB#3360 (MDEV-21978) changes ABIs with their introduction of `__attribute__((format(printf, …))` tags. For building convenience, I have changed `do_abi_check.cmake` to warn rather than fatally error. I will revert this commit near the end of this PR.
[Breaking] Good news: GCC now checks your `my_snprintf` (direct) calls (`-Wformat` was on); Bad news: The build process no longer lets your incorrect formats/arguments sneak past (`-Werror` was also on). As such, this commit also fixes all new `my_snprintf` `-Wformat` errors. This commit does not update the ABI records – half a dozen `include/mysql/plugin_*.h.pp`s are now missing the new `__attribute__`s.
Use `my_attribute.h` in `service_my_snprintf.h` and `unittest/mytap/tap.h` for compatibility with non-GCC
also migrate string copyings away from unnecessarily using `my_snprintf`
Let GCC `-Wformat` check the formats sent to these `my_vsnprintf` users and migrate them from the old `%b` to the new `%sB`
[Breaking] Let GCC `-Wformat` check them just like `my_vsnprintf` itself
[Breaking] The `my_print_error` service passes formats and args directly to `my_vsnprintf`. Just like the `my_snprintf` service, I increased this service’s major version because: * Custom suffixes are now a thing (and custom specifiers will soon no longer be). * GCC `-Wformat` now checks formats sent to them.
Let GCC `-Wformat` check formats sent to these `my_vsnprintf_ex` users * Migrate them from the old extension specifiers to the new `-Wformat`-compatible suffixes * Fix mistakes caught by `-Wformat` * For “format not a string literal and no format arguments” warnings, switch them to use the non-`printf` equivalent `push_warning` * “no format arguments” in general should use `push_warning` instead, but it’s outside of `-Wformat`’s concerns.
(Re)ënable the `ATTRIBUTE_FORMAT` on `my_dbug.h`’s `_db_doprnt_` (better known by its frontend `DBUG_PRINT`) and `ma_recovery_util.h`’s `tprint` & `eprint` to leverage GCC `-Wformat` checking c4bf4b7 introduced `WAITING_FOR_BUGFIX_TO_VSPRINTF` to conditionally (read: temporarily) disable `ATTRIBUTE_FORMAT`. Whatever that bug was aside, MDEV-21978 Zulip suggested that the preference for `%b` was probably intended, although c52e62a reverted the one in `storage/maria/ma_recovery.c` back to `%s`. All mistake fixes and extension migrations (e.g., `%b` ➡ `%sB`) included in this commit were on `DBUG_PRINT` – GCC didn’t catch any `t`/`eprint` problems. Most of these mistakes were passing `size_t` arguments without using the C/C++-standard `%zu` construct.
Let GCC `-Wformat` check formats sent to these users of `my_vsnprintf_ex` users (heh) Fix lots of mistakes caught by `-Wformat` and migrate others from the old extension specifiers to the new `-Wformat`-compatible suffixes
[Breaking] The `logger` service passes formats and args directly to `my_vsnprintf`. Just like the `my_snprintf` service, I increased this service’s major version because: * Custom suffixes are now a thing (and custom specifiers will soon no longer be). * GCC `-Wformat` now checks formats sent to them.
Let GCC `-Wformat` check the formats sent to these `my_vsnprintf` users
The function pointer typedef `my_error_reporter` is already tagged. This commit inherits this attribute to all `my_getopt_error_reporter`s and `my_charset_error_reporter`s for consistency. (It future-proofs for deliberate direct uses of those functions.)
This commit is the final batch of MariaDB#3360’s `ATTRIBUTE_FORMAT` process, covering various insignificant (as in, requires few-to-no changes in addition to gaining this attribute) functions. One of the main focus of this PR is to enable GCC `-Wformat` (by tagging `ATTRIBUTE_FORMAT`) on ALL `my_snprintf` utilities. To be throughout, functions that delegate to `my_vsnprintf` must also inherit this attribute because `-Wformat` doesn’t trace argument across call stacks.
* Run `make abi_update` * Revert the commit “Allow ABI mismatches temporarily” * Remove trailing spaces in `cmake/do_abi_check.cmake`
* Migrate `sql/share/errmsg-utf8.txt` to use suffix-based, `-Wformat` -compatible `my_snprintf` format extensions introduced in MDEV-21978 * Update relevant tests caught by BuildBot as well While GCC `-Wformat` (with `ATTRIBUTE_FORMAT`) can catch obsolete or malformed format string literals, formats originating from other sources (such as this translations file) (still) require manual review. This commit also escapes the only (1) instance of existing strings conflicted by the introduction of suffixes: (Not all `printf`s goes to `my_snprintf`, thus I `grep`ped and confirmed that this does indeed land on `my_snprintf` eventually.) chi "不能%sSLAVE'%.*s'" This commit also fixes the following: (You’re welcome.) * Delete extraneous spaces after the `%` (they’re all Swahili) * Update `extra/comp_err.c` * Add the missing standard C/C++ specifiers `c`, `i`, `o`, `p` and `X` (Especially `%i`: it otherwise was complaining about the new `%iE`) * Removed the old and obsolete extension formats `%b`, `%M` and `%T`
Migrate `mysys/errors.c`, `sql-common/errmsg.c` and a couple of insignificant loose ends to use suffix-based, `-Wformat`-compatible `my_snprintf` format extensions introduced in MDEV-21978 This commit is the final batch of MDEV-21978’s migration process. While GCC `-Wformat` (with `ATTRIBUTE_FORMAT`) can catch obsolete or malformed format string literals, formats originating from other sources (such as those strings headers) (still) require manual review. Thus, after all the automatic `-Wformat` complaints fixed in previous commits, I’ve done a manual `grep` and caught these final matches.
PR MariaDB#3309 (MDEV-21978) introduced more preferrable alternatives to those extensions. This commit removes the above old and deprecated syntax. An earlier commit has already removed tests for these specifiers. With the code for the old extension formats gone, this commit also improves the code around the new extension suffixes: * Remove code for formatting ``%`T`` * Those code are now dead, for the new suffix-based syntax does not recognize `%sQT`/`%sTQ`. * `suffix_q= TRUE` now additionally replaces `…|= ESCAPED_ARG`. * Flatten ``flag = true if /%iE/ `` and `do_iE if flag` code together [Breaking] This commit removes obsolete features. Although earlier commits (should have) migrated every usages direct or indirect in the entire MariaDB/server, other codebases might still be using them. This final change will break *everything* in those outdated foreign lands.
dd06777
to
fbe8c57
Compare
I will keep this branch updated with that prerequisite branch.
When that prerequisite merges, I will rebase this PR to the latest branch directly.
Why wasn’t this project merged during GSoC?
(Yes, I’m linking this PR as my GSoC final submission.)
Sporadic (inconsistent) CI failures on the base branch have been consistently plaguing the MariaDB/server repo before and during this GSoC event. How do we like them 🍎s?
These issues confuse both new (such as me and other GSoC participants) and old contributors, leading to wild geese chases and even alert fatigue.1
MariaDB organizers are aiming to improve coördination and address those bugs promptly.
Unfortunately, this shift in focus puts new features such as most of the GSoC projects in the back seat.
For more information, folks are tracking those failures at
MDEV-33073 always green buildbot and #3425, and are discussing specific issues on the “Test failures” Zulip thread.
After GSoC, I will probably continue updating this PR and the prerequisite “part 1” PR, and also publish more related patches.
For the record, this GSoC only owns the content in these two PRs that were created before 2024-08-26T18:00Z (GSoC final submission deadline), including PR descriptions, comments, and commits up to and including “Remove
%`s %b %M %T
”.Description
This is the second part of MDEV-21978. It may be the lesser part, but the last 10% takes 90% of the time, they say.
The majority of this PR tags the
my_snprintf
service functions and all2 functions that use those functions (see § Release Notes for notable ones) with__attribute__((format(printf, …)))
(viaATTRIBUTE_FORMAT
). I committed them as multiple sets – LMK if you prefer my commits squashed.This attribute effectively enables GCC
-Wformat
checks on them.It therefore also pushes for (a lot of) format-related fixes to insignificant security vulnerabilities and possible vulnerabilities:
(My GSoC mentor told me in Direct Message that they aren’t much of a problem and I can open PR(s) normally.)
For some post-GSoC ketchup, I am currently also extracting (read: backporting) these corrections in subsets to older maintained version according to the bug fix process.
DBUG_PRINT
format ofgroup_trp->records
#3537my_snprintf
arg mismatches #3541LEX_STRING::str
s formy_snprintf
#3538"%s"
fix. Forpush_warning_printf
, I switched them to the non-printf
equivalentpush_warning
.%s
(imlicitly casting tochar*
)size_t
, fixed-size and platform-dependant integer types and size specifiers-Wformat
cannot catch these if their sizes happen to line up for the platform. See § non-goals under § More descriptionsThe attribute also (conveniently) complained about all format string literals that used the pre-#3309, specifier-based and therefore
-Wformat
-incompatible extension syntaxes (%`s
,%b
,%M
&%T
).To be thorough, I’ve also done a manual
grep
at the end to identify the loose ends, most of which are error message( template)s fromerrmsg-utf8.txt
,mysys/errors.c
andsql-common/errmsg.c
.I have updated them all to #3309 (the prerequisite)’s suffix-based,
-Wformat
-compatible extension syntaxes (%sQ
,%sB
,%iE
&%sT
respectively). I’ve updated all impacted tests as well, namely those involving these error message templates.With all usages upgraded, I’ve finally deleted the old extension syntaxes, marking the completion of the MDEV-21978 transition.
I did not increase
service_my_snprintf
’s version, as I’ve already done so in the part 1 PR #3309, and I intended these two parts to be merged in the same MariaDB version.I did, though, bump the major versions of other
__attribute__
d services (logger
andmy_print_error
) to remind that they too now respect-Wformat
and switched up the extension syntax.What problem is the patch trying to solve?
Development tools – namely GCC’s
-Wformat
– can checkprintf
formats and arguments and catch mistakes.Howëver,
my_vsnprintf
and co., our in-house platform-agnostic replacement for theprintf
suite, are incompatible with these tools because of their custom extensions.Before the prerequisite PR, our format extensions relied on specifiers that’re undefined in the C/C++ Standard and therefore were guaranteed false positives to these error detectors.
This problem forced our
my_sprintf
service to stay away from these helpful features, which contributes to human errors remaining elusive and piling up like technical debt.After I created those compatible alternatives in the prerequisite, I enabled
-Wformat
onmy_snprintf
s and descendants, originally intended to get my GCC builds to automatically locate (most of) those false-positive old constructs for migration (hence beïng the follow-up).Instead, those forgotten mistakes relentlessly haunted me like a freshly-excavated skeleton-in-our-closet.
Since the MariaDB recently shifted focus on squashing long-time bugs, I too decided to not abandon commit quality.
I willingly stretched my GSoC project’s scope for this cause by including those dozens of corrections in this Part 2 PR.
(As stated in § why GSoC wasn’t merged, we can’t merge GSoC project(s) in time anyhow because of priorities.)
In summary, this
beginner-friendly
project became much more impactful than I imagined.We not only fixed countless entries of elusive mistakes but are also future-proofing them from reöccuring.
More descriptions
If some output changed that is not visible in a test case, what was it looking like before the change and how it's looking with this patch applied?
Good question. Before fixing those mismatching argument sizes and counts, who knows what out-of-bounds stuff they’ve been reading?
There’s also this super insignificant cosmetic fix.
Do you think this patch might introduce side-effects in other parts of the server?
Technically yes, but hopefully not.
The old syntaxes have been obsolete and practically deprecated since Part 1, but were once features nonetheless. Although earlier commits should’ve migrated every usage (direct or indirect) in MariaDB/server, it is still theoretically possible that some usages are so stealthy they evaded my
grep
.non-goals
-Wformat
doesn’t complainunsigned int
to a%d
PRId32
)sql/table.cc
because the main process already heavily modified the file.snprintf
alternatives withATTRIBUTE_FORMAT
and fix calls to them or the C/C++snprintf
itselfmy_vsnprintf_utf32
my_b_printf
my_vsnprintf
(myvsnprintf
) and those that use it.my_snprintf
to Usestrmake
overmy_snprintf
to copy strings #3429.They will likely conflict with the old patch here; in which case, that PR’s
strmakes
take precedence.CODING_STANDARDS.md
Release Notes
checks part 1 notes
amend part 1’s notes along the lines of:
my_snprintf
extensions%`s
,%b
,%M
and%T
are removed. They are now%sQ
,%sB
,%iE
and%sT
respectively, which are fully compatible withprintf
checks such as GCC-Wformat
.my_snprintf
and all functions that utilize it, such as:__attribute__((format(printf, …)))
to enable said GCC-Wformat
checks, and with that came countless fixes to garbled displays (if not outright crashing) of error messages and debug logs on certain platforms that were stealthed for years!I searched on https://mariadb.com/kb/en/ and found no relevant pages.
How can this PR be tested?
Most of the changes leverage
ATTRIBUTE_FORMAT
which enables GCC-Wprintf
(we also had-Werror
enabled).ATTRIBUTE_FORMAT
is currently empty for non-GCC builders.What
ATTRIBUTE_FORMAT
cannot capture are the aforementioned error messages.I discovered that
extra/comp_err.c
checks for consistency in fields between localizations; but beyond this, I don’t know how the MariaDB team reviews applications of those error messages.PR quality check
This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.Footnotes
O. Kekäläinen et al., “Can we get MariaDB GitHub CI to consistently be green?” [Mailing list]. Available: https://lists.mariadb.org/hyperkitty/list/[email protected]/thread/NRMKHZ6JRDEWQ73HKV4XYVKHSEY7X3WF/ ↩
The only omission is
static inline void wsrep_override_error(THD*, uint, const char*=, ...)
, whose default argument is inherently a NULL pointer error to-Wformat
. ↩