RFR: 8263136: C4530 was reported from VS 2019 at access bridge

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8263136: C4530 was reported from VS 2019 at access bridge

Yasumasa Suenaga-7
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.

-------------

Commit messages:
 - 8263136: C4530 was reported from VS 2019 at access bridge

Changes: https://git.openjdk.java.net/jdk/pull/2859/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2859&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263136
  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge

Phil Race
On Sun, 7 Mar 2021 03:18:53 GMT, Yasumasa Suenaga <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge

Thomas Stuefe
On Sun, 7 Mar 2021 16:19:15 GMT, Phil Race <[hidden email]> 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 <ostream> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge

Sergey Bylokhov-2
On Sun, 7 Mar 2021 19:34:36 GMT, Thomas Stuefe <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]

Yasumasa Suenaga-7
In reply to this post by Yasumasa Suenaga-7
> 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&pr=2859&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2859&range=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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge

Yasumasa Suenaga-7
In reply to this post by Sergey Bylokhov-2
On Sun, 7 Mar 2021 22:00:41 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Yes, including c++ standard library headers like <ostream> 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 `11644473600000ULL` 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge

Thomas Stuefe
In reply to this post by Sergey Bylokhov-2
On Sun, 7 Mar 2021 22:00:41 GMT, Sergey Bylokhov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]

Thomas Stuefe
In reply to this post by Yasumasa Suenaga-7
On Mon, 8 Mar 2021 00:56:24 GMT, Yasumasa Suenaga <[hidden email]> 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 / 10000ULL) - 11644473600000ULL; // 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 <windows.h>
> 34: #include <cstdlib>
> 35: #include <cstring>

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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]

Yasumasa Suenaga-7
On Mon, 8 Mar 2021 06:34:26 GMT, Thomas Stuefe <[hidden email]> 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 / 10000ULL) - 11644473600000ULL; // 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]

Yasumasa Suenaga-7
In reply to this post by Thomas Stuefe
On Mon, 8 Mar 2021 06:37:07 GMT, Thomas Stuefe <[hidden email]> 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 <windows.h>
>> 34: #include <cstdlib>
>> 35: #include <cstring>
>
> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]

Sergey Bylokhov-2
In reply to this post by Yasumasa Suenaga-7
On Mon, 8 Mar 2021 07:46:54 GMT, Yasumasa Suenaga <[hidden email]> 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 / 10000ULL) - 11644473600000ULL; // 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge [v2]

Yasumasa Suenaga-7
On Mon, 8 Mar 2021 19:30:17 GMT, Sergey Bylokhov <[hidden email]> 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