Add compile-time checking of mp_printf format strings

[jepler]

Summary

It's always nice when errors can be detected at compile time. In traditional C programs, gcc can check that the printf argument types match the printf format string. This has not been possible up to now with mp_printf, because it has both extensions to standard printf (e.g., the %q format type) and is missing things in standard printf (e.g., %zd is not supported).

To that end, I have developed a GCC plugin that does this checking at compile time, and enabled it during the ci build process where possible.

Testing

I built the unix port coverage variant & ran the tests locally. The plugin itself should cause no code changes, and the code size check (which does NOT turn on the plugin) reports no size change.

A total of 45 sites in ci.sh use the new plugin.

Trade-offs and Alternatives

As a gcc plugin this can only support gcc-based toolchains. clang and proprietary compilers cannot use the plugin. This does not seem important, as this feature only produces diagnostics.

The plugin is GPL licensed. I started with a GPL-licensed plugin template, and plugins need to be GPL-or-compatible in license in order to be loaded in gcc anyway. The plugin code IMO does not affect the license situation of the output object code, as you'd get the exact same code with or without the plugin.

Missing support for:

• Knowing whether %ll is runtime-supported
• handling enum argmuents properly (just one enum was %d printed across all checked builds and I added a cast instead of fixing the checker)
• non-gnumake based builds
• mingw-based builds on windows (mingw-based cross builds are checked though)
• mac-based builds

I never specifically measured the build-time impact of enabling the plugin.

[jepler]

Example diagnostic:

coverage.c: In function ‘extra_coverage’:
coverage.c:206:9: error: argument 3: expected ‘long int’ or ‘long unsigned int’, not ‘int’ [-
Werror=format=]
  206 |         mp_printf(&mp_plat_print, "%ld\n", 123); // long
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 98.38%. Comparing base (78ff170) to head (6a18c22).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #17556   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22300    22300           
=======================================
  Hits        21939    21939           
  Misses        361      361           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions[bot]]

Code size report:

Reference:  all: Bump version to 1.27.0. [78ff170]
Comparison: ci: Enable format checking in many builds. [merge of 6a18c22]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

[jepler]

the plugin can also run during the gcc windows builds. I tested it locally using the cross-building steps but I assume it'd work on windows too. I won't complicate this PR by adding it, but I'd plan to add it in a subsequent PR. Some of the format string "findings" came out of me doing that locally.

[jepler]

@dpgeorge Please let me know if you think this is worth pursuing.

[jepler]

I did some checks in a sibling branch, and on macos, gcc is clang (!) which has a different incompatible plugin api; gcc-14 is also installed via brew in the runner, but it is missing some indirect dependency required for plugin building (/opt/homebrew/Cellar/gcc@14/14.3.0/bin/../lib/gcc/14/gcc/aarch64-apple-darwin23/14/plugin/include/system.h:700:10: fatal error: 'gmp.h' file not found). this may be solvable with brew, but as this would not cover any additional source files or integer size models it's probably not worth the time to figure out how to fix it.

[jepler]

Plugin support on Windows/MinGW has a number of limitations and additional requirements so adding support for that would better be postponed to a subsequent PR.

[dpgeorge]

Please let me know if you think this is worth pursuing.

I'm mildly in favour of this.

The three main things to consider would be:

1. The code being GPL licensed: no GPL code in this repo can be part of compiled firmware/executables. But the code added here is only part of the toolchain, which is OK. After all, gcc itself is GPL, and having this new plugin under GPL is the same as that, it has the same scope.
2. Increased complexity of the build process: before building any object files the plugin needs to be built. I guess this is fine, although it does add yet more rules to Make (and eventually CMake).
3. Being enabled by default: this will probably annoy some developers who would either need to install gcc-plugin-dev or disable it each time with DISABLE_PLUGIN=1. Luckily Arch Linux includes gcc-plugin-dev by default (for both gcc and arm-none-eabi-gcc) so that lowers the bar somewhat.

I did test out this PR locally with the unix coverage build and it works well.

It looks like this did find same cases of mp_printf that need to be fixed, so I guess that's a big reason to have it.

[jepler]

Thanks for the feedback.

I did consider trying to automatically enable the plugin if possible, and disable it if not; but this looked fragile.

One option would be to switch it to requring the plugin to be enabled (and enabling it during as many CI jobs as possible). A developer who runs into a diagnostic during CI would then have the option to install the plugin and use ENABLE locally, or whether to make a stab at fixing it and submit another job to CI.

Would it make more sense to work on fixing the diagnosed format problems in a separate PR (some of them do seem to be "real bugs" that will bite at runtime) and then bring the format checker in later? Or are you content to let the fixes and the checker land at the same time, which would make the fixes later?

Final question: C99 "solves" some format string problems by using macros like PRId64 that expand to a correct format string depending on the underlying types (e.g., it might expand to "lld" on an LLp64 sytem or "ld" on an LP64 system). What do you think of introducing such macros in micropython for mp_int_t/mp_uint_t? It looks like there's also a problem integer-printing pointer-width types which leads to the current Windows build failures (and which I'll correct)..

(Some issues are getting fixed in #17538 because the problems DID turn up during CI; this branch will need to be rebased when that one goes in)

[dpgeorge]

One option would be to switch it to requring the plugin to be enabled

Yes, I also thought about that.

At the very least, I think the option should be in the positive tense, eg GCC_ENABLE_MP_PRINTF_PLUGIN, and that could either be disabled by default, or enabled by default on selected builds (eg just unix coverage).

Would it make more sense to work on fixing the diagnosed format problems in a separate PR

Yes, please. That PR would be a much easier thing to review and merge.

Final question: C99 "solves" some format string problems by using macros like PRId64 ... What do you think of introducing such macros in micropython for mp_int_t/mp_uint_t?

Yes, I think that's a good idea.

I have many times considered using the PRIxxx macros for all printf strings. But it's a fair bit of work to do that. But a good idea to start with them for mp_int_t/mp_uint_t.

[jepler]

My thoughts on sequencing the work:

1. Land https://github.com/micropython/micropython/pull/17538 because it has some initial format-string fixes
2. Cherry pick just the fixes from this PR into a new branch and get it to pass CI.
3. Introduce PRI-macros if it helps. Internally I'll use the format checker during this step.
4. Finally, once that new format string fix PR is in, rebase & return to this PR.

[jepler]

huh. two wrongs make a "fascinating". I didn't expect to see code size savings. Is it coming from 3f9b9c4, which I had actually intended to rebase out? Let's find out over at https://github.com/micropython/micropython/pull/17618

[jepler]

More sequencing: Once #17618 goes in, I'll rebase this, add XINT_FMT (the format string to print mp_int_t as hex), and use it for cell printing (@jepler objcell: Fix printing cell ID.). Once that's done and all green with the checker in this PR, I'll create a separate "fixes only" PR.

[jepler]

Rebased. I'll get it back to green, then submit a 2nd PR with "just the bug fixes".

[jepler]

OK, everything this PR depends on has been merged. I've updated the initial comment to reflect the current state of things.

[jepler]

@dpgeorge I think this is ready for review.