[MDEV-21978 part 2] Enable and Fix GCC `-Wformat` Checks on `my_vsnprintf` and Users #3360

ParadoxV5 · 2024-06-26T03:27:35Z

The Jira issue number for this PR is: MDEV-21978
- Zulip thread
- Possibly supercedes MDEV-19784
Resolves push_warning_printf: gain attribute(printf) #1531 (comment)
Prerequisite (MDEV-21978 & GSoC part 1): MDEV-21978 make my_vsnprintf to use gcc-compatible format extensions #3309
- This PR bases on that prerequisite PR and currently shows up inheriting those commits.
  I will keep this branch updated with that prerequisite branch.
  When that prerequisite merges, I will rebase this PR to the latest branch directly.
Google Summer of Code (GSoC) 2024 milestone 2
- … And it even assigns me a responsibility I’ve rarely taken before – housekeeping. Everyone dreads chores, amirite?
- My experience was also regular C, not C++, and I thought I wouldn’t need to deal with C++ for this project. 😶‍🌫️

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.¹

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 all² functions that use those functions (see § Release Notes for notable ones) with __attribute__((format(printf, …))) (via ATTRIBUTE_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.

10.5: Extract some of #3360 fixes to 10.5.x #3485
10.6: Extract some of #3360 fixes to 10.6.x #3493
10.11: Extract some of #3360 fixes to 10.11.x #3518
11.2: Fix DBUG_PRINT format of group_trp->records #3537
11.4: TODO
11.5: Add missing LEX_STRING::strs for my_snprintf #3538

Passing content directly as the format string
- I solved most of them with the simple "%s" fix. For push_warning_printf, I switched them to the non-printf equivalent push_warning.
Missing or extraneous arguments missed by refactorings
Passing our string structs to %s (imlicitly casting to char*)
Mixed use of 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 descriptions

The 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 from errmsg-utf8.txt, mysys/errors.c and sql-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 and my_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 check printf formats and arguments and catch mistakes.
Howëver, my_vsnprintf and co., our in-house platform-agnostic replacement for the printf 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 on my_snprintfs 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

Go out of my way to catch argument type mistakes that GCC -Wformat doesn’t complain
- e.g.,
  - feeding an unsigned int to a %d
  - (implicitly or explicitly) casting fixed-size integer args to platform-dependent types rather than utilizing fixed-size format macros (e.g. PRId32)
- I can only lightly review manually on a best-effort basis – I’m not interested in tracking down types through layers of C++ class inheritance.
  - Just the samples I came across already uncovered many instances. Catching them all will be a laborious and intimidating project.
  - I only did an extra meter on sql/table.cc because the main process already heavily modified the file.
Tag other snprintf alternatives with ATTRIBUTE_FORMATand fix calls to them or the C/C++ snprintf itself
- e.g., my_vsnprintf_utf32
- e.g., my_b_printf
- This PR only concerns with my_vsnprintf (my vsnprintf) and those that use it.
Port every (uncaught) “copy-paste” calls (those that simply clone the format string over and formats no args) to alternatives if available
- I’ve divided those for my_snprintf to Use strmake over my_snprintf to copy strings #3429.
  They will likely conflict with the old patch here; in which case, that PR’s strmakes take precedence.
Reformat all those old entire files to match the CODING_STANDARDS.md
- This includes normalizing tabs to spaces.

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 with printf checks such as GCC -Wformat.
- This applies to both my_snprintf and all functions that utilize it, such as:
```
service_my_snprintf
  my_snprintf
  my_vsnprintf
service_logger
  logger_printf
  logger_vprintf
service_my_print_error
  my_printf_error
  my_printv_error
DBUG_PRINT
  _db_doprnt_
push_warning_printf
```
  (Refer to the commit history for the complete listing)
- On that note, all those functions also gained __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 new feature or a refactoring, and the PR is based against the latest MariaDB development branch.
- This PR bases on the prerequisite PR which itself is based on the latest development branch.
~~This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.~~
I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

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. ↩

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.

ParadoxV5 · 2024-06-29T03:14:53Z

To confirm, are our -Wformat flags good?

Update: Actually, let me see if it can be -Wformat=2.

Update: Nope. Too many -Wformat-nonliteral concerns (P.S. some from submodules too) that it well exceeds my project’s scope. I’ll focus on my_snprintf and delegates only.

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.

ParadoxV5

Recent force-pushes extracted some work for latter commits (i.e., commit split & reörder).
The previous force-push also amends mistakes:

Use NullS over nullptr
Update assertion count in unit test

sql/sql_admin.cc

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.

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”.

ParadoxV5

The changeset is getting wide – LMK if you prefer certain commits squashed together.

Regarding client/mysqltest.cc,

server/client/mysqltest.cc

Line 25 in 25b5c63

Please keep the test framework tools identical in all versions!

What does it mean by this line?

server/client/mysqltest.cc

Line 36 in 25b5c63

#define VER "3.5"

Do I need to increase this version tag… if yes, how?

ParadoxV5 · 2024-08-02T23:12:10Z

storage/spider/spd_db_mysql.cc

@@ -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]);


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).

you don't need to increase the version tag.

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]

ParadoxV5 · 2024-08-02T23:14:16Z

storage/oqgraph/ha_oqgraph.cc

@@ -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);


git blame 4e66318

sql/item_cmpfunc.cc

plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc

ParadoxV5 · 2024-08-02T23:25:15Z

sql/item_xmlfunc.cc

      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);


(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.

ParadoxV5 · 2024-08-02T23:41:27Z

storage/innobase/handler/ha_innodb.cc

 		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);


(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.

sql/sys_vars.cc

storage/maria/s3_func.c

ParadoxV5 · 2024-08-05T04:14:33Z

sql/opt_range.cc

@@ -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",


Fix DBUG_PRINT format of group_trp->records #3537

ParadoxV5 · 2024-08-05T04:21:09Z

sql/table.cc

-            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


t is the size modifier for ptrdiff_t, similar to z is for size_t.
https://en.cppreference.com/w/c/io/fprintf#Parameters

ParadoxV5 · 2024-08-05T04:48:17Z

storage/innobase/fsp/fsp0fsp.cc

@@ -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));


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;

And now %zu has backfired in storage/innobase/dict/dict0load.cc.

ParadoxV5 · 2024-08-05T05:28:56Z

sql/sql_class.h

-#ifdef WITH_WSREP
 #include <inttypes.h>
+#ifdef WITH_WSREP


(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.

sql/log.cc

ParadoxV5 · 2024-08-10T03:42:14Z

sql/log.h

@@ -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 isn’t a guideline on where to put attributes.
I checked existing entries: most put attributes after the signature, a few put them before.

sql/net_serv.cc

ParadoxV5 · 2024-08-10T03:43:28Z

storage/innobase/btr/btr0btr.cc

@@ -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);


I guessed this one as well and could use a contact.

ParadoxV5 · 2024-08-10T03:43:48Z

storage/innobase/log/log0recv.cc

@@ -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));


git blame 28b27b9

The ' is a cosmetic fix.

ParadoxV5 · 2024-08-10T03:45:26Z

sql/log_event_server.cc

@@ -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 "


I would use PRIu64 over %llu for the uint64 here, but somehow it doesn’t work, at least on my (WSL) Ubuntu.

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.

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes.

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes (excludes MariaDB#3485).

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes.

ParadoxV5 · 2024-09-13T05:59:17Z

storage/myisam/mi_check.c

+				  "Found block with impossible length %lu at %s; Skipped",
+				  block_info.block_len+ (ulong) (block_info.filepos-pos),


I forgot how I came up with this…

Other (block_info.filepos-pos)s nearby were casted to (uint)s.
For consistency, either this should be reverted or the other instances should also change (improve).

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes. (Excludes previous parts MariaDB#3485 and MariaDB#3493)

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes.

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes. (Excludes previous parts MariaDB#3485 and MariaDB#3493)

ParadoxV5 · 2024-09-14T22:14:15Z

sql/log_event.cc

@@ -3149,7 +3149,7 @@ Rows_log_event::Rows_log_event(const uchar *buf, uint event_len,
  uchar *ptr_after_width= (uchar*) ptr_width;
  DBUG_PRINT("debug", ("Reading from %p", ptr_after_width));
  m_width= net_field_length(&ptr_after_width);
-  DBUG_PRINT("debug", ("m_width=%lu", m_width));
+  DBUG_PRINT("debug", ("m_width=%u", m_width));


But

server/sql/log_event.cc

Line 500 in dd06777

ulong m_width= net_field_length((uchar **)&tmp);

I forgot where this change came from.

ParadoxV5 mentioned this pull request Jun 26, 2024

MDEV-21978 make my_vsnprintf to use gcc-compatible format extensions #3309

Open

3 tasks

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from 65dceed to a488e87 Compare June 29, 2024 03:09

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from a488e87 to 4c51a35 Compare June 29, 2024 21:42

ParadoxV5 commented Jun 29, 2024

View reviewed changes

sql/sql_admin.cc Show resolved Hide resolved

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from 2dff369 to dc990a8 Compare July 9, 2024 21:18

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from dc990a8 to f1425a2 Compare July 11, 2024 19:18

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch 2 times, most recently from 1207c8d to a97e4e5 Compare July 20, 2024 04:13

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from a97e4e5 to dcd8abc Compare July 26, 2024 04:14

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from dcd8abc to 43c7984 Compare July 27, 2024 20:08

ParadoxV5 mentioned this pull request Jul 28, 2024

Use strmake over my_snprintf to copy strings #3429

Open

3 tasks

ParadoxV5 commented Aug 2, 2024

View reviewed changes

ParadoxV5 commented Aug 5, 2024

View reviewed changes

storage/maria/s3_func.c Show resolved Hide resolved

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from 20711ef to 8f4ee14 Compare August 5, 2024 04:10

ParadoxV5 commented Aug 5, 2024

View reviewed changes

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from 8f4ee14 to 4544a03 Compare August 5, 2024 05:16

ParadoxV5 commented Aug 5, 2024

View reviewed changes

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from 4544a03 to dd6d9ea Compare August 5, 2024 21:04

ParadoxV5 commented Aug 10, 2024

View reviewed changes

ParadoxV5 added 11 commits August 15, 2024 22:52

Tag the sql/log.h family with ATTRIBUTE_FORMAT

ed3c751

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

Tag myisamdef.h printers with ATTRIBUTE_FORMAT

f89abfe

Let GCC `-Wformat` check the formats sent to these `my_vsnprintf` users

Update ABIs

f4fc288

* Run `make abi_update` * Revert the commit “Allow ABI mismatches temporarily” * Remove trailing spaces in `cmake/do_abi_check.cmake`

ParadoxV5 force-pushed the mdev-21978-attribute-printf branch from 1798af1 to dd06777 Compare August 16, 2024 05:10

ParadoxV5 changed the title ~~[MDEV-21978 part 2] Enable and Fix GCC printf Checks on my_vsnprintf and Users~~ [MDEV-21978 part 2] Enable and Fix GCC -Wformat Checks on my_vsnprintf and Users Aug 18, 2024

ParadoxV5 marked this pull request as ready for review August 19, 2024 03:31

ParadoxV5 changed the base branch from 11.6 to main August 27, 2024 23:05

ParadoxV5 added a commit to ParadoxV5/MariaDB-server that referenced this pull request Aug 28, 2024

Extract some of MariaDB#3360 fixes to 10.5.x

ab0e105

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes.

ParadoxV5 added a commit to ParadoxV5/MariaDB-server that referenced this pull request Aug 28, 2024

Extract some of MariaDB#3360 fixes to 10.5.x

a2aba56

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes.

ParadoxV5 mentioned this pull request Aug 28, 2024

Extract some of #3360 fixes to 10.5.x #3485

Open

3 tasks

ParadoxV5 added a commit to ParadoxV5/MariaDB-server that referenced this pull request Aug 29, 2024

Extract some of MariaDB#3360 fixes to 10.5.x

eca5a6f

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes.

ParadoxV5 added a commit to ParadoxV5/MariaDB-server that referenced this pull request Sep 1, 2024

Extract some of MariaDB#3360 fixes to 10.6.x

9221411

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes (excludes MariaDB#3485).

ParadoxV5 mentioned this pull request Sep 1, 2024

Extract some of #3360 fixes to 10.6.x #3493

Open

3 tasks

ParadoxV5 added a commit to ParadoxV5/MariaDB-server that referenced this pull request Sep 2, 2024

Extract some of MariaDB#3360 fixes to 10.5.x

c56a501

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes.

ParadoxV5 commented Sep 13, 2024

View reviewed changes

ParadoxV5 added a commit to ParadoxV5/MariaDB-server that referenced this pull request Sep 13, 2024

Extract some of MariaDB#3360 fixes to 10.5.x

562f5a6

That PR uncovered countless issues on `my_snprintf` uses. This commit backports a squashed subset of their fixes.

ParadoxV5 mentioned this pull request Sep 14, 2024

Extract some of #3360 fixes to 10.11.x #3518

Open

3 tasks

ParadoxV5 commented Sep 14, 2024

View reviewed changes

This was referenced Sep 21, 2024

Fix DBUG_PRINT format of group_trp->records #3537

Open

Add missing LEX_STRING::strs for my_snprintf #3538

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MDEV-21978 part 2] Enable and Fix GCC `-Wformat` Checks on `my_vsnprintf` and Users #3360

[MDEV-21978 part 2] Enable and Fix GCC `-Wformat` Checks on `my_vsnprintf` and Users #3360

ParadoxV5 commented Jun 26, 2024 •

edited

Loading

ParadoxV5 commented Jun 29, 2024 •

edited

Loading

ParadoxV5 left a comment

ParadoxV5 left a comment

ParadoxV5 Aug 2, 2024

vuvova Aug 10, 2024

ParadoxV5 Aug 28, 2024 •

edited

Loading

ParadoxV5 Aug 2, 2024 •

edited

Loading

ParadoxV5 Aug 2, 2024

ParadoxV5 Aug 2, 2024

ParadoxV5 Aug 5, 2024

ParadoxV5 Sep 21, 2024

ParadoxV5 Aug 5, 2024

ParadoxV5 Aug 5, 2024

ParadoxV5 Aug 10, 2024

ParadoxV5 Aug 5, 2024 •

edited

Loading

ParadoxV5 Aug 10, 2024

ParadoxV5 Aug 10, 2024

ParadoxV5 Aug 10, 2024

ParadoxV5 Aug 18, 2024

ParadoxV5 Aug 10, 2024

ParadoxV5 Sep 13, 2024

ParadoxV5 Sep 14, 2024 •

edited

Loading

	/** 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;

		"Found block with impossible length %lu at %s; Skipped",
		block_info.block_len+ (ulong) (block_info.filepos-pos),

[MDEV-21978 part 2] Enable and Fix GCC -Wformat Checks on my_vsnprintf and Users #3360

Are you sure you want to change the base?

[MDEV-21978 part 2] Enable and Fix GCC -Wformat Checks on my_vsnprintf and Users #3360

Conversation

ParadoxV5 commented Jun 26, 2024 • edited Loading

Why wasn’t this project merged during GSoC?

Description

What problem is the patch trying to solve?

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?

Do you think this patch might introduce side-effects in other parts of the server?

non-goals

Release Notes

How can this PR be tested?

PR quality check

Footnotes

ParadoxV5 commented Jun 29, 2024 • edited Loading

ParadoxV5 left a comment

Choose a reason for hiding this comment

ParadoxV5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

ParadoxV5 Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

[MDEV-21978 part 2] Enable and Fix GCC `-Wformat` Checks on `my_vsnprintf` and Users #3360

[MDEV-21978 part 2] Enable and Fix GCC `-Wformat` Checks on `my_vsnprintf` and Users #3360

ParadoxV5 commented Jun 26, 2024 •

edited

Loading

ParadoxV5 commented Jun 29, 2024 •

edited

Loading

ParadoxV5 Aug 28, 2024 •

edited

Loading

ParadoxV5 Aug 2, 2024 •

edited

Loading

ParadoxV5 Aug 5, 2024 •

edited

Loading

ParadoxV5 Sep 14, 2024 •

edited

Loading