Page MenuHomePhabricator

[sanitizer] Add a ForEach callback interface for AddrHashMap.
AcceptedPublic

Authored by snehasish on Thu, Oct 7, 4:59 PM.

Details

Reviewers
vitalybuka
Summary

This change adds a ForEach method to the AddrHashMap class which can
then be used to iterate over all the key value pairs in the hash map.
I intend to use this in an upcoming change to the memprof runtime.

Diff Detail

Event Timeline

snehasish requested review of this revision.Thu, Oct 7, 4:59 PM
snehasish created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Oct 7, 4:59 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

I couldn't find any unit tests for this hashmap implementation. @vitalybuka if you can point me to an appropriate place, I would be happy to add a test to cover this new method.

some basic unittest could be nice

compiler-rt/lib/sanitizer_common/sanitizer_addrhashmap.h
117

ReadLock l(bucket->mtx);

I couldn't find any unit tests for this hashmap implementation. @vitalybuka if you can point me to an appropriate place, I would be happy to add a test to cover this new method.

I see, just create new sanitizer_addrhashmap_test.cpp

Added a unit test.

snehasish marked an inline comment as done.Mon, Oct 11, 11:57 AM

Added a unit test and addressed the comment. PTAL, thanks!

vitalybuka added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_addrhashmap.h
130

I believe all 3 atomic_loads needs memory_order_acquire to make sure that val is loaded after we loaded addr1

133

cc @dvyukov
I guess nothing prevents other threads from modifying content of *(c->val) when callback is running ReadLock only guaranties that storage for val will not be released. So other syncronization will be needed on callback side.

It would be nice to see patches which use the new function.

compiler-rt/lib/sanitizer_common/tests/sanitizer_addrhashmap_test.cpp
2

first two lines need formating

10

lines 9-12 can be removed

35

maybe delete them from ref as well

and then at the end

m.ForEach(ExistsInReferenceMap, &reference_map);
EXPECT_TRUE(m.empty());
36

ASSERT_NE()

snehasish updated this revision to Diff 379167.Tue, Oct 12, 1:27 PM

Address comments.

snehasish marked 4 inline comments as done.Tue, Oct 12, 1:40 PM

Thanks for the review. PTAL!

compiler-rt/lib/sanitizer_common/sanitizer_addrhashmap.h
133

I changed the lock type to just a read write lock. The remainder of the code is borrowed from the locked version of acquire (L243-L276) in this revision. If we hold an exclusive lock for the bucket then I don't think we need to ensure that the operations are ordered.

Memprof usage is shown in child revision D111676 - see memprof_allocator.cpp L258. It is called when the profiler destructor is called and the contents of the map, i.e. the profile is written out.

vitalybuka added inline comments.Tue, Oct 12, 4:58 PM
compiler-rt/lib/sanitizer_common/sanitizer_addrhashmap.h
133

ReadLock is fine.
Issue is that bucket->mtx only locks structure of AddrHashMap only, neither Lock or ReadLock help us here regarding content of the val.

Thread1:
Map::Handle h(&m, pre_existing_address); // returns from acquire on L223 doing essentially nothing
h->update_data();

Thread2:
m.ForEach(Print, ..)

So there is data race between Print and Merge

snehasish updated this revision to Diff 379522.Wed, Oct 13, 2:24 PM

Address comments.

Thanks for taking the time to clarify with an example, Vitaly. I completely missed the point when you raised it previously. I've reverted the change here and added some documentation to clarify expectations for users. I'll update the child revision with additional locking to prevent the data race on accessing the element.

vitalybuka accepted this revision.Wed, Oct 13, 5:55 PM

LGTM, but I slightly prefer you avoid this patch and go with simpler solution on another patch.

This revision is now accepted and ready to land.Wed, Oct 13, 5:55 PM