Message ID | 20240816153251.2833702-6-andre.przywara@arm.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | kselftest/arm64: various compilation fixes | expand |
On Fri, Aug 16, 2024 at 04:32:48PM +0100, Andre Przywara wrote: > When masking the return value of a prctl, which is clearly an "int", we > use a uapi header provided mask, which is defined using an "UL" modifier, > so the whole expression is promoted to a long. This upsets the compiler's > printf type checker, because we use "%x" in the format string. > > While we could simply upgrade this to a "%lx", it sounds wrong to > promote the "ret" variable, that is clearly an int. > Downcast the mask instead, to keep the type correct. This suggests that we've got some confusion with the UAPI, these flags need to go through a prctl() return so they shouldn't be unsigned long... That said, it's UAPI so I'm not sure that's fixable. > - if ((ret & PR_MTE_TCF_MASK) == mask) { > + if ((ret & (int)PR_MTE_TCF_MASK) == mask) { > ksft_test_result_pass("%s\n", name); > } else { > ksft_print_msg("Got %x, expected %x\n", > - (ret & PR_MTE_TCF_MASK), mask); > + (ret & (int)PR_MTE_TCF_MASK), mask); > ksft_test_result_fail("%s\n", name); TBH my inclination is that this is worse than letting the value be promoted, casts (particularly casts of constants) are just obviously suspect in a way that printf() formats aren't.
On Fri, 16 Aug 2024 17:30:14 +0100 Mark Brown <broonie@kernel.org> wrote: Hi Mark, thanks for having a look! > On Fri, Aug 16, 2024 at 04:32:48PM +0100, Andre Przywara wrote: > > When masking the return value of a prctl, which is clearly an "int", we > > use a uapi header provided mask, which is defined using an "UL" modifier, > > so the whole expression is promoted to a long. This upsets the compiler's > > printf type checker, because we use "%x" in the format string. > > > > While we could simply upgrade this to a "%lx", it sounds wrong to > > promote the "ret" variable, that is clearly an int. > > Downcast the mask instead, to keep the type correct. > > This suggests that we've got some confusion with the UAPI, these flags > need to go through a prctl() return so they shouldn't be unsigned > long... That said, it's UAPI so I'm not sure that's fixable. My thoughts, exactly. Not sure if this mask is used on some larger types somewhere else, but it's indeed moot since it's UAPI. > > - if ((ret & PR_MTE_TCF_MASK) == mask) { > > + if ((ret & (int)PR_MTE_TCF_MASK) == mask) { > > ksft_test_result_pass("%s\n", name); > > } else { > > ksft_print_msg("Got %x, expected %x\n", > > - (ret & PR_MTE_TCF_MASK), mask); > > + (ret & (int)PR_MTE_TCF_MASK), mask); > > ksft_test_result_fail("%s\n", name); > > TBH my inclination is that this is worse than letting the value be > promoted, casts (particularly casts of constants) are just obviously > suspect in a way that printf() formats aren't. Fair enough, I wasn't sure about this either, and indeed this down-cast of a constant smells dodgy. So shall I just use %lx, and rely on the promotion (so the MASK being defined as UL), or cast "ret" to long? Cheers, Andre
On Fri, Aug 16, 2024 at 05:55:48PM +0100, Andre Przywara wrote: > Mark Brown <broonie@kernel.org> wrote: > > On Fri, Aug 16, 2024 at 04:32:48PM +0100, Andre Przywara wrote: > > > ksft_print_msg("Got %x, expected %x\n", > > > - (ret & PR_MTE_TCF_MASK), mask); > > > + (ret & (int)PR_MTE_TCF_MASK), mask); > > TBH my inclination is that this is worse than letting the value be > > promoted, casts (particularly casts of constants) are just obviously > > suspect in a way that printf() formats aren't. > Fair enough, I wasn't sure about this either, and indeed this down-cast of > a constant smells dodgy. So shall I just use %lx, and rely on the > promotion (so the MASK being defined as UL), or cast "ret" to long? My inclination would be to go with the former.
diff --git a/tools/testing/selftests/arm64/mte/check_prctl.c b/tools/testing/selftests/arm64/mte/check_prctl.c index f139a33a43ef4..a16d7005affdf 100644 --- a/tools/testing/selftests/arm64/mte/check_prctl.c +++ b/tools/testing/selftests/arm64/mte/check_prctl.c @@ -81,11 +81,11 @@ void set_mode_test(const char *name, int hwcap2, int mask) return; } - if ((ret & PR_MTE_TCF_MASK) == mask) { + if ((ret & (int)PR_MTE_TCF_MASK) == mask) { ksft_test_result_pass("%s\n", name); } else { ksft_print_msg("Got %x, expected %x\n", - (ret & PR_MTE_TCF_MASK), mask); + (ret & (int)PR_MTE_TCF_MASK), mask); ksft_test_result_fail("%s\n", name); } }
When masking the return value of a prctl, which is clearly an "int", we use a uapi header provided mask, which is defined using an "UL" modifier, so the whole expression is promoted to a long. This upsets the compiler's printf type checker, because we use "%x" in the format string. While we could simply upgrade this to a "%lx", it sounds wrong to promote the "ret" variable, that is clearly an int. Downcast the mask instead, to keep the type correct. Fixes: e2d9642a5a51 ("kselftest/arm64: Add simple test for MTE prctl") Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- tools/testing/selftests/arm64/mte/check_prctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)