| Message ID | 20220802161206.228707-1-mairacanal@riseup.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
On Tue, Aug 2, 2022 at 9:12 AM Maíra Canal <mairacanal@riseup.net> wrote: > > Currently, in order to compare arrays in KUnit, the KUNIT_EXPECT_EQ or > KUNIT_EXPECT_FALSE macros are used in conjunction with the memcmp function, > such as: > KUNIT_EXPECT_EQ(test, memcmp(foo, bar, size), 0); > > Although this usage produces correct results for the test cases, if the > expectation fails the error message is not very helpful, indicating only the > return of the memcmp function. > > Therefore, create a new set of macros KUNIT_EXPECT_ARREQ and > KUNIT_EXPECT_ARRNEQ that compare memory blocks until a determined size. In > case of expectation failure, those macros print the hex dump of the memory > blocks, making it easier to debug test failures for arrays. I totally agree with this. The only reason I hadn't sent an RFC out for this so far is * we didn't have enough use cases quite yet (now resolved) * I wasn't sure how we'd want to format the failure message. For the latter, right now this series produces dst == 00000000: 33 0a 60 12 00 a8 00 00 00 00 8e 6b 33 0a 60 12 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00 result->expected == 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00 I was thinking something like what KASAN produces would be nice, e.g. from https://www.kernel.org/doc/html/v5.19/dev-tools/kasan.html#error-reports (I'll paste the bit here, but my email client doesn't support monospaced fonts, so it won't look nice on my end) Memory state around the buggy address: ffff8801f44ec200: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb ffff8801f44ec280: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc >ffff8801f44ec300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 03 ^ I just wasn't quite sure how to do it for a diff, since this only really works well when showing one bad byte. If we blindly followed that approach, we get dst == >00000000: 33 0a 60 12 00 a8 00 00 00 00 8e 6b 33 0a 60 12 ^ >00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00 ^ result->expected == >00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12 ^ >00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00 ^ But perhaps we could instead highlight the bad bytes with something like dst == 00000000: 33 0a 60 12 00 a8 00 00 00 00 <8e> 6b 33 0a 60 12 00000010: 00 00 00 00 <00> a8 8e 6b 33 0a 00 00 00 00 result->expected == 00000000: 31 0a 60 12 00 a8 00 00 00 00 <81> 6b 33 0a 60 12 00000010: 00 00 00 00 <01> a8 8e 6b 33 0a 00 00 00 00 Thoughts, suggestions?
On 8/2/22 13:59, 'Daniel Latypov' via KUnit Development wrote: > On Tue, Aug 2, 2022 at 9:12 AM Maíra Canal <mairacanal@riseup.net> wrote: >> >> Currently, in order to compare arrays in KUnit, the KUNIT_EXPECT_EQ or >> KUNIT_EXPECT_FALSE macros are used in conjunction with the memcmp function, >> such as: >> KUNIT_EXPECT_EQ(test, memcmp(foo, bar, size), 0); >> >> Although this usage produces correct results for the test cases, if the >> expectation fails the error message is not very helpful, indicating only the >> return of the memcmp function. >> >> Therefore, create a new set of macros KUNIT_EXPECT_ARREQ and >> KUNIT_EXPECT_ARRNEQ that compare memory blocks until a determined size. In >> case of expectation failure, those macros print the hex dump of the memory >> blocks, making it easier to debug test failures for arrays. > > I totally agree with this. > > The only reason I hadn't sent an RFC out for this so far is > * we didn't have enough use cases quite yet (now resolved) > * I wasn't sure how we'd want to format the failure message. > > For the latter, right now this series produces > dst == > 00000000: 33 0a 60 12 00 a8 00 00 00 00 8e 6b 33 0a 60 12 > 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00 > result->expected == > 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12 > 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00 > > I was thinking something like what KASAN produces would be nice, e.g. > from https://www.kernel.org/doc/html/v5.19/dev-tools/kasan.html#error-reports > (I'll paste the bit here, but my email client doesn't support > monospaced fonts, so it won't look nice on my end) > > Memory state around the buggy address: > ffff8801f44ec200: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb > ffff8801f44ec280: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc >> ffff8801f44ec300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 03 > ^ > I just wasn't quite sure how to do it for a diff, since this only > really works well when showing one bad byte. > If we blindly followed that approach, we get > > dst == >> 00000000: 33 0a 60 12 00 a8 00 00 00 00 8e 6b 33 0a 60 12 > ^ >> 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00 > ^ > result->expected == >> 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12 > ^ >> 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00 > ^ > > But perhaps we could instead highlight the bad bytes with something like > dst == > 00000000: 33 0a 60 12 00 a8 00 00 00 00 <8e> 6b 33 0a 60 12 > 00000010: 00 00 00 00 <00> a8 8e 6b 33 0a 00 00 00 00 > result->expected == > 00000000: 31 0a 60 12 00 a8 00 00 00 00 <81> 6b 33 0a 60 12 > 00000010: 00 00 00 00 <01> a8 8e 6b 33 0a 00 00 00 00 My problem with this approach is that the bytes get slightly misaligned when adding the <>. Maybe if we aligned as: dst: 00000000: <33> 0a 60 12 00 a8 00 00 00 00 <8e> 6b 33 0a 60 12 00000010: 00 00 00 00 <00> a8 8e 6b 33 0a 00 00 00 00 result->expected: 00000000: <31> 0a 60 12 00 a8 00 00 00 00 <81> 6b 33 0a 60 12 00000010: 00 00 00 00 <01> a8 8e 6b 33 0a 00 00 00 00 Although I don't know exactly how we can produce this output. I was using hex_dump_to_buffer to produce the hexdump, so maybe I need to change the strategy to generate the hexdump. I guess the KASAN approach could be easier to implement. But I guess it can turn out to be a little polluted if many bytes differ. For example: dst: 00000000: 33 31 31 31 31 31 31 31 31 31 8e 31 33 0a 60 12 ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00 ^ result->expected: 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12 ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00 ^ I don't know exactly with option I lean. Thank you for your inputs, Daniel! - Maíra Canal > > Thoughts, suggestions? >
On Tue, Aug 2, 2022 at 11:43 AM Maíra Canal <mairacanal@riseup.net> wrote: > > But perhaps we could instead highlight the bad bytes with something like > > dst == > > 00000000: 33 0a 60 12 00 a8 00 00 00 00 <8e> 6b 33 0a 60 12 > > 00000010: 00 00 00 00 <00> a8 8e 6b 33 0a 00 00 00 00 > > result->expected == > > 00000000: 31 0a 60 12 00 a8 00 00 00 00 <81> 6b 33 0a 60 12 > > 00000010: 00 00 00 00 <01> a8 8e 6b 33 0a 00 00 00 00 > > My problem with this approach is that the bytes get slightly misaligned > when adding the <>. Maybe if we aligned as: > > dst: > 00000000: <33> 0a 60 12 00 a8 00 00 00 00 <8e> 6b 33 0a 60 12 > 00000010: 00 00 00 00 <00> a8 8e 6b 33 0a 00 00 00 00 > result->expected: > 00000000: <31> 0a 60 12 00 a8 00 00 00 00 <81> 6b 33 0a 60 12 > 00000010: 00 00 00 00 <01> a8 8e 6b 33 0a 00 00 00 00 And yes, that's a good point re alignment. Handling that would be annoying and perhaps a reason to leave this off until later. Perhaps in the short-term, we could add output like First differing byte at index 0 if others think that could be useful. I'm quite surprised I didn't notice the first bytes differed (as you can tell from my example), so I personally would have been helped out by such a thing. > > Although I don't know exactly how we can produce this output. I was > using hex_dump_to_buffer to produce the hexdump, so maybe I need to > change the strategy to generate the hexdump. Indeed, we'd probably have to write our own code to do this. I think it might be reasonable to stick with the code as-is so we can just reuse hex_dump_to_buffer. We'd then be able to think about the format more and bikeshed without blocking this patch. But note: we could leverage string_stream to build up the output a bit more easily than you might expect. Here's a terrible first pass that you can paste into kunit-example-test.c #include "string-stream.h" static void diff_hex_dump(struct kunit *test, const u8 *a, const u8 *b, size_t num_bytes, size_t row_size) { size_t i; struct string_stream *stream1 = alloc_string_stream(test, GFP_KERNEL); struct string_stream *stream2 = alloc_string_stream(test, GFP_KERNEL); for (i = 0; i < num_bytes; ++i) { if (i % row_size) { string_stream_add(stream1, " "); string_stream_add(stream2, " "); } else { string_stream_add(stream1, "\n> "); string_stream_add(stream2, "\n> "); } if (a[i] == b[i]) { string_stream_add(stream1, "%02x", a[i]); string_stream_add(stream2, "%02x", b[i]); } else { string_stream_add(stream1, "<%02x>", a[i]); string_stream_add(stream2, "<%02x>", b[i]); } } string_stream_add(stream1, "\nwant"); string_stream_append(stream1, stream2); kunit_info(test, "got%s\n", string_stream_get_string(stream1)); } static void example_hex_test(struct kunit *test) { const u8 a1[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0xde, 0xad, 0xbe, 0xef}; const u8 a2[] = {0x1, 0x3, 0x2, 0x4, 0x5, 0x6, 0x7, 0xde, 0xad, 0xbe, 0xef}; diff_hex_dump(test, a1, a2, sizeof(a1), 8); } It produces the following output: # example_hex_test: got > 01 <02> <03> 04 05 06 07 de > ad be ef want > 01 <03> <02> 04 05 06 07 de > ad be ef It doesn't handle re-aligning the other bytes as you'd pointed out above. > > I guess the KASAN approach could be easier to implement. But I guess it > can turn out to be a little polluted if many bytes differ. For example: > > dst: > 00000000: 33 31 31 31 31 31 31 31 31 31 8e 31 33 0a 60 12 > ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ > 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00 > ^ > result->expected: > 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12 > ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ > 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00 > ^ > > I don't know exactly with option I lean. Agreed, it doesn't scale up too well when pointing out >1 buggy bytes.
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c index 3106abb3bead..942aa131a768 100644 --- a/drivers/gpu/drm/tests/drm_format_helper_test.c +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c @@ -131,9 +131,9 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = { .rgb565_result = { .dst_pitch = 10, .expected = { - 0x0A33, 0x1260, 0xA800, 0x0000, 0x0000, - 0x6B8E, 0x0A33, 0x1260, 0x0000, 0x0000, - 0xA800, 0x6B8E, 0x0A33, 0x0000, 0x0000, + 0x0A31, 0x1260, 0xA800, 0x0000, 0x0000, + 0x6B81, 0x0A33, 0x1260, 0x0000, 0x0000, + 0xA801, 0x6B8E, 0x0A33, 0x0000, 0x0000, }, .expected_swab = { 0x330A, 0x6012, 0x00A8, 0x0000, 0x0000,}}}