| Message ID | 20240221092728.1281499-3-davidgow@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | kunit: Fix printf format specifier issues in KUnit assertions | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Wed, Feb 21, 2024 at 05:27:15PM +0800, David Gow wrote: > The correct format specifier for p - n (both p and n are pointers) is > %td, as the type should be ptrdiff_t. > > This was discovered by annotating KUnit assertion macros with gcc's > printf specifier, but note that gcc incorrectly suggested a %d or %ld > specifier (depending on the pointer size of the architecture being > built). > > Fixes: 0ea09083116d ("lib/cmdline: Allow get_options() to take 0 to validate the input") > Signed-off-by: David Gow <davidgow@google.com> Tested-by: Guenter Roeck <linux@roeck-us.net> > --- > lib/cmdline_kunit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c > index d4572dbc9145..705b82736be0 100644 > --- a/lib/cmdline_kunit.c > +++ b/lib/cmdline_kunit.c > @@ -124,7 +124,7 @@ static void cmdline_do_one_range_test(struct kunit *test, const char *in, > n, e[0], r[0]); > > p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0])); > - KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %u out of bound", n, p - r); > + KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %td out of bound", n, p - r); > } > > static void cmdline_test_range(struct kunit *test) > -- > 2.44.0.rc0.258.g7320e95886-goog >
Hi, On Wed, Feb 21, 2024 at 05:27:15PM +0800, David Gow wrote: > The correct format specifier for p - n (both p and n are pointers) is > %td, as the type should be ptrdiff_t. I think %tu is better. d specifies a signed type. I don't doubt that the warning is fixed but I think %tu represents the type semantics here. > > This was discovered by annotating KUnit assertion macros with gcc's > printf specifier, but note that gcc incorrectly suggested a %d or %ld > specifier (depending on the pointer size of the architecture being > built). > > Fixes: 0ea09083116d ("lib/cmdline: Allow get_options() to take 0 to validate the input") > Signed-off-by: David Gow <davidgow@google.com> > --- > lib/cmdline_kunit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c > index d4572dbc9145..705b82736be0 100644 > --- a/lib/cmdline_kunit.c > +++ b/lib/cmdline_kunit.c > @@ -124,7 +124,7 @@ static void cmdline_do_one_range_test(struct kunit *test, const char *in, > n, e[0], r[0]); > > p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0])); > - KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %u out of bound", n, p - r); > + KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %td out of bound", n, p - r); > } > > static void cmdline_test_range(struct kunit *test) > -- > 2.44.0.rc0.258.g7320e95886-goog > Thanks Justin
On Thu, 22 Feb 2024 at 04:10, 'Justin Stitt' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > Hi, > > On Wed, Feb 21, 2024 at 05:27:15PM +0800, David Gow wrote: > > The correct format specifier for p - n (both p and n are pointers) is > > %td, as the type should be ptrdiff_t. > > I think %tu is better. d specifies a signed type. I don't doubt that the > warning is fixed but I think %tu represents the type semantics here. > While I agree that this should never be negative, I'd still lean on this being a signed type, for two reasons: - I think, if there's a bug in this code, it's easier to debug this if a 'negative' value were to appear as such. - While, as I understand it, the C spec does provide for a ptrdiff_t-sized unsigned printf specifier in '%tu', the difference between two pointers is always signed: "When two pointers are subtracted, both shall point to elements of the same array object, or one past the last element of the array object; the result is the difference of the subscripts of the two array elements. The size of the result is implementation-defined, and its type (a signed integer type) is ptrdiff_t defined in the <stddef.h> header" (Technically, the kernel's ptrdiff_t type isn't defined in stddef.h, so a bit of deviation from the spec is happening anyway, though.) If there's a particularly good reason to make this unsigned in this case, I'd be happy to change it, of course. But I'd otherwise prefer to keep it as-is. Cheers, -- David
On Wed, Feb 21, 2024 at 10:22 PM David Gow <davidgow@google.com> wrote: > > On Thu, 22 Feb 2024 at 04:10, 'Justin Stitt' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > Hi, > > > > On Wed, Feb 21, 2024 at 05:27:15PM +0800, David Gow wrote: > > > The correct format specifier for p - n (both p and n are pointers) is > > > %td, as the type should be ptrdiff_t. > > > > I think %tu is better. d specifies a signed type. I don't doubt that the > > warning is fixed but I think %tu represents the type semantics here. > > > > While I agree that this should never be negative, I'd still lean on > this being a signed type, for two reasons: > - I think, if there's a bug in this code, it's easier to debug this if > a 'negative' value were to appear as such. > - While, as I understand it, the C spec does provide for a > ptrdiff_t-sized unsigned printf specifier in '%tu', the difference > between two pointers is always signed: > > "When two pointers are subtracted, both shall point to elements of the > same array object, > or one past the last element of the array object; the result is the > difference of the > subscripts of the two array elements. The size of the result is > implementation-defined, > and its type (a signed integer type) is ptrdiff_t defined in the > <stddef.h> header" > > (Technically, the kernel's ptrdiff_t type isn't defined in stddef.h, > so a bit of deviation from the spec is happening anyway, though.) > > If there's a particularly good reason to make this unsigned in this > case, I'd be happy to change it, of course. But I'd otherwise prefer > to keep it as-is. Copying the line for context, it's about `p-r` where p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0])); `p-r` should never be negative unless something has gone horribly horribly wrong. So in this particular case, either %tu or %td would be fine. (sorta bikeshedding warning) But, I'd personally lean towards using the signed %td in tests to guard against typos in test code as _a guiding principle._ This is especially true given that the failure messages aren't verified since they are mostly "dead code." You can have crazy incorrect things going on in the format arguments, see patch 1/9 in this series [1]. One of kunit's own tests would do a read from a ~random memory region if that specific assertion failed. Not a good look ;) We never noticed until this series enabled the format string checks. You also can't expect reviewers to go through and modify every assertion to fail to see what the failure mode looks like, so these kinds of errors will continue to slip through. *So IMO, we should generally adopt a more defensive stance when it comes to these.* Also consider the user experience if there is a failure and I accidentally wrote `r-p` here. Someone else sees an error report from this test and needs to investigate. What message is easier to deal with? > in test 18 at -5 out of bound or > in test 18 at 18446744073709551611 out of bound Sure, I can eventually figure out what both messages mean, but it's a immediately obvious from the first that there's a a) real error: something is wrong at index 5 b) test code error: there's a flipped sign somewhere So I'd strongly prefer the current version of the patch over one with %tu. Reviewed-by: Daniel Latypov <dlatypov@google.com> [1] https://lore.kernel.org/linux-kselftest/20240221092728.1281499-2-davidgow@google.com/ Thanks, Daniel
On Thu, 22 Feb 2024 at 09:36, Daniel Latypov <dlatypov@google.com> wrote: > > Copying the line for context, it's about `p-r` where > p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0])); > `p-r` should never be negative unless something has gone horribly > horribly wrong. Sure it would - if 'p' is NULL. Of course, then a negative value wouldn't be helpful either, and in this case that's what the EXPECT_PTR_EQ checking is testing in the first place, so it's a non-issue. IOW, in practice clearly the sign should simply not matter here. I do think that the default case for pointer differences should be that they are signed, because they *can* be. Just because of that "default case", unless there's some actual reason to use '%tu', I think '%td' should be seen as the normal case to use. That said, just as a quick aside: be careful with pointer differences in the kernel. For this particular case, when we're talking about just 'char *', it's not a big deal, but we've had code where people didn't think about what it means to do a pointer difference in C, and how it can be often unnecessarily expensive due to the implied "divide by the size of the pointed object". Sometimes it's actually worth writing the code in ways that avoids pointer differences entirely (which might involve passing around indexes instead of pointers). Linus
diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c index d4572dbc9145..705b82736be0 100644 --- a/lib/cmdline_kunit.c +++ b/lib/cmdline_kunit.c @@ -124,7 +124,7 @@ static void cmdline_do_one_range_test(struct kunit *test, const char *in, n, e[0], r[0]); p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0])); - KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %u out of bound", n, p - r); + KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %td out of bound", n, p - r); } static void cmdline_test_range(struct kunit *test)
The correct format specifier for p - n (both p and n are pointers) is %td, as the type should be ptrdiff_t. This was discovered by annotating KUnit assertion macros with gcc's printf specifier, but note that gcc incorrectly suggested a %d or %ld specifier (depending on the pointer size of the architecture being built). Fixes: 0ea09083116d ("lib/cmdline: Allow get_options() to take 0 to validate the input") Signed-off-by: David Gow <davidgow@google.com> --- lib/cmdline_kunit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)