Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44669 )
Change subject: tests: Add lib/imd-test test case ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c File tests/lib/imd-test.c:
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@9 PS2, Line 9: #include <limits.h> What do you need this for? I can't see anything that needs it but I may have missed something.
In general, I think we probably need to be more careful with mixing glibc headers and coreboot headers together, because it is very dangerous. coreboot headers always take precedence, so if you add a dependency on say glibc's <limits.h> here now and then later we want to add a <limits.h> to coreboot that maybe not covers the whole POSIX-required range of definitions, things will break. Even worse, maybe glibc internally decides that <limits.h> should include something else like <stddef.h> to include something that glibc knows are in its own <stddef.h>, but it doesn't know that it would actually get coreboot's version with that include which might not contain what it needs. These kinds of things could change randomly between glibc versions and break things.
In fact, I wonder if we should just build tests with -nostdinc to completely ensure we can't have these issues. Cmocka needs some libc features, but thankfully it's all compiled separately so we could compile that with glibc headers but the tests themselves without them. Only expect_assert_failure() with setjmp() is a problem, unfortunately... to make that work the part calling setjmp() would also have to be hidden in a .c file that is compiled and linked separately. I guess that could be tone if <tests/test.h> does an #undef expect_assert_failure() and replaces it with our own, though a bit hacky.
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@13 PS2, Line 13: void *memset(void *ptr, int value, size_t num); For these kinds of cases I would suggest to centralize them rather than than defining them manually in every test. You could put them in #include <tests/libc/stdlib.h> or something like that. (memset() is provided by coreboot's <string.h>, btw, so you could just include that instead of providing it separately.)
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@38 PS2, Line 38: } __packed; Let's not duplicate all of this stuff, let's just factor them out from the code under test into a separate header (e.g. src/include/imd_private.h or something). (I know I usually say coreboot code shouldn't need to change to conform to tests, but factoring out definitions should be simple and harmless enough.)
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@43 PS2, Line 43: #define CBMEM_ID_IMD_SMALL 0x53a11439 Don't you get these from cbmem.h already?
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@56 PS2, Line 56: #define CURSOR_SZ sizeof(struct imd_cursor) Not sure why you have all of these rather than using sizeof() directly in the code? Not sure they really make things easier to follow...
You also use sizeof() for all these same things directly in the code in many places, please at least decide to do one or the other and then do that consistently.
https://review.coreboot.org/c/coreboot/+/44669/2/tests/lib/imd-test.c@69 PS2, Line 69: uint32_t align_up_pow2(uint32_t x) You can just use (1 << log2_ceil(x)) from coreboot's <lib.h>.