Re: Ping: Re: [PATCH] libgcc: fix SEH C++ rethrow semantics [PR113337]
Il 31/01/24 04:24, LIU Hao ha scritto: 在 2024-01-31 08:08, Jonathan Yong 写道: On 1/24/24 15:17, Matteo Italia wrote: Ping! That's a one-line fix, and you can find all the details in the bugzilla entry. Also, I can provide executables built with the affected toolchains, demonstrating the problem and the fix. Thanks, Matteo I was away last week. LH, care to comment? Changes look fine to me. The change looks good to me, too. I haven't tested it though. According to a similar construction around 'libgcc/unwind.inc:265' it should be that way. Hello, thank you for the replies, is there anything else I can do to help push this forward?
Re: Ping: Re: [PATCH] libgcc: fix SEH C++ rethrow semantics [PR113337]
Il 06/02/24 10:17, Jonathan Yong ha scritto: On 2/6/24 05:31, NightStrike wrote: On Mon, Feb 5, 2024, 06:53 Matteo Italia wrote: Il 31/01/24 04:24, LIU Hao ha scritto: 在 2024-01-31 08:08, Jonathan Yong 写道: On 1/24/24 15:17, Matteo Italia wrote: Ping! That's a one-line fix, and you can find all the details in the bugzilla entry. Also, I can provide executables built with the affected toolchains, demonstrating the problem and the fix. Thanks, Matteo I was away last week. LH, care to comment? Changes look fine to me. The change looks good to me, too. I haven't tested it though. According to a similar construction around 'libgcc/unwind.inc:265' it should be that way. Hello, thank you for the replies, is there anything else I can do to help push this forward? Remember to mention the pr with the right syntax in the ChangeLog so the bot adds a comment field. I didn't see it in yours, but I might have missed it. Thanks all, pushed to master branch. Thanks all :-) do you think this warrants backports? On one hand this is a pretty niche feature, and I am probably the first to notice the problem in ~12 years since that code was written, OTOH Win64/SEH was not super widespread for a long time, and seems like a safe enough change. Also: should I explicitly mark PR113337 as resolved? The bot added the reference to the commit, but the PR is still marked as "UNCONFIRMED".
[PATCH] libgcc: fix Win32 CV abnormal spurious wakeups in timed wait [PR113850]
The Win32 threading model uses __gthr_win32_abs_to_rel_time to convert
the timespec used in gthreads to specify the absolute time for end of
the condition variables timed wait to a milliseconds value relative to
"now" to pass to the Win32 SleepConditionVariableCS function.
Unfortunately, the conversion is incorrect, as, due to a typo, it
returns the relative time _in seconds_, so SleepConditionVariableCS
receives a timeout value 1000 times shorter than it should be, resulting
in a huge amount of spurious wakeups in calls such as
std::condition_variable::wait_for or wait_until.
This can be demonstrated easily by this sample program:
```
int main() {
std::condition_variable cv;
std::mutex mx;
bool pass = false;
auto thread_fn = [&](bool timed) {
int wakeups = 0;
using sc = std::chrono::system_clock;
auto before = sc::now();
std::unique_lock ml(mx);
if (timed) {
cv.wait_for(ml, std::chrono::seconds(2), [&]{
++wakeups;
return pass;
});
} else {
cv.wait(ml, [&]{
++wakeups;
return pass;
});
}
printf("pass: %d; wakeups: %d; elapsed: %d ms\n", pass, wakeups,
int((sc::now() - before) / std::chrono::milliseconds(1)));
pass = false;
};
{
// timed wait, let expire
std::thread t(thread_fn, true);
t.join();
}
{
// timed wait, wake up explicitly after 1 second
std::thread t(thread_fn, true);
std::this_thread::sleep_for(std::chrono::seconds(1));
{
std::unique_lock ml(mx);
pass = true;
}
cv.notify_all();
t.join();
}
{
// non-timed wait, wake up explicitly after 1 second
std::thread t(thread_fn, false);
std::this_thread::sleep_for(std::chrono::seconds(1));
{
std::unique_lock ml(mx);
pass = true;
}
cv.notify_all();
t.join();
}
return 0;
}
```
On builds that other threading models (e.g. POSIX on Linux, or
winpthreads or MCF on Win32) the output is something like
```
pass: 0; wakeups: 2; elapsed: 2000 ms
pass: 1; wakeups: 2; elapsed: 991 ms
pass: 1; wakeups: 2; elapsed: 996 ms
```
while on the affected builds it's more like
```
pass: 0; wakeups: 1418; elapsed: 2000 ms
pass: 1; wakeups: 479; elapsed: 988 ms
pass: 1; wakeups: 2; elapsed: 992 ms
```
(notice the huge number of wakeups).
This commit fixes the conversion, adjusting the final division by
NSEC100_PER_SEC to use NSEC100_PER_MSEC instead (already defined in the
file and not used in any other place, so the problem here was probably a
typo or some rebase/refactoring mishap).
libgcc/ChangeLog:
* config/i386/gthr-win32-cond.c (__gthr_win32_abs_to_rel_time):
fix absolute timespec to relative milliseconds count
conversion (it incorrectly returned seconds instead of
milliseconds); this avoids spurious wakeups in
__gthr_win32_cond_timedwait
---
libgcc/config/i386/gthr-win32-cond.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libgcc/config/i386/gthr-win32-cond.c
b/libgcc/config/i386/gthr-win32-cond.c
index ecb007a54b2..650c448f286 100644
--- a/libgcc/config/i386/gthr-win32-cond.c
+++ b/libgcc/config/i386/gthr-win32-cond.c
@@ -78,7 +78,7 @@ __gthr_win32_abs_to_rel_time (const __gthread_time_t
*abs_time)
if (abs_time_nsec100 < now.nsec100)
return 0;
- return (DWORD) CEIL_DIV (abs_time_nsec100 - now.nsec100, NSEC100_PER_SEC);
+ return (DWORD) CEIL_DIV (abs_time_nsec100 - now.nsec100, NSEC100_PER_MSEC);
}
/* Check the sizes of the local version of the Win32 types. */
--
2.34.1
Re: [PATCH] libgcc: fix Win32 CV abnormal spurious wakeups in timed wait [PR113850]
Il 09/02/24 15:18, Matteo Italia ha scritto:
The Win32 threading model uses __gthr_win32_abs_to_rel_time to convert
the timespec used in gthreads to specify the absolute time for end of
the condition variables timed wait to a milliseconds value relative to
"now" to pass to the Win32 SleepConditionVariableCS function.
Unfortunately, the conversion is incorrect, as, due to a typo, it
returns the relative time _in seconds_, so SleepConditionVariableCS
receives a timeout value 1000 times shorter than it should be, resulting
in a huge amount of spurious wakeups in calls such as
std::condition_variable::wait_for or wait_until.
This can be demonstrated easily by this sample program:
```
int main() {
std::condition_variable cv;
std::mutex mx;
bool pass = false;
auto thread_fn = [&](bool timed) {
int wakeups = 0;
using sc = std::chrono::system_clock;
auto before = sc::now();
std::unique_lock ml(mx);
if (timed) {
cv.wait_for(ml, std::chrono::seconds(2), [&]{
++wakeups;
return pass;
});
} else {
cv.wait(ml, [&]{
++wakeups;
return pass;
});
}
printf("pass: %d; wakeups: %d; elapsed: %d ms\n", pass, wakeups,
int((sc::now() - before) / std::chrono::milliseconds(1)));
pass = false;
};
{
// timed wait, let expire
std::thread t(thread_fn, true);
t.join();
}
{
// timed wait, wake up explicitly after 1 second
std::thread t(thread_fn, true);
std::this_thread::sleep_for(std::chrono::seconds(1));
{
std::unique_lock ml(mx);
pass = true;
}
cv.notify_all();
t.join();
}
{
// non-timed wait, wake up explicitly after 1 second
std::thread t(thread_fn, false);
std::this_thread::sleep_for(std::chrono::seconds(1));
{
std::unique_lock ml(mx);
pass = true;
}
cv.notify_all();
t.join();
}
return 0;
}
```
On builds that other threading models (e.g. POSIX on Linux, or
winpthreads or MCF on Win32) the output is something like
```
pass: 0; wakeups: 2; elapsed: 2000 ms
pass: 1; wakeups: 2; elapsed: 991 ms
pass: 1; wakeups: 2; elapsed: 996 ms
```
while on the affected builds it's more like
```
pass: 0; wakeups: 1418; elapsed: 2000 ms
pass: 1; wakeups: 479; elapsed: 988 ms
pass: 1; wakeups: 2; elapsed: 992 ms
```
(notice the huge number of wakeups).
This commit fixes the conversion, adjusting the final division by
NSEC100_PER_SEC to use NSEC100_PER_MSEC instead (already defined in the
file and not used in any other place, so the problem here was probably a
typo or some rebase/refactoring mishap).
libgcc/ChangeLog:
* config/i386/gthr-win32-cond.c (__gthr_win32_abs_to_rel_time):
fix absolute timespec to relative milliseconds count
conversion (it incorrectly returned seconds instead of
milliseconds); this avoids spurious wakeups in
__gthr_win32_cond_timedwait
---
libgcc/config/i386/gthr-win32-cond.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libgcc/config/i386/gthr-win32-cond.c
b/libgcc/config/i386/gthr-win32-cond.c
index ecb007a54b2..650c448f286 100644
--- a/libgcc/config/i386/gthr-win32-cond.c
+++ b/libgcc/config/i386/gthr-win32-cond.c
@@ -78,7 +78,7 @@ __gthr_win32_abs_to_rel_time (const __gthread_time_t
*abs_time)
if (abs_time_nsec100 < now.nsec100)
return 0;
- return (DWORD) CEIL_DIV (abs_time_nsec100 - now.nsec100, NSEC100_PER_SEC);
+ return (DWORD) CEIL_DIV (abs_time_nsec100 - now.nsec100, NSEC100_PER_MSEC);
}
/* Check the sizes of the local version of the Win32 types. */
Re-reading the commit message I found a few typos, and it was generally
a bit more obscure than I like; reworded it now, hope it's better.From 1592691385f6639b64bd6ec43bdbd8175628fa12 Mon Sep 17 00:00:00 2001
From: Matteo Italia
Date: Fri, 9 Feb 2024 15:04:20 +0100
Subject: [PATCH] libgcc: fix Win32 CV abnormal spurious wakeups in timed wait
[PR113850]
To: gcc-patches@gcc.gnu.org
Cc: 10wa...@gmail.com,
ebotca...@adacore.com,
mat...@mitalia.net
Fix a typo in __gthr_win32_abs_to_rel_time that caused it to return a
relative time in seconds instead of milliseconds. As a consequence,
__gthr_win32_cond_timedwait called SleepConditionVariableCS with a
1000x shorter timeout; this caused ~1000x more spurious wakeups in
CV timed waits such as std::condition_variable::wait_for or wait_until,
resulting generally in much higher CPU usage.
This can be demonstrated by this sample program:
```
#include
#include
#include
#include
int main() {
std::condition_variable cv;
std::mutex mx;
bool pass = false;
auto thread_f
Re: [PATCH] libgcc: fix Win32 CV abnormal spurious wakeups in timed wait [PR113850]
Il 17/02/24 01:24, Jonathan Yong ha scritto: On 2/10/24 10:10, Matteo Italia wrote: Il 09/02/24 15:18, Matteo Italia ha scritto: The Win32 threading model uses __gthr_win32_abs_to_rel_time to convert the timespec used in gthreads to specify the absolute time for end of the condition variables timed wait to a milliseconds value relative to "now" to pass to the Win32 SleepConditionVariableCS function. Unfortunately, the conversion is incorrect, as, due to a typo, it returns the relative time _in seconds_, so SleepConditionVariableCS receives a timeout value 1000 times shorter than it should be, resulting in a huge amount of spurious wakeups in calls such as std::condition_variable::wait_for or wait_until. Re-reading the commit message I found a few typos, and it was generally a bit more obscure than I like; reworded it now, hope it's better. Thanks, pushed to master and 13.x branches. Great, thank you! Do I need to change the status of the Bugzilla entry to RESOLVED, or it's going to be closed automatically at the next releases, or something else?
Re: Ping: Re: [PATCH] libgcc: fix SEH C++ rethrow semantics [PR113337]
Il 26/02/24 02:41, NightStrike ha scritto: It's mostly up to you whether you want to make the patch and test it. I mean, the whole file has no code modifications since bd6ecbe48ada (2020), and that specific function is the same since it was first committed (bf1431e3596b, from 2012). I don't think I should make a separate patch for backports: the one I originally posted cherry-picks cleanly even to releases/gcc-4.8.0 (first release containing bf1431e3596b, at least according to git tag --contains), and the caller code is just the same as well, so I expect that technically it could be applied pretty much up to there without any modification. Now, having a look at https://gcc.gnu.org/releases.html I seem to understand that the current "active" major releases are 11, 12 and 13. I would surely backport to 13, especially given that the patch was developed and tested on that release in first place. 11 and 12 would be nice too, although I made no explicit tests there; a quick check with git diff releases/gcc-11.1.0 origin/master -- libgcc/unwind.inc libgcc/unwind-seh.c libstdc++-v3/libsupc++/eh_throw.cc doesn't show any difference that is relevant for my patch. Still, if I find some time for that I could compile these patched releases and see if the patch still works correctly for extra caution. Matteo
[PATCH] libgcc: fix SEH C++ rethrow semantics [PR113337]
SEH _Unwind_Resume_or_Rethrow invokes abort directly if
_Unwind_RaiseException doesn't manage to find a handler for the rethrown
exception; this is incorrect, as in this case std::terminate should be
invoked, allowing an application-provided terminate handler to handle
the situation instead of straight crashing the application through
abort.
The bug can be demonstrated with this simple test case:
===
static void custom_terminate_handler() {
fprintf(stderr, "custom_terminate_handler invoked\n");
std::exit(1);
}
int main(int argc, char *argv[]) {
std::set_terminate(&custom_terminate_handler);
if (argc < 2) return 1;
const char *mode = argv[1];
fprintf(stderr, "%s\n", mode);
if (strcmp(mode, "throw") == 0) {
throw std::exception();
} else if (strcmp(mode, "rethrow") == 0) {
try {
throw std::exception();
} catch (...) {
throw;
}
} else {
return 1;
}
return 0;
}
===
On all gcc builds with non-SEH exceptions, this will print
"custom_terminate_handler invoked" both if launched as ./a.out throw or
as ./a.out rethrow, on SEH builds instead if will work as expected only
with ./a.exe throw, but will crash with the "built-in" abort message
with ./a.exe rethrow.
This patch fixes the problem, forwarding back the error code to the
caller (__cxa_rethrow), that calls std::terminate if
_Unwind_Resume_or_Rethrow returns.
The change makes the code path coherent with SEH _Unwind_RaiseException,
and with the generic _Unwind_Resume_or_Rethrow from libgcc/unwind.inc
(used for SjLj and Dw2 exception backend).
libgcc/ChangeLog:
* unwind-seh.c (_Unwind_Resume_or_Rethrow): forward
_Unwind_RaiseException return code back to caller instead of
calling abort, allowing __cxa_rethrow to invoke std::terminate
in case of uncaught rethrown exception
---
libgcc/unwind-seh.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libgcc/unwind-seh.c b/libgcc/unwind-seh.c
index 8ef0257b616..f1b8f5a8519 100644
--- a/libgcc/unwind-seh.c
+++ b/libgcc/unwind-seh.c
@@ -395,9 +395,9 @@ _Unwind_Reason_Code
_Unwind_Resume_or_Rethrow (struct _Unwind_Exception *exc)
{
if (exc->private_[0] == 0)
-_Unwind_RaiseException (exc);
- else
-_Unwind_ForcedUnwind_Phase2 (exc);
+return _Unwind_RaiseException (exc);
+
+ _Unwind_ForcedUnwind_Phase2 (exc);
abort ();
}
--
2.34.1
Ping: Re: [PATCH] libgcc: fix SEH C++ rethrow semantics [PR113337]
Ping! That's a one-line fix, and you can find all the details in the
bugzilla entry. Also, I can provide executables built with the affected
toolchains, demonstrating the problem and the fix.
Thanks,
Matteo
Il 17/01/24 12:51, Matteo Italia ha scritto:
SEH _Unwind_Resume_or_Rethrow invokes abort directly if
_Unwind_RaiseException doesn't manage to find a handler for the rethrown
exception; this is incorrect, as in this case std::terminate should be
invoked, allowing an application-provided terminate handler to handle
the situation instead of straight crashing the application through
abort.
The bug can be demonstrated with this simple test case:
===
static void custom_terminate_handler() {
fprintf(stderr, "custom_terminate_handler invoked\n");
std::exit(1);
}
int main(int argc, char *argv[]) {
std::set_terminate(&custom_terminate_handler);
if (argc < 2) return 1;
const char *mode = argv[1];
fprintf(stderr, "%s\n", mode);
if (strcmp(mode, "throw") == 0) {
throw std::exception();
} else if (strcmp(mode, "rethrow") == 0) {
try {
throw std::exception();
} catch (...) {
throw;
}
} else {
return 1;
}
return 0;
}
===
On all gcc builds with non-SEH exceptions, this will print
"custom_terminate_handler invoked" both if launched as ./a.out throw or
as ./a.out rethrow, on SEH builds instead if will work as expected only
with ./a.exe throw, but will crash with the "built-in" abort message
with ./a.exe rethrow.
This patch fixes the problem, forwarding back the error code to the
caller (__cxa_rethrow), that calls std::terminate if
_Unwind_Resume_or_Rethrow returns.
The change makes the code path coherent with SEH _Unwind_RaiseException,
and with the generic _Unwind_Resume_or_Rethrow from libgcc/unwind.inc
(used for SjLj and Dw2 exception backend).
libgcc/ChangeLog:
* unwind-seh.c (_Unwind_Resume_or_Rethrow): forward
_Unwind_RaiseException return code back to caller instead of
calling abort, allowing __cxa_rethrow to invoke std::terminate
in case of uncaught rethrown exception
---
libgcc/unwind-seh.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libgcc/unwind-seh.c b/libgcc/unwind-seh.c
index 8ef0257b616..f1b8f5a8519 100644
--- a/libgcc/unwind-seh.c
+++ b/libgcc/unwind-seh.c
@@ -395,9 +395,9 @@ _Unwind_Reason_Code
_Unwind_Resume_or_Rethrow (struct _Unwind_Exception *exc)
{
if (exc->private_[0] == 0)
-_Unwind_RaiseException (exc);
- else
-_Unwind_ForcedUnwind_Phase2 (exc);
+return _Unwind_RaiseException (exc);
+
+ _Unwind_ForcedUnwind_Phase2 (exc);
abort ();
}
