[RFC] Add compile-time checking of mp_printf format strings #17556
Conversation
|
Example diagnostic: |
df0062d to
c39e2bd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17556 +/- ##
==========================================
- Coverage 98.23% 98.23% -0.01%
==========================================
Files 171 171
Lines 22140 22139 -1
==========================================
- Hits 21749 21748 -1
Misses 391 391 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Code size report: |
0153502 to
536f6fb
Compare
|
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. |
|
@dpgeorge Please let me know if you think this is worth pursuing. |
|
I did some checks in a sibling branch, and on macos, |
|
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. |
I'm mildly in favour of this. The three main things to consider would be:
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 |
|
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) |
Yes, I also thought about that. At the very least, I think the option should be in the positive tense, eg
Yes, please. That PR would be a much easier thing to review and merge.
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 |
|
My thoughts on sequencing the work:
|
990966c to
f0a7d80
Compare
The name field of type objects is of type uint16_t for efficiency, but when the type is passed to mp_printf it must be cast explicitly to type qstr. These locations were found using an experimental gcc plugin for mp_printf error checking, cross-building for x64 windows on Linux. Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
The type of the argument must match the format string. Add casts to ensure that they do. It's possible that casting from `size_t` to `unsigned` loses the correct values by masking off upper bits, but it seems likely that the quantities involved in practice are small enough that the %u formatter (32 bits on most platforms, 16 on pic16bit) will in fact hold the correct value. The alternative, casting to a wider type, adds code size. These locations were found using an experimental gcc plugin for mp_printf error checking, cross-building for x64 windows on Linux. In one case there was already a cast, but it was written in 8000 correctly and did not have the intended effect. Signed-off-by: Jeff Epler <jepler@gmail.com>
These locations were found using an experimental gcc plugin for mp_printf error checking. Signed-off-by: Jeff Epler <jepler@gmail.com>
During the coverage test, all the values encountered are within the range of %d. These locations were found using an experimental gcc plugin for mp_printf error checking. Signed-off-by: Jeff Epler <jepler@gmail.com>
On the nanbox build, `o->obj` is a 64-bit type but `%p` formats a 32-bit type, leading to undefined behavior. Print the cell's ID as an integer instead. It can't be printed in hex because the '%llx' format specifier is not supported in mp_printf. This location was found using an experimental gcc plugin for mp_printf error checking. Signed-off-by: Jeff Epler <jepler@gmail.com>
we still want this not to crash a runtime but the new static checker wouldn't like it. Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
This adds support for %llx (needed by XINT_FMT for printing cell objects) and incidentally support for capitalized output of %P. Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
f0a7d80 to
3bb0775
Compare
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
%qformat type) and is missing things in standard printf (e.g.,%zdis not supported).To that end, I have developed a GCC plugin that does this checking at compile time. I've also made the necessary changes for the unix coverage build to complete with the plugin enabled, and enabled it during the coverage build process.
Creating in draft mode to get feedback and also because this is cumulative with some other outstanding PRs that are needed to get the CI board to green.
Testing
I built the unix port coverage variant & ran the tests locally. The plugin itself should cause no code changes. There is a small code growth reported, so one of the added casts must not actually be a no-op. I have not determined which one.
Trade-offs and Alternatives
As a gcc plugin this can only support gcc-based toolchains. clang and proprietary compilers would not work. 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:
%llis runtime-supportedPRId32for printing mp_{u,}int_t values: some ports need%dand others%ld, and maybe some even need%lld(I think maybe 32-bit nanbox builds would require this, for example). e.g.,#define PRIdPY "lld"next totypedef long long mp_int_t. This would replace(int)casts which was the easiest way to get local builds to finish.CI may need a new package installed -- debian needed
gcc-12-plugin-devand there's nogcc-plugin-dev(w/o version number) package to install the 'usual' one). I tried to code this, we'll see if it works.Whether to enable it on more ports. This could catch problems in port-specific files, or for different fundamental object sizes.
Adding support for cmake-based builds and any other oddball build configurations