Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v3]
On Mon, 13 Mar 2023 15:55:27 GMT, Daniel Jeliński wrote:
>> This patch modifies the `getLastErrorString` method to return a `jstring`.
>> Thanks to that we can avoid unnecessary back and forth conversions between
>> Unicode and other charsets on Windows.
>>
>> Other changes include:
>> - the Windows implementation of `getLastErrorString` no longer checks
>> `errno`. I verified all uses of the method and confirmed that `errno` is not
>> used anywhere.
>> - While at it, I found and fixed a few calls to
>> `JNU_ThrowIOExceptionWithLastError` that were done in context where
>> `LastError` was not set.
>> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
>> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
>> have identical behavior.
>> - zip_util was modified to return static messages instead of generated ones.
>> The generated messages were not observed anywhere, because they were
>> replaced by a static message in ZIP_Open, which is the only method used by
>> other native code.
>> - `getLastErrorString` is no longer exported by libjava.
>>
>> Tier1-3 tests continue to pass.
>>
>> No new automated regression test; testing this requires installing a
>> language pack that cannot be displayed in the current code page.
>> Tested this manually by installing Chinese language pack on English Windows
>> 11, selecting Chinese language, then checking if the message on exception
>> thrown by `InetAddress.getByName("nonexistent.local");` starts with
>> `"不知道这样的主机。"` (or
>> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
>> change, the exception message started with a row of question marks.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use NULL where appropriate
we never use any of the JNU_XXX functions to report errno on Windows as far as
I could tell. And even if we did, we'd need to call SetLastError(0) before
JNU_Throw to make it work, which we never did. I think we're ok here.
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v3]
On Mon, 13 Mar 2023 15:55:27 GMT, Daniel Jeliński wrote:
>> This patch modifies the `getLastErrorString` method to return a `jstring`.
>> Thanks to that we can avoid unnecessary back and forth conversions between
>> Unicode and other charsets on Windows.
>>
>> Other changes include:
>> - the Windows implementation of `getLastErrorString` no longer checks
>> `errno`. I verified all uses of the method and confirmed that `errno` is not
>> used anywhere.
>> - While at it, I found and fixed a few calls to
>> `JNU_ThrowIOExceptionWithLastError` that were done in context where
>> `LastError` was not set.
>> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
>> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
>> have identical behavior.
>> - zip_util was modified to return static messages instead of generated ones.
>> The generated messages were not observed anywhere, because they were
>> replaced by a static message in ZIP_Open, which is the only method used by
>> other native code.
>> - `getLastErrorString` is no longer exported by libjava.
>>
>> Tier1-3 tests continue to pass.
>>
>> No new automated regression test; testing this requires installing a
>> language pack that cannot be displayed in the current code page.
>> Tested this manually by installing Chinese language pack on English Windows
>> 11, selecting Chinese language, then checking if the message on exception
>> thrown by `InetAddress.getByName("nonexistent.local");` starts with
>> `"不知道这样的主机。"` (or
>> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
>> change, the exception message started with a row of question marks.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use NULL where appropriate
The change in Windows behaviour seems like a worrying gotcha that someone using
the method might be trapped by (C library errors and system errors are reported
for Unix while only WINAPI is returned on Windows). Although this has already
been pushed I'd still like to mention that me and Thomas did have quite a bit
of discussion regarding the conundrum on Windows about whether to report both
when the error routines are called or have separate methods/mechanism to select
either, see [8292016](https://bugs.openjdk.org/browse/JDK-8292016?filter=-1)
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v3]
On Mon, 13 Mar 2023 15:55:27 GMT, Daniel Jeliński wrote:
>> This patch modifies the `getLastErrorString` method to return a `jstring`.
>> Thanks to that we can avoid unnecessary back and forth conversions between
>> Unicode and other charsets on Windows.
>>
>> Other changes include:
>> - the Windows implementation of `getLastErrorString` no longer checks
>> `errno`. I verified all uses of the method and confirmed that `errno` is not
>> used anywhere.
>> - While at it, I found and fixed a few calls to
>> `JNU_ThrowIOExceptionWithLastError` that were done in context where
>> `LastError` was not set.
>> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
>> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
>> have identical behavior.
>> - zip_util was modified to return static messages instead of generated ones.
>> The generated messages were not observed anywhere, because they were
>> replaced by a static message in ZIP_Open, which is the only method used by
>> other native code.
>> - `getLastErrorString` is no longer exported by libjava.
>>
>> Tier1-3 tests continue to pass.
>>
>> No new automated regression test; testing this requires installing a
>> language pack that cannot be displayed in the current code page.
>> Tested this manually by installing Chinese language pack on English Windows
>> 11, selecting Chinese language, then checking if the message on exception
>> thrown by `InetAddress.getByName("nonexistent.local");` starts with
>> `"不知道这样的主机。"` (or
>> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
>> change, the exception message started with a row of question marks.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use NULL where appropriate
Marked as reviewed by naoto (Reviewer).
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v3]
> This patch modifies the `getLastErrorString` method to return a `jstring`.
> Thanks to that we can avoid unnecessary back and forth conversions between
> Unicode and other charsets on Windows.
>
> Other changes include:
> - the Windows implementation of `getLastErrorString` no longer checks
> `errno`. I verified all uses of the method and confirmed that `errno` is not
> used anywhere.
> - While at it, I found and fixed a few calls to
> `JNU_ThrowIOExceptionWithLastError` that were done in context where
> `LastError` was not set.
> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
> have identical behavior.
> - zip_util was modified to return static messages instead of generated ones.
> The generated messages were not observed anywhere, because they were replaced
> by a static message in ZIP_Open, which is the only method used by other
> native code.
> - `getLastErrorString` is no longer exported by libjava.
>
> Tier1-3 tests continue to pass.
>
> No new automated regression test; testing this requires installing a language
> pack that cannot be displayed in the current code page.
> Tested this manually by installing Chinese language pack on English Windows
> 11, selecting Chinese language, then checking if the message on exception
> thrown by `InetAddress.getByName("nonexistent.local");` starts with
> `"不知道这样的主机。"` (or
> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
> change, the exception message started with a row of question marks.
Daniel Jeliński has updated the pull request incrementally with one additional
commit since the last revision:
Use NULL where appropriate
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12922/files
- new: https://git.openjdk.org/jdk/pull/12922/files/ea91b651..efd72a1d
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12922&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12922&range=01-02
Stats: 12 lines in 1 file changed: 0 ins; 0 del; 12 mod
Patch: https://git.openjdk.org/jdk/pull/12922.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12922/head:pull/12922
PR: https://git.openjdk.org/jdk/pull/12922
