This is an archive of the discontinued LLVM Phabricator instance.

Generalize stackdepot.
ClosedPublic

Authored by eugenis on May 15 2014, 7:28 AM.

Details

Reviewers
kcc
dvyukov
Summary

This is a work in progress, but I'd like some comments on where this is going.

Allow more than one stackdepot in a process.
Generalize the code to provide a compact storage for chained origins, which is a map (u32, u32) -> u32.
It is different from the normal stackdepot in that it does not keep a copy of the hash value in each table cell.
I also add a counter to stackdepot nodes that would be increased each time a stack participates in origin chaining. I use the lower 20 bits of the hash value for this counter.

I'm not sure I like the end result, it's pretty complex and tightly coupled. I don't see how to make it simpler short of duplicating the hash table implementation.

Diff Detail

Event Timeline

eugenis updated this revision to Diff 9443.May 15 2014, 7:28 AM
eugenis retitled this revision from to Generalize stackdepot. .
eugenis updated this object.
eugenis edited the test plan for this revision. (Show Details)
eugenis added reviewers: dvyukov, kcc, deleted.
eugenis set the repository for this revision to rL LLVM.
dvyukov edited edge metadata.May 16 2014, 12:32 AM

Please move the code back to .cc file. Otherwise it's impossible to figure out what's changed.

eugenis updated this revision to Diff 9463.May 16 2014, 1:32 AM
eugenis edited edge metadata.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: Unknown Object (MLST).

*test*

eugenis updated this revision to Diff 9468.May 16 2014, 5:01 AM

Split the new code into multiple files for easier understanding.

glider added a subscriber: glider.May 16 2014, 5:26 AM
glider added inline comments.
sanitizer_common/sanitizer_chainedorigindepot.cc
10 ↗(On Diff #9468)

Drive-by nit: this comment doesn't explain the what this file is for.

sanitizer_common/sanitizer_chainedorigindepot.h
1 ↗(On Diff #9468)

Drive-by nit: please split "chainedorigindepot" like we do for other lengthy filenames.

sanitizer_common/sanitizer_region_allocator.h
10 ↗(On Diff #9468)

Ditto.

sanitizer_common/sanitizer_stackdepot.h
66 ↗(On Diff #9468)

Nit: spare newline(s)?

eugenis updated this revision to Diff 9470.May 16 2014, 5:52 AM

Adding MSan changes to give an idea of how this is going to be used.
I'll commit them separately.

dvyukov added inline comments.May 18 2014, 11:57 PM
sanitizer_common/sanitizer_chainedorigindepot.h
22 ↗(On Diff #9470)

Why doesn't this live in msan dir now? This looks msan-specific.

sanitizer_common/sanitizer_region_allocator.h
1 ↗(On Diff #9470)

please use "svn copy" for this file to preserve history.

23 ↗(On Diff #9470)

I would call it "PersistentAllocator" rather than RegionAllocator. Region allocators usually assume existence of plurality of regions (e.g. one per each incoming server request). Region allocators also usually provide region free operations.

sanitizer_common/sanitizer_stackdepot.h
18 ↗(On Diff #9470)

does this need to be included in the header file?
it would be nice if template classes are included only in source files.

sanitizer_common/sanitizer_stackdepotbase.h
1 ↗(On Diff #9470)

svn copy as well

eugenis updated this revision to Diff 9574.May 19 2014, 7:58 AM
eugenis updated this revision to Diff 9601.May 20 2014, 2:25 AM

Added 2 msan flags to limit history growth, removed a deprecated test-only flag, added a test. Addressed comments.

sanitizer_common/sanitizer_chainedorigindepot.h
22 ↗(On Diff #9470)

done

sanitizer_common/sanitizer_region_allocator.h
1 ↗(On Diff #9470)

done

23 ↗(On Diff #9470)

done

sanitizer_common/sanitizer_stackdepot.h
18 ↗(On Diff #9470)

removed this include from header files

sanitizer_common/sanitizer_stackdepotbase.h
1 ↗(On Diff #9470)

done

eugenis updated this revision to Diff 9605.May 20 2014, 3:59 AM
eugenis updated this revision to Diff 9607.May 20 2014, 4:14 AM
eugenis updated this revision to Diff 9617.May 20 2014, 6:11 AM

+ a comment in msan_origin.h explaining what is this all about.

dvyukov accepted this revision.May 20 2014, 10:55 AM
dvyukov edited edge metadata.

LGTM

lib/msan/msan.cc
284

Isn't it all in __msan namespace?

566

Isn't it subject to races? What if two threads trigger this alloca for the first time?

lib/msan/msan_allocator.cc
108

Origin(id, 1)
can't we start depth from 0 here? that would enlarge the maximum chain depth by 1.

This revision is now accepted and ready to land.May 20 2014, 10:55 AM
eugenis added inline comments.May 21 2014, 1:17 AM
lib/msan/msan.cc
284

right

566

Yeah, we may have duplicate entries for stack origins.

lib/msan/msan_allocator.cc
108

The idea here is to reserve depth value of 0 for unlimited history length for this single origin. Not sure we need that, but adding it later would reduce max history length by 1 and may break existing users.

dvyukov added inline comments.May 21 2014, 1:43 AM
lib/msan/msan.cc
566

Then remove the CHECK.

lib/msan/msan_allocator.cc
108

Ack.

eugenis updated this revision to Diff 9654.May 21 2014, 2:00 AM
eugenis edited edge metadata.
eugenis added inline comments.
lib/msan/msan.cc
566

done

eugenis closed this revision.May 21 2014, 2:11 AM

Committed as r209284.
Thanks for the review!