This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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.Oct 7 2021, 4:59 PM
snehasish created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2021, 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
109

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.Oct 11 2021, 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
122

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

125

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
1

first two lines need formating

9

lines 9-12 can be removed

34

maybe delete them from ref as well

and then at the end

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

ASSERT_NE()

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

Address comments.

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

Thanks for the review. PTAL!

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

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.Oct 12 2021, 4:58 PM
compiler-rt/lib/sanitizer_common/sanitizer_addrhashmap.h
125

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.Oct 13 2021, 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.Oct 13 2021, 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.Oct 13 2021, 5:55 PM