On Sun, 13 Nov 2022 22:55:52 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use >> `os::snprintf`. >> >> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918. >> Regarding `os::snprintf` and `os::vsnprintf`, see >> https://bugs.openjdk.org/browse/JDK-8285506. >> >> I think the only reason we haven't marked `::sprintf` and `::snprintf` >> forbidden >> (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has gotten around >> to dealing with it. `::snprintf` in the list of candidates for >> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been >> marked. But I don't see new bugs for the as-yet unmarked ones. >> >> As a general note, as a reviewer my preference is against non-trivial and >> persnickety code changes that are scattered all over the code base. For >> something like this I'd prefer multiple more bite-sized changes that were >> dealing with specific uses. I doubt everyone agrees with me though. > >> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use >> `os::snprintf`. > > Updated to use os::snprintf, except the files under adlc where the > os::snptintf definition is not included. The use of snprintf could be > cleaned up with existing code in the future. > >> >> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918. >> Regarding `os::snprintf` and `os::vsnprintf`, see >> https://bugs.openjdk.org/browse/JDK-8285506. >> >> I think the only reason we haven't marked `::sprintf` and `::snprintf` >> forbidden (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has >> gotten around to dealing with it. `::snprintf` in the list of candidates for >> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been >> marked. But I don't see new bugs for the as-yet unmarked ones. >> >> As a general note, as a reviewer my preference is against non-trivial and >> persnickety code changes that are scattered all over the code base. For >> something like this I'd prefer multiple more bite-sized changes that were >> dealing with specific uses. I doubt everyone agrees with me though. > > It makes sense to me. I'd better focus on the building issue in this PR. > > Thank you for the review! Hi @XueleiFan and @kimbarrett , I agree to the change if we, as Kim suggested, add assertions for truncation where we use the return value of snprintf. I am not fully happy with the solution though, since printing is notoriously runtime-data dependent and runtime data can change in release builds. So we could have truncation at a customer that we never see in our tests with debug builds. But seeing that this patch takes so long now and blocks the MacOS build, I don't want to block it. We can improve the code in follow up RFEs. These places would be a lot simpler with stringStream. Cheers, Thomas ------------- PR: https://git.openjdk.org/jdk/pull/11115
