On 08/21/2018 10:46 AM, Martin Sebor wrote: > On 08/21/2018 09:56 AM, Jeff Law wrote: >> On 08/21/2018 09:37 AM, Martin Sebor wrote: >>> On 08/21/2018 05:50 AM, Joseph Myers wrote: >>>> On Tue, 21 Aug 2018, Martin Sebor wrote: >>>> >>>>> I still believe it would be simpler, more robust, and safe >>>>> to pass in a flag to either count bytes (the default, for >>>>> all callers except sprintf %ls), or elements of the string >>>>> type (for sprintf %ls). >>>> >>>> But the correct thing to count for sprintf %ls is elements of the type >>>> expected by sprintf %ls. That may not be the type of the string >>>> constant >>>> (and as this is in variable arguments, there isn't any implicit >>>> conversion >>>> to const wchar_t * like you would get with a function prototype - >>>> I'm not >>>> sure if anything ensures this code is only reached, regardless of >>>> language, if the argument does actually (after any casts present in the >>>> source code) point to the correct wchar_t type). >>> >>> Sure, but the only valid argument to %ls is wchar_t*. Passing >>> it something else is undefined. >>> >>> The only caller interested in the element count is %ls and >>> the only case where the function could return a result that >>> might be different from that of C wcslen() with the same >>> (invalid) argument is the undefined: >>> >>> snprintf (0, 0, "%ls", "1234"); >>> >>> In this case, c_strlen() would return 4 which GCC's snprintf >>> turns into [0, 24], and calls the libc snprintf which will >>> then proceed to read past the end of the narrow string and >>> return some bogus value (if it doesn't crash). >>> >>> So I don't see a problem here(*). >> I think we should be counting wchar_ts in this case. If someone passes >> something other than a wchar_t as the argument for a %ls, then the code >> is in error. > > Sure, counting wchar_t's would be fine too. But it doesn't > do that either -- it fails. It fails when it encounters something unexpected (specifically when the size of the objects being counted is not the same as the requested count_by. We've discussed that chunk of code and there's arguments to keep the sanity check as well as arguments to drop the sanity check.
This can be addressed incrementally, IMHO it is not a fundamental flaw in the patch. How we choose to address it, either by dropping the test, using a different function, flags, whatever we decide as engineers is the best way to enhance the code so that we can get the diagnostics we want. But I think this should defer until after we figure out a plan for the 86711/86714 codegen issues. I was hoping to be further on that today, but when half the day is lost to meetings, it's tough to get much else done. Jeff
