lib/libc/tests/string: add extended unit tests for strcmp()

[fuz]

Summary

This changeset add a new set of tests that comprehensively test strcmp() on
various alignments of the input. This made it easy to smoke out many
exciting new bugs in my SSE strcmp() implementation from D41971.

Sponsored by: The FreeBSD Foundation

Test Plan

n/a

[asomers]

Let me see if I understand this dlopen business correctly. Your plan is that if you create a separate .c file containing "test_strcmp" and link it to t_strcmp.o, then the test will operate on that function. Otherwise, it will operate on the standard strcmp?

[fuz]

In D41970#957091, @asomers wrote:

Let me see if I understand this dlopen business correctly. Your plan is that if you create a separate .c file containing "test_strcmp" and link it to t_strcmp.o, then the test will operate on that function. Otherwise, it will operate on the standard strcmp?

What I usually do is prepare my own strcmp to be named test_strcmp. Then I assemble and link it into a shared object and preload the shared object with LD_PRELOAD. It doesn't suffice to just name it strcmp, as defects in the code can mean that the test crashes before it gets to actually run any unit tests. This design pattern is already used in some other libc string tests such as contrib/netbsd-tests/lib/libc/string/t_strchr.c.

[ngie]

Please don't modify the NetBSD test: if the plan is to test out FreeBSD-specific behavior for strcmp, it should go in its own test.

[fuz]

Ok. Should I unhook the netbsd strcmp tests from the build and replace them with a FreeBSD-specific file then? What about the other open DRs (D41520, D41528)?

[ngie]

In D41970#957458, @fuz wrote:

Ok. Should I unhook the netbsd strcmp tests from the build and replace them with a FreeBSD-specific file then? What about the other open DRs (D41520, D41528)?

I'm a bit confused -- why would you want to unhook them from the build...? Why not just add the FreeBSD-specific tests to another test file?

[fuz]

So there will then be two test files for strcmp? If the build system supports this, sure, why not.

[fuz]

• tests: split strcmp_alignments into new test

This implements your requested move of the new strcmp unit test into a
FreeBSD-specific file.

[ngie]

All of my questions relate to understanding the rationale behind some of the design choices presented in the change.

contrib/netbsd-tests/lib/libc/string/t_strcmp.c

12–20

Please sort per style(9).

lib/libc/tests/string/strcmp_test.c

103 ↗

(On Diff #127845)

• What do 64, 16, and 3 represent? Why are those numbers important? What assumptions are you making about the hardware architecture -- does this work with 32-bit, 64-bit, will this work with 128-bit architectures, etc? It would be nice if these numbers were better explained in the code with comments and/or descriptive constant names.

105–106 ↗

(On Diff #127845)

Why '-'? Is this an arbitrary character choice?

110–114 ↗

(On Diff #127845)

Why doesn't this use nitems or sizeof and instead hardcoded values are used instead?

[fuz]

Will prepare a new revision pending response to my comments.

contrib/netbsd-tests/lib/libc/string/t_strcmp.c

12–20

Will do in the upcoming revision.

lib/libc/tests/string/strcmp_test.c

103 ↗

(On Diff #127845)

The 64 + 16 + 3 characters are needed to support 64 string lengths plus 16 possibly alignment offsets plus up to 3 sentinel characters behind the string. These values directly result from the code. I can add a comment with my next revision if you like.

105–106 ↗

(On Diff #127845)

More or less arbitrary. The code uses A and B as sentinels after the end of the string and X and Y to induce a mismatch. The fill character merely needs to have a character code lower than these two. A typical test case will look like strcmp("----X\0A", "-----\0B"); which is easy to see in a memory dump or in the debugger.

110–114 ↗

(On Diff #127845)

Because these values fall out of the properties of my strcmp routine I want to test. It uses SSE registers, which are 16 bytes in size. So there are 16 possible misalignments to consider. The code starts by processing up to 32 bytes in special “head” handling code. The main loop is then unrolled twice and thus processes 32 bytes per iteration. A 64 byte maximum length ensures that we test at least until the main loop completes once, catching all possibilities.

The sizeof operator cannot be used as none of these loops iterate by the size of some array.

For future implementations using wider registers, the values can be increased. I have taken this approach as it was also taken by various other unit tests. Please let me know if you would like the code to be more verbose.

[fuz]

I'm waiting for a response to my questions so I can prepare an update to this revision.

[ngie]

Ok, that part wasn't apparent before.

If you're willing to go through upstreaming the change to NetBSD and they accept it, I say it's worth taking -- if however upstream isn't interested in taking changes to the test, I don't see the point in modifying their test: I'd take the test, fork it, apply the necessary changes, then maintain it internally.

The goal is to consume the NetBSD test suite without having to hack it too much/do the bare minimum necessary to make it work on FreeBSD, while upstreaming anything possible back to the NetBSD project. It's not where FreeBSD specific tests/testing should go.

[ngie]

Also, please add comments for non-obvious things: you have the context now, but I didn't understand the reasoning, and you might not have the context at a later date. The code needs to either be intuitive by design or intuitive via comments.

[fuz]

Add comments to clarify parameter choices.
Also drop changes to the NetBSD bits. They are not required for
test suite coverage and prevent this DR from landing.

[ngie]