On 11/06/15 11:28, Martin Storsjo wrote:
int CDECL _callnewh(MSVCRT_size_t size) {
- int ret = 0;
- LOCK_HEAP; if(MSVCRT_new_handler)
- (*MSVCRT_new_handler)(size);
- return 0;
- ret = (*MSVCRT_new_handler)(size);
- UNLOCK_HEAP;
- return ret; }
It doesn't make sense to lock heap cs in this case. Native is not doing it (at least in case of msvcp90.dll). The return value change looks almost correct - the function only returns 0 or 1.
Thanks, Piotr
On Fri, 6 Nov 2015, Piotr Caban wrote:
On 11/06/15 11:28, Martin Storsjo wrote:
int CDECL _callnewh(MSVCRT_size_t size) {
- int ret = 0;
- LOCK_HEAP; if(MSVCRT_new_handler)
- (*MSVCRT_new_handler)(size);
- return 0;
- ret = (*MSVCRT_new_handler)(size);
- UNLOCK_HEAP;
- return ret; }
It doesn't make sense to lock heap cs in this case. Native is not doing it (at least in case of msvcp90.dll).
Hmm, but wouldn't it risk a race with another thread doing set_new_handler then? OTOH I guess that's ok/expected?
But there's a similar lock in MSVCRT_operator_new; should we do the same locking on the msvcp side instead then, or just skip it altogether?
The return value change looks almost correct - the function only returns 0 or 1.
Ok, so should I force the return value to 0 or 1 regardless of what the function returns? I.e. ret = (*MSVCRT_new_handler)(size) ? 1 : 0;
(or !!), or should I just repost it without the locking?
// Martin
On 11/06/15 11:53, Martin Storsjö wrote:
On Fri, 6 Nov 2015, Piotr Caban wrote:
On 11/06/15 11:28, Martin Storsjo wrote:
int CDECL _callnewh(MSVCRT_size_t size) {
- int ret = 0;
- LOCK_HEAP; if(MSVCRT_new_handler)
- (*MSVCRT_new_handler)(size);
- return 0;
- ret = (*MSVCRT_new_handler)(size);
- UNLOCK_HEAP;
- return ret; }
It doesn't make sense to lock heap cs in this case. Native is not doing it (at least in case of msvcp90.dll).
Hmm, but wouldn't it risk a race with another thread doing set_new_handler then? OTOH I guess that's ok/expected?
Yes, there's a possibility of race in wine code. It can be easily avoided by storing new_handler in local variable in callnewh. Then it will be possible to call old handler while new handler was set. I think that it's better then possible crash if new_handler is set to NULL.
But there's a similar lock in MSVCRT_operator_new; should we do the same locking on the msvcp side instead then, or just skip it altogether?
I guess that operator_new should use callnewh without locking the heap section. It can be easily tested by setting new_handler that takes lots of time to finish and trying to allocate big memory chunks in 2 threads. If second new_handler is called while first is still executed than there's no lock in native library (it's not a test that can go into wine because it depends on allocation failure).
The return value change looks almost correct - the function only returns 0 or 1.
Ok, so should I force the return value to 0 or 1 regardless of what the function returns? I.e. ret = (*MSVCRT_new_handler)(size) ? 1 : 0;
Yes, this is what native does.
Thanks, Piotr
On Fri, 6 Nov 2015, Piotr Caban wrote:
On 11/06/15 11:53, Martin Storsjö wrote:
On Fri, 6 Nov 2015, Piotr Caban wrote:
On 11/06/15 11:28, Martin Storsjo wrote:
int CDECL _callnewh(MSVCRT_size_t size) {
- int ret = 0;
- LOCK_HEAP; if(MSVCRT_new_handler)
- (*MSVCRT_new_handler)(size);
- return 0;
- ret = (*MSVCRT_new_handler)(size);
- UNLOCK_HEAP;
- return ret; }
It doesn't make sense to lock heap cs in this case. Native is not doing it (at least in case of msvcp90.dll).
Hmm, but wouldn't it risk a race with another thread doing set_new_handler then? OTOH I guess that's ok/expected?
Yes, there's a possibility of race in wine code. It can be easily avoided by storing new_handler in local variable in callnewh. Then it will be possible to call old handler while new handler was set. I think that it's better then possible crash if new_handler is set to NULL.
But there's a similar lock in MSVCRT_operator_new; should we do the same locking on the msvcp side instead then, or just skip it altogether?
I guess that operator_new should use callnewh without locking the heap section. It can be easily tested by setting new_handler that takes lots of time to finish and trying to allocate big memory chunks in 2 threads. If second new_handler is called while first is still executed than there's no lock in native library (it's not a test that can go into wine because it depends on allocation failure).
Ok, I tested this, and indeed, two threads can simultaneously execute the new handler. So I'll send a patch for removing the lock from MSVCRT_operator_new then as well.
// Martin
-
Martin Storsjö -
Piotr Caban