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@xxxxxxxxxx> 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? >
