On Tue, 12 Aug 2025 16:04:06 GMT, Saint Wesonga <d...@openjdk.org> wrote:
>> So, we run gtests on Windows with `--gtest_catch_exceptions=0` which >> disables the inbuilt exception handler. >> >> See issues >> - https://bugs.openjdk.org/browse/JDK-8185734 >> - https://bugs.openjdk.org/browse/JDK-8267138 (fixed an oversight in 8185734) >> >> See more details in my [PR description for >> 8185734](https://github.com/openjdk/jdk/pull/1757#issue-765285610), that's >> why this all sounded so familiar. >> >> The mechanism in place since 8267138 should be enough for my understanding. >> @swesonga, can you find out why this is not sufficient in your case? Is this >> option not passed on to your test? >> >>> That led me to wonder why, on Windows, we build libgtest and rebuild libjvm >>> with exceptions enabled, by using -EHsc instead of no -EH option as done >>> for the non-gtest libjvm? >> >> In the libjvm code itself, we don't use C++ exceptions, and we catch all >> Windows SEH with `__try`/`__except`. We also don't want standard stack >> unwinding with these signals. So no need for `/EHxx`. My assumption is that >> the gtest framework itself uses C++ exceptions and therefore needs `/EHsc`. >> The documentation is not super clear on some aspects of this >> (https://learn.microsoft.com/en-us/cpp/build/reference/eh-exception-handling-model), >> but it says that for standard C++ exceptions to work `/EHsc` is needed. >> >>> I was concerned that this might effectively undo JDK-8185734, since the >>> suggestion in JBS was to conditionalize some code on GTEST_HAS_SEH being >>> true. But it looks like the actual change didn't do that. >> >> In my PR for 8184734, I wrote: >> >> " In JBS, @kimbarrett suggested to build gtests with GTEST_HAS_SEH switched >> off to prevent gtest from using SEH. Unfortunately that won't work since the >> use of death tests means we need SEH. If we switch GTEST_HAS_SEH off, the >> death tests don't build. I also do not like this suggestion since this >> configuration may have a higher chance of bitrotting upstream." >> >> So it looks this was the first thing I tried back then, and it failed. > >> So, we run gtests on Windows with `--gtest_catch_exceptions=0` which >> disables the inbuilt exception handler. >> >> See issues >> >> * https://bugs.openjdk.org/browse/JDK-8185734 >> * https://bugs.openjdk.org/browse/JDK-8267138 (fixed an oversight in 8185734) >> >> See more details in my [PR description for >> 8185734](https://github.com/openjdk/jdk/pull/1757#issue-765285610), that's >> why this all sounded so familiar. >> >> The mechanism in place since 8267138 should be enough for my understanding. >> @swesonga, can you find out why this is not sufficient in your case? Is this >> option not passed on to your test? >> >> > That led me to wonder why, on Windows, we build libgtest and rebuild >> > libjvm with exceptions enabled, by using -EHsc instead of no -EH option as >> > done for the non-gtest libjvm? >> >> In the libjvm code itself, we don't use C++ exceptions, and we catch all >> Windows SEH with `__try`/`__except`. We also don't want standard stack >> unwinding with these signals. So no need for `/EHxx`. My assumption is that >> the gtest framework itself uses C++ exceptions and therefore needs `/EHsc`. >> The documentation is not super clear on some aspects of this >> (https://learn.microsoft.com/en-us/cpp/build/reference/eh-exception-handling-model), >> but it says that for standard C++ exceptions to work `/EHsc` is needed. >> >> > I was concerned that this might effectively undo JDK-8185734, since the >> > suggestion in JBS was to conditionalize some code on GTEST_HAS_SEH being >> > true. But it looks like the actual change didn't do that. >> >> In my PR for 8184734, I wrote: >> >> " In JBS, @kimbarrett suggested to build gtests with GTEST_HAS_SEH switched >> off to prevent gtest from using SEH. Unfortunately that won't work since the >> use of death tests means we need SEH. If we switch GTEST_HAS_SEH off, the >> death tests don't build. I also do not like this suggestion since this >> configuration may have a higher chance of bitrotting upstream." >> >> So it looks this was the first thing I tried back then, and it failed. > > Thank you for this detailed background and explanation. Turns out we have 2 > separate gtest runs happening, one that matches the GitHub actions test you > pointed out and another that doesn't have the `--gtest_catch_exceptions=0` > flag. I was focused on the latter scenario but based on your explanation, the > correct resolution is for us to not run gtests without that flag. I will > close this pull request and update our test infrastructure to remove that > scenario. @swesonga Thank you for the update! Good to see that we have understood everything. I admit having to control this externally instead of having this knowledge baked into the launcher at build time is less satisfactory. What would be helpful would be if we could have a warning in the test when running on Windows. Edit: Also, good analysis, sorry that it did not result in a patch. ------------- PR Comment: https://git.openjdk.org/jdk/pull/26661#issuecomment-3183089466
