Skip to content

Conversation

@stefano-zanotti-88
Copy link
Contributor

Adds checks about the length of the produced string, and the size reported as the return value.
Also checks it against the return value of the sys printf.

This is just an idea. I haven't actually compiled the NPF tests (usual problems).
It should also be extended to other test files.

Adding these checks caught some issues. I've run those checks on (a replica of) conformance.cc only, not on the test cases in paland.cc nor the other test suites.
Issues:
require_conform(output, "%c", i);: prints "", returns 1
require_conform("", "%+c", 0);: prints "", returns 1
require_conform(buf, "%p", p);: due to implementation-defined behavior; might be fixed in your latest commits (about optional "0x" in %p), but we should check
require_conform("0.00390625", "%.8Lf", (long double)0.00390625);: my own sys printf prints "0.00000000". Maybe something to look out for when comparing npf to sys.
require_conform("-0.00390625", "%.8Lf", (long double)-0.00390625);: my own sys printf prints "0.00000000". Maybe something to look out for when comparing npf to sys

Note that printing '\0' is UB.
I've seen this behavior:

printf("a%cx", '\0'); // NPF: "ax", returns 3
printf("a%cx", '\0'); // GCC/clang: "ax", returns 3
printf("a%cx", '\0'); // other: "a", returns 3 (presumably actually writes "a\0x" in the buffer)

A test like printf("a%cx", '\0') should be added to our tests -- I'm thinking of a new file gathering all UB/IB, maybe the nan test cases could be moved there as well.

@charlesnicholson
Copy link
Owner

Thanks, this is a very good insight and I agree that it should be added to the test harness and a non-stupid solution should be applied (clearly npf is screwing up by returning 3). How would you like to proceed on this PR, given the conversation about testing in #132 ?

(Additionally, I agree that your extremely thorough listing of all the UB issues should be extracted into an explicit UB test module and pulled out of the conformance stuff; how do you see this working with that?)

Maybe we should convert this PR to an issue instead to track it there?

@stefano-zanotti-88
Copy link
Contributor Author

There are tests for UB in #298. However, they are specifically to check that the "safety" option catches them correctly.
If that option is not used, NPF does something (crashing, or producing IB results), which is very much worth testing as well.
I'd like to have one single file for testing all standard-conforming behavior, and another file for IB behavior (plus all the other files like the various flavors of ftoa). Maybe paland should be removed, ie its tests split between the standard-conforming tests, and the IB tests.
And, the "vs sys printf" tests could be done in yet another file -- at most, the standard-conformance tests could be done (in the very same test case) both against an oracle and against the system printf.

About the problematic behavior of NPF:
%c with argument '\0' should just ignore that character. Emitting a '\0' mid-sequence and then emitting other stuff is just asking for trouble. It's true that the callback version could handle that '\0' without problems, but this would introduce an inconsistency between the callback and the sprintf versions that I'm sure is no good at all.
So, printf("a%cb", '\0') should emit "ab" and return 2

About %p, we should not test against sys-printf at all. And we should either:

  • test with (void*)some_integer_constant, disregarding the fact that it might be technically UB to do this cast (but printf does nothing with it except cast it to an integer and print it, no actual pointer stuff involved)
  • or, use real pointers, and test the pointer against a scanf of the produced string.
    The second one might be "cleaner", but it's also more complex, and doesn't let us check for corner cases at all. Also, it makes tests completely irreproducible on desktop OSes, due to ASLR.

Feel free to organize things as you see most fit, ie open an issue about this, or else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants