Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]
On Mon, 8 Mar 2021 19:30:17 GMT, Sergey Bylokhov wrote: >> It seems to be fine not to use UNIX epoch at first glance as long as we can >> know the timing of events, but I'm not sure (Thus I rewrote to comply with >> the original code). So I want to hear from the others. > > Did you check via some a11y tool that the new code actually works? No, can you help? - PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]
On Mon, 8 Mar 2021 07:46:54 GMT, Yasumasa Suenaga wrote: >> src/jdk.accessibility/windows/native/common/AccessBridgeDebug.cpp line 80: >> >>> 78: uli.LowPart = ft.dwLowDateTime; >>> 79: uli.HighPart = ft.dwHighDateTime; >>> 80: return (uli.QuadPart / 1ULL) - 1164447360ULL; // Rebase >>> Epoch from 1601 to 1970 >> >> This is good and true to the original change; >> >> I am not even sure the epoch rebase is needed. All 8196681 did was to print >> out the timestamps verbatim. I do not know enough about how that debug trace >> is used, and if the timestamps really have to be 1970 based. > > It seems to be fine not to use UNIX epoch at first glance as long as we can > know the timing of events, but I'm not sure (Thus I rewrote to comply with > the original code). So I want to hear from the others. Did you check via some a11y tool that the new code actually works? - PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]
On Mon, 8 Mar 2021 06:37:07 GMT, Thomas Stuefe wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Refactoring > > src/jdk.accessibility/windows/native/common/AccessBridgeDebug.cpp line 35: > >> 33: #include >> 34: #include >> 35: #include > > Matter of taste, but I would prefer stdlib.h and string.h instead of cxxx. > Just to keep in line with the rest of the coding. Weird mix of styles > otherwise (I mean this code still uses 16bit era Windows APIs). AccessBridgeDebug has `.cpp` in its extension, so the compiler can handle it as C++ code, and also "cstring" seems to be prefer to "string.h" in @mrserb 's comment. It's nature to use "cstring" in C++ source code IMHO. Let's see comments from the others. - PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]
On Mon, 8 Mar 2021 06:34:26 GMT, Thomas Stuefe wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Refactoring > > src/jdk.accessibility/windows/native/common/AccessBridgeDebug.cpp line 80: > >> 78: uli.LowPart = ft.dwLowDateTime; >> 79: uli.HighPart = ft.dwHighDateTime; >> 80: return (uli.QuadPart / 1ULL) - 1164447360ULL; // Rebase >> Epoch from 1601 to 1970 > > This is good and true to the original change; > > I am not even sure the epoch rebase is needed. All 8196681 did was to print > out the timestamps verbatim. I do not know enough about how that debug trace > is used, and if the timestamps really have to be 1970 based. It seems to be fine not to use UNIX epoch at first glance as long as we can know the timing of events, but I'm not sure (Thus I rewrote to comply with the original code). So I want to hear from the others. - PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]
On Mon, 8 Mar 2021 00:56:24 GMT, Yasumasa Suenaga wrote: >> I saw C4530 with VS 2019 (16.9.0) as following (on Japanese locale): >> >> AccessBridgeDebug.cpp >> メモ: インクルード ファイル: >> d:\github-forked\jdk\src\jdk.accessibility\windows\native\common\AccessBridgeDebug.h >> >> : >> >> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): >> error C2220: 次の警 >> 告はエラーとして処理されます >> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): >> warning C4530: C++ >> 例外処理を使っていますが、アンワインド セマンティクスは有効にはなりません。/EHsc を指定してください。 >> メモ: インクルード ファイル: >> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\string >> >> `/EHsc` has been already passed in other makefiles, and also >> AccessBridgeDebug.cpp uses some STL classes (e.g. `chrono` namespace). So >> `/EHsc` is a solution for this problem. > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Refactoring Hi Yasumasa, small nits below. But this looks fine to me as it is already so I leave it up to you and the others whether to change anything. Thank you for taking my suggestion. Cheers, Thomas src/jdk.accessibility/windows/native/common/AccessBridgeDebug.cpp line 80: > 78: uli.LowPart = ft.dwLowDateTime; > 79: uli.HighPart = ft.dwHighDateTime; > 80: return (uli.QuadPart / 1ULL) - 1164447360ULL; // Rebase Epoch > from 1601 to 1970 This is good and true to the original change; I am not even sure the epoch rebase is needed. All 8196681 did was to print out the timestamps verbatim. I do not know enough about how that debug trace is used, and if the timestamps really have to be 1970 based. src/jdk.accessibility/windows/native/common/AccessBridgeDebug.cpp line 35: > 33: #include > 34: #include > 35: #include Matter of taste, but I would prefer stdlib.h and string.h instead of cxxx. Just to keep in line with the rest of the coding. Weird mix of styles otherwise (I mean this code still uses 16bit era Windows APIs). - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge
On Sun, 7 Mar 2021 22:00:41 GMT, Sergey Bylokhov wrote: > > I wondered why C++ std headers are even used. The source code looks C-ish; > > but "8196681: Java Access Bridge logging and debug flags dynamically > > controlled" added some coding, adding a bunch of C++11x semantics and > > included C++ std headers. Using "/Ehsc" had even been discussed: > > https://mail.openjdk.java.net/pipermail/awt-dev/2018-December/014847.html > > but not done. > > After that discussion the usage of "string.h"/etc was dropped and "C string > manipulation API" was used instead, and that suppressed the warning. I do not > know why it was not complaining about "getTimeStamp". I think the fix should > refactor the code again. Ah, I missed that part. Thanks for clarifying the history :) - PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge
On Sun, 7 Mar 2021 22:00:41 GMT, Sergey Bylokhov wrote: >> Yes, including c++ standard library headers like means you need to >> deal with C++ exceptions thrown from library functions, and the code needs >> to be compiled with unwind capabilities. If its not switched on, and a C++ >> exception happens, the behavior is undefined. In my experience it results in >> the process being terminated. >> >> I wondered why C++ std headers are even used. The source code looks C-ish; >> but "8196681: Java Access Bridge logging and debug flags dynamically >> controlled" added some coding, adding a bunch of C++11x semantics and >> included C++ std headers. Using "/Ehsc" had even been discussed: >> https://mail.openjdk.java.net/pipermail/awt-dev/2018-December/014847.html >> but not done. >> >> Adding /Ehsc is fine of course, but I think thats not enough. Now C++ >> exceptions can pass through the code, but if they do, then what? E.g. this >> function `getTimeStamp()` added with 8196681: >> >> https://github.com/openjdk/jdk/blob/113b5184cfc78fde057a7fe4d5872b463930da00/src/jdk.accessibility/windows/native/common/AccessBridgeDebug.cpp#L85 >> >> calls into the C++ standard library to get a time stamp. Can't these >> function not throw C++ exceptions? Is that not what the compiler is warning >> about? If they throw, exceptions propagate through the code unbounded, until >> they either end the process or cause havoc at the next upper >> C-only-interface. >> >> Or, maybe, rewrite this coding to use standard C- and Windows-APIs. I think >> 8196681 could had been done with traditional windows- or standard C APIs. In >> particular, `getTimeStamp()` could probably be done simply with >> `GetTickCount` or `GetSystemTime` or functions from time.h. >> >> Cheers, Thomas > >> I wondered why C++ std headers are even used. The source code looks C-ish; >> but "8196681: Java Access Bridge logging and debug flags dynamically >> controlled" added some coding, adding a bunch of C++11x semantics and >> included C++ std headers. Using "/Ehsc" had even been discussed: >> https://mail.openjdk.java.net/pipermail/awt-dev/2018-December/014847.html >> but not done. > > After that discussion the usage of "string.h"/etc was dropped and "C string > manipulation API" was used instead, and that suppressed the warning. I do not > know why it was not complaining about "getTimeStamp". I think the fix should > refactor the code again. Thank you for comments! I refactored `getTimeStamp()` not to use STL functions in new commit. The error messages have gone without /EHsc. I use `1164447360ULL` to rebase Epoch from 1601 to 1970, it comes from ProcessHandleImpl_win.c: https://github.com/openjdk/jdk/blob/22a3117d229cba10c690a4e66baf9c754a09e57c/src/java.base/windows/native/libjava/ProcessHandleImpl_win.c#L342 - PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]
> I saw C4530 with VS 2019 (16.9.0) as following (on Japanese locale): > > AccessBridgeDebug.cpp > メモ: インクルード ファイル: > d:\github-forked\jdk\src\jdk.accessibility\windows\native\common\AccessBridgeDebug.h > > : > > c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): > error C2220: 次の警 > 告はエラーとして処理されます > c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): > warning C4530: C++ > 例外処理を使っていますが、アンワインド セマンティクスは有効にはなりません。/EHsc を指定してください。 > メモ: インクルード ファイル: > c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\string > > `/EHsc` has been already passed in other makefiles, and also > AccessBridgeDebug.cpp uses some STL classes (e.g. `chrono` namespace). So > `/EHsc` is a solution for this problem. Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision: Refactoring - Changes: - all: https://git.openjdk.java.net/jdk/pull/2859/files - new: https://git.openjdk.java.net/jdk/pull/2859/files/f68a1281..4a357eb9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2859=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2859=00-01 Stats: 12 lines in 2 files changed: 2 ins; 1 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/2859.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2859/head:pull/2859 PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge
On Sun, 7 Mar 2021 19:34:36 GMT, Thomas Stuefe wrote: > I wondered why C++ std headers are even used. The source code looks C-ish; > but "8196681: Java Access Bridge logging and debug flags dynamically > controlled" added some coding, adding a bunch of C++11x semantics and > included C++ std headers. Using "/Ehsc" had even been discussed: > https://mail.openjdk.java.net/pipermail/awt-dev/2018-December/014847.html but > not done. After that discussion the usage of "string.h"/etc was dropped and "C string manipulation API" was used instead, and that suppressed the warning. I do not know why it was not complaining about "getTimeStamp". I think the fix should refactor the code again. - PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge
On Sun, 7 Mar 2021 16:19:15 GMT, Phil Race wrote: >> I saw C4530 with VS 2019 (16.9.0) as following (on Japanese locale): >> >> AccessBridgeDebug.cpp >> メモ: インクルード ファイル: >> d:\github-forked\jdk\src\jdk.accessibility\windows\native\common\AccessBridgeDebug.h >> >> : >> >> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): >> error C2220: 次の警 >> 告はエラーとして処理されます >> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): >> warning C4530: C++ >> 例外処理を使っていますが、アンワインド セマンティクスは有効にはなりません。/EHsc を指定してください。 >> メモ: インクルード ファイル: >> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\string >> >> `/EHsc` has been already passed in other makefiles, and also >> AccessBridgeDebug.cpp uses some STL classes (e.g. `chrono` namespace). So >> `/EHsc` is a solution for this problem. > > translate.google.com says the error in (almost) English is : > c: \ program ~ 2 \ micros ~ 2 \ 2019 \ commun ~ 1 \ vc \ tools \ msvc \ 1428 > ~ 1.299 \ include \ ostream (611): warning C4530: C ++ > I'm using exception handling, but unwind semantics aren't enabled. Please > specify / EHsc. Yes, including c++ standard library headers like means you need to deal with C++ exceptions thrown from library functions, and the code needs to be compiled with unwind capabilities. If its not switched on, and a C++ exception happens, the behavior is undefined. In my experience it results in the process being terminated. I wondered why C++ std headers are even used. The source code looks C-ish; but "8196681: Java Access Bridge logging and debug flags dynamically controlled" added some coding, adding a bunch of C++11x semantics and included C++ std headers. Using "/Ehsc" had even been discussed: https://mail.openjdk.java.net/pipermail/awt-dev/2018-December/014847.html but not done. Adding /Ehsc is fine of course, but I think thats not enough. Now C++ exceptions can pass through the code, but if they do, then what? E.g. this function `getTimeStamp()` added with 8196681: https://github.com/openjdk/jdk/blob/113b5184cfc78fde057a7fe4d5872b463930da00/src/jdk.accessibility/windows/native/common/AccessBridgeDebug.cpp#L85 calls into the C++ standard library to get a time stamp. Can't these function not throw C++ exceptions? Is that not what the compiler is warning about? If they throw, exceptions propagate through the code unbounded, until they either end the process or cause havoc at the next upper C-only-interface. Or, maybe, rewrite this coding to use standard C- and Windows-APIs. I think 8196681 could had been done with traditional windows- or standard C APIs. In particular, `getTimeStamp()` could probably be done simply with `GetTickCount` or `GetSystemTime` or functions from time.h. Cheers, Thomas - PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge
On Sun, 7 Mar 2021 03:18:53 GMT, Yasumasa Suenaga wrote: > I saw C4530 with VS 2019 (16.9.0) as following (on Japanese locale): > > AccessBridgeDebug.cpp > メモ: インクルード ファイル: > d:\github-forked\jdk\src\jdk.accessibility\windows\native\common\AccessBridgeDebug.h > > : > > c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): > error C2220: 次の警 > 告はエラーとして処理されます > c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): > warning C4530: C++ > 例外処理を使っていますが、アンワインド セマンティクスは有効にはなりません。/EHsc を指定してください。 > メモ: インクルード ファイル: > c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\string > > `/EHsc` has been already passed in other makefiles, and also > AccessBridgeDebug.cpp uses some STL classes (e.g. `chrono` namespace). So > `/EHsc` is a solution for this problem. translate.google.com says the error in (almost) English is : c: \ program ~ 2 \ micros ~ 2 \ 2019 \ commun ~ 1 \ vc \ tools \ msvc \ 1428 ~ 1.299 \ include \ ostream (611): warning C4530: C ++ I'm using exception handling, but unwind semantics aren't enabled. Please specify / EHsc. - PR: https://git.openjdk.java.net/jdk/pull/2859
