Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I hadn't thought to write to /dev/null to get the length. Good idea. But why do you check for !errno? The docs just say that if _vsnprintf fails it returns -1, it doesn't say anything about errno.
https://msdn.microsoft.com/en-us/library/1kt27hek.aspx
If buffer or format is NULL, or if count is less than or equal to zero,
these functions invoke the invalid parameter handler, as described in Parameter
Validation https://msdn.microsoft.com/en-us/library/ksazx244.aspx. If
execution is allowed to continue, these functions return -1 *and set *
*errno to EINVAL*.
Yea, I saw that, but it only says that it sets errno in the case that buffer==nullptr or count==0 (not sure why it says count<=0 since count is an unsigned type). So as far as I can tell the value of errno is undefined in the case that buffer, format, and count are all valid, but count is just not big enough (which is really the case we're trying to handle anyway).
Even in the case the MSDN doc talks about, you either won't return from the function at all (i.e. it will abort) or it will return -1. So I think it's best to remove the errno check. Otherwise lgtm.
So as far as I can tell the value of errno is undefined in the case that
buffer, format, and count are all valid, but count is just not big
enough (which is really the case we're trying to handle anyway).
Yeah, errno should be 0 only in the case that count is not big enough,
which is exactly what I'm checking for. (It's the caller's responsibility
to clear errno before calling snprintf). Otherwise, there's an error, and
it should be returned as is, and the fprintf wouldn't be necessary.
So as far as I can tell the value of errno is undefined in the case that
buffer, format, and count are all valid, but count is just not big
enough (which is really the case we're trying to handle anyway).
FWIW, I think it's not defined in the MSDN docs because it's already
defined in the C standard for all system/library calls.
Well, the whole reason we have to write this function is because Microsoft's implementation doesn't conform :) So I don't think we should assume they conform for anything else either.
In any case, I just tested it with this code:
int main(int argc, char* argv[])
{
char buffer[10];
errno = 0;
snprintf(buffer, 10, "This is a test of the snprintf function and buffer size");return 0;
}
When snprintf calls _vsnprintf, _vsnprintf returns -1 as expected, and sets errno to 34. So the check is definitely wrong currently.
Well, the whole reason we have to write this function is because
Microsoft's implementation doesn't conform :) So I don't think we should
assume they conform for anything else either.
https://memegen.googleplex.com/templateimage?template=burn
Should we just check for ERANGE (34) then? We need to save and restore
errno in that case to conform with the C standards. But I'm not sure about
using an undocumented behavior.
| source/Host/windows/Windows.cpp | ||
|---|---|---|
| 209 ↗ | (On Diff #26560) | I think there's still a problem here. _vsnprintf will not terminate if the intended content exactly fills count. My understanding on other platforms is that vsnprintf will always terminate. I'm reading between these lines from MSDN: "If there is room at the end (that is, if the number of characters to write is less than count), the buffer will be null-terminated." |
I don't think we should check for ERANGE because the documentation doesn't even say that it returns ERANGE. I think we should just check for -1 and ignore errno. If you want to save and restore errno you can do that by saving it to a temporary variable first, and restoring it if _vsnprintf returns >= 0.
I'm not sure if that will be 100% standards conforming, but it will at least work correctly :)
I still think there's a bug where the string can be unterminated.
Do you mean the case where the number of characters written (no terminating
'\0') is the same as count? buffer[count-1] = '\0' should handle that.
Oh, I missed that line. Yes, that's what I meant and it looks like this handles it. Thanks.