Closed ioannis-e closed 8 years ago
See if you like ioannis-e@357f91a
I don't know how I fill about this changes :-) Too many critical sections and locks. Also, help for DbgHelp doesn't guarantee that your changes will work.
All DbgHelp functions, are single threaded. Therefore, calls from more than one thread to this function will likely result in unexpected behavior or memory corruption. To avoid this, you must synchronize all concurrent calls from more than one thread to this function.
But you use 4 CS.
@KindDragon I understand, so let me explain my reasoning.
There were already 2 locks in code g_symbolLock
and g_imageLock
so I guess there was a reasoning behind having two locks instead of just one lock.
Using g_symbolLock
with StackWalk64
caused a deadlock in safe
and StaticCrt
mode.
I added 2 more one for StackWalk64
and one for EnumerateLoadedModulesW64
I kept the same lock for all other Sym*
functions.
Also I used the wrapper functions in order to avoid unecessary blocking or neigbouring code by the locks.
Regarding DbgHelp
help its unclear to me whether all functions of DbgHelp
need to be syncronised between threads or whether calls to the same function need to be syncronised between threads, I went with the latter and haven't identified any corruption yet...
I also fix deadlock in Magic commit :-) 9902f53e12ccf761208f78601b42024747da7969 https://ci.appveyor.com/project/KindDragon/vld/build/100 But I do not remember why I added heapMapLock earlier
heapMapLock was added not so long ago https://github.com/KindDragon/vld/commit/b9a53cd07818539045248e812dbdc52211f38ce0 instead of g_stackWalkLock
Also I used the wrapper functions in order to avoid unecessary blocking or neigbouring code by the locks.
But frequent entry/exit to critical section is slower than once to enter before loop.
How you think if I partially revert b9a53cd ("CS g_stackWalkLock merged with g_heapMapLock"). I think this should fix deadlock
You see upto that commit each DbgHelp function had its own locker and all other Sym* functions had one locker, which is basically what i did but instead put them in a class.
So I guess you should go back to g_stackWalkLock for StackWalk64 and probably add one more lock for EnumerateModulesW64.
What I also considered is that StackWalk64 and EnumerateModulesW64 are the most time consuming functions
I would also suggest the following changes
m_modulesLock does not need to block IsModulePatched because that function acquires other locks that could lead to deadlock
https://github.com/ioannis-e/vld/commit/357f91a#diff-a895db1a9b34fcae7efbc9c48edaf1c4L726
and
Also I think nobody likes IsBadReadPtr
:)
https://github.com/ioannis-e/vld/commit/357f91a#diff-a895db1a9b34fcae7efbc9c48edaf1c4L723
I finally managed to build under VS2010.
I have an issue with _heap_term()
where in VS2010 it calls HeapDestroy
while in VS2013 and VS2015 it does not.
This causes some StaticCrt
tests ie dynamic_app
and vld_unload
to fail in VS2010 as the leaks are reported before destroying the heap (in order to perform the memory dump) and the heap map is erased, and are not taken into account when calling GetLeakCount
.
I was wandering whether you would accept on HeapDestroy
in addition to reporting the leaks immediately, to keep the heap info (if leaks are found) and resolve the stacks in order to be able to report the leaks when vld unloads without a memory dump.
I'm currently working on some changes to see how it goes.
Did you get any feedback from appveyor support regarding v110 toolset ?
They use VS Express and it doesn't contain MFC
I finally managed to build under VS2010.
VS2012 build failed for Environment: VldStackWalkMethod=fast, Solution=vld_vs11_wo_mfc.sln, GTESTFILTER=-.Mfc_; Configuration: Release_StaticCrt; Platform: Win32 https://ci.appveyor.com/project/KindDragon/vld/build/119
This causes some StaticCrt tests ie dynamic_app and vld_unload to fail in VS2010 as the leaks are reported before destroying the heap (in order to perform the memory dump) and the heap map is erased, and are not taken into account when calling GetLeakCount.
Maybe just change leaks count for this configuration (will simply be known difference in behavior). Someday we will remove support for VS2010
The highlights of the changes made are as follows:
this may have an effect in performance.I tried to keep commits independed of each other as much as possible, to make review easier.
I would suggest to cherypick as much as possible into master in order to rebase this PR and leave us with only the commits that need discussion.