Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Race in InlinedFunction::setFile #20

Closed
blue42u opened this issue Apr 21, 2019 · 0 comments
Closed

Race in InlinedFunction::setFile #20

blue42u opened this issue Apr 21, 2019 · 0 comments

Comments

@blue42u
Copy link
Owner

blue42u commented Apr 21, 2019

Yet another boost::multi_index_container that gets used in various locations.

Valgrind Stanza:

==19807== Possible data race during write of size 8 at 0x1D7B6E30 by thread #4
==19807== Locks held: none
==19807==    at 0x4E7BC1E: link (ord_index_node.hpp:389)
==19807==    by 0x4E7BC1E: insert_<boost::multi_index::detail::rvalue_tag> (ord_index_impl.hpp:718)
==19807==    by 0x4E7BC1E: insert_<boost::multi_index::detail::rvalue_tag> (ord_index_impl.hpp:716)
==19807==    by 0x4E7BC1E: insert_<boost::multi_index::detail::rvalue_tag> (random_access_index.hpp:778)
==19807==    by 0x4E7BC1E: insert_<boost::multi_index::detail::rvalue_tag> (multi_index_container.hpp:558)
==19807==    by 0x4E7BC1E: insert_rv_ (multi_index_container.hpp:575)
==19807==    by 0x4E7BC1E: final_insert_rv_ (index_base.hpp:221)
==19807==    by 0x4E7BC1E: insert (ord_index_impl.hpp:302)
==19807==    by 0x4E7BC1E: Dyninst::SymtabAPI::InlinedFunction::setFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (Function.C:484)
==19807==    by 0x4F25D2F: Dyninst::SymtabAPI::DwarfWalker::parseCallsite() (dwarfWalker.C:507)
==19807==    by 0x4F2EF18: Dyninst::SymtabAPI::DwarfWalker::parseSubprogram(Dyninst::SymtabAPI::DwarfWalker::inline_t) (dwarfWalker.C:649)
==19807==    by 0x4F2E61C: Dyninst::SymtabAPI::DwarfWalker::parse_int(Dwarf_Die, bool) (dwarfWalker.C:364)
==19807==    by 0x4F2E778: Dyninst::SymtabAPI::DwarfWalker::parse_int(Dwarf_Die, bool) (dwarfWalker.C:456)
==19807==    by 0x4F2E778: Dyninst::SymtabAPI::DwarfWalker::parse_int(Dwarf_Die, bool) (dwarfWalker.C:456)
==19807==    by 0x4F2F237: Dyninst::SymtabAPI::DwarfWalker::parseModule(Dwarf_Die, Dyninst::SymtabAPI::Module*&) (dwarfWalker.C:259)
==19807==    by 0x4F2F42E: Dyninst::SymtabAPI::DwarfWalker::parse() [clone ._omp_fn.0] (dwarfWalker.C:169)
==19807==    by 0x64F1E55: gomp_thread_start (team.c:119)
==19807==    by 0x4C2DF4C: mythread_wrapper (hg_intercepts.c:389)
==19807==    by 0x69239D0: start_thread (in /lib64/libpthread-2.12.so)
==19807==    by 0x6C21B6C: clone (in /lib64/libc-2.12.so)
==19807==  Address 0x1d7b6e30 is 80 bytes inside a block of size 120 alloc'd
==19807==    at 0x4C285CC: operator new(unsigned long) (vg_replace_malloc.c:332)
==19807==    by 0x4E7BB49: allocate (new_allocator.h:104)
==19807==    by 0x4E7BB49: allocate_node (multi_index_container.hpp:530)
==19807==    by 0x4E7BB49: insert_ (index_base.hpp:117)
==19807==    by 0x4E7BB49: insert_<boost::multi_index::detail::rvalue_tag> (ord_index_impl.hpp:716)
==19807==    by 0x4E7BB49: insert_<boost::multi_index::detail::rvalue_tag> (ord_index_impl.hpp:716)
==19807==    by 0x4E7BB49: insert_<boost::multi_index::detail::rvalue_tag> (random_access_index.hpp:778)
==19807==    by 0x4E7BB49: insert_<boost::multi_index::detail::rvalue_tag> (multi_index_container.hpp:558)
==19807==    by 0x4E7BB49: insert_rv_ (multi_index_container.hpp:575)
==19807==    by 0x4E7BB49: final_insert_rv_ (index_base.hpp:221)
==19807==    by 0x4E7BB49: insert (ord_index_impl.hpp:302)
==19807==    by 0x4E7BB49: Dyninst::SymtabAPI::InlinedFunction::setFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (Function.C:484)
==19807==    by 0x4F25D2F: Dyninst::SymtabAPI::DwarfWalker::parseCallsite() (dwarfWalker.C:507)
==19807==    by 0x4F2EF18: Dyninst::SymtabAPI::DwarfWalker::parseSubprogram(Dyninst::SymtabAPI::DwarfWalker::inline_t) (dwarfWalker.C:649)
==19807==    by 0x4F2E61C: Dyninst::SymtabAPI::DwarfWalker::parse_int(Dwarf_Die, bool) (dwarfWalker.C:364)
==19807==    by 0x4F2E778: Dyninst::SymtabAPI::DwarfWalker::parse_int(Dwarf_Die, bool) (dwarfWalker.C:456)
==19807==    by 0x4F2E778: Dyninst::SymtabAPI::DwarfWalker::parse_int(Dwarf_Die, bool) (dwarfWalker.C:456)
==19807==    by 0x4F2E778: Dyninst::SymtabAPI::DwarfWalker::parse_int(Dwarf_Die, bool) (dwarfWalker.C:456)
==19807==    by 0x4F2E778: Dyninst::SymtabAPI::DwarfWalker::parse_int(Dwarf_Die, bool) (dwarfWalker.C:456)
==19807==    by 0x4F2E778: Dyninst::SymtabAPI::DwarfWalker::parse_int(Dwarf_Die, bool) (dwarfWalker.C:456)
==19807==    by 0x4F2F237: Dyninst::SymtabAPI::DwarfWalker::parseModule(Dwarf_Die, Dyninst::SymtabAPI::Module*&) (dwarfWalker.C:259)
==19807==    by 0x4F2F42E: Dyninst::SymtabAPI::DwarfWalker::parse() [clone ._omp_fn.0] (dwarfWalker.C:169)
==19807==  Block was alloc'd by thread #1
hainest added a commit to dyninst/dyninst that referenced this issue Jan 13, 2020
* changes for parallelizing symtabapi

* Tweaks to use OpenMP (still one last reducer left)

* Mark a false race in Type.C.
    std::call_once has a barrier at the end, so anything that happens inside is visible to anything that happens after.

* Replace the Cilk reducer with an OpenMP reduction

* Remove some dead code, and move the reducer into the source file.

* Mark the last race so far, move VG macros to a separate file.

* Annotate the two core locks, negating a lot of the race reports
    Implementation is not great though, I moved mcs_init to be a real function call.
    There might be a better way to do this, will investigate at some point.

* Very awkwardly annotate a parallel hashmap by key. Consider implementing better magic.

* Wrap the callback in `omp critical`, to make it parallel-safe no matter what happens.

* Swap out the vector for a TBB concurrent_queue.

* Remove some unnessesary annotations, using a proper OpenMP handles them.

* Revert the core lock annotations, they don't actually really work.

* Macro-replace the locks with boost equivalents. Revert this commit later once the testing is complete, or refactor it fully out.

* Replace the 1-entry cache with a vector that expands with the number of threads.

* Reuse the DwarfWalker between loop iterations, to save some work

* Use the standard allocator, so that Valgrind can track it properly

* Silence some warnings

* Replace the write with a CAS, letting Valgrind mostly ignore it.

* Strengthen the condition, its good enough for now.

* Re-annotate the hash map similar to an RW lock (which it technically is)

* Nearly fully fix the function-static annotations, using a very small constructor.

* Add libc++ annotations to vgannotations, and shift includes around to make them work

* Actually do the annotations right. I learned things today.

* Tell Helgrind to ignore a few more things

* Disable checking on everything

* A few tweaks to annotation

* Add h-b arcs to the pfq rwlock

* Apparently vectors didn't work like I thought they did. Whoops.

* Elfutils is now more thread-safe, and fix the annotations to keep DRD quiet.

* Make vgannotations.h local to Dyninst, and do something C++ for the lazy inits.

* Make the custom locks first-class C++ types, and compatible with C++17's syntax.

* Replace the lock implementations with more reasonable alternatives.

* Move the annotation to handle the case where std::pair does the write

* Unify all the TBB types under a Dyninst-tagged namespace, to permit refactor

Also fix a few whitespace errors near affected lines.

* Move concurrent_hash_map annotations into the unified class.

* Shift the thread-local stuff into a template class

* Wiggle the parallel loops and fine-grain the locking for a performance boost.

* Use Boost's call_once to try and be a bit more portable

* Put some parallelism into DwarfFrameParser. Fixes blue42u#18 and blue42u#19

* Use atomics for the reference counting. Fixes blue42u#4.

* Add an extra lock to the StringTable, and use it to mediate access to the internals. Fixes blue42u#20.

* Add a lock for inlines management, fixes blue42u#22.

* Add a lock to the function frame vectors, fixes blue42u#21.

* Replace a map+lock with a proper parallel hashmap for performance.

* Replace a mutex'd multi_index_container with a series of concurrent_hash_maps.

* Remove the comments that were left over from Cilkscreen race detection.

...I meant to do this a long time ago...

* Fix a number of minor issues, and one possibly important typo.

* Use RAII-style classes when handling the locks.

* Add a few defines for when Valgrind annotations are turned off.

* Parallelize some stuff in Object, makes the loading of files a little faster.

* Swap out some hash_maps for their concurrent forms, more easy parallelism.

* Actually do the parsing properly, and add a lock where it was needed.

* Adding more parallelism.

* Get rid of unnecessary serial code in finalization and add parallelism for hints initialization

* Removed a lock, and attempts to fix the resulting issues that arose afterwards.

* Tweaks to remove the phase-based approach

* 1. Fix missing parsed edges caused by early resuming frames
2. Always choosing the alphabetically smallest name for a function if there are multiple

* Delete swap_busy

* 1. Handle ud2 instruction, which will raise an undefined opcode exception. Therefore, control flow should not fall through
2. When deleting a bogus function, the reference counts of the blocks in the function should be decremented

* 1. Rewrite createAndRecordFrame to allow concurrent frame creations.
2. Simplify the use of frame status: BAD_LOOKUP means frame does not exists
   and a caller should only create a new frame when the result is BAD_LOOKUP

* 1. Resume functions as soon as a function finds a ret instruction
2. Rewrite parts of the tail call correction in finalization

* Estimate parsing task size by function symbol size
and launch large tasks first

* Delete omp critical in parisng of a fram

* Get rid of unnecessary assert

* Small changes to fix some maybe-races

* Munge the annotations for c_hash_map a little, and hotfix a possible race.

* Silence a number of warnings. Identations a mess but its quieter.

* Rewrite the c_hash_map to expose the accessors as a rwlock.

* Replace lock for delayed frames with concurrent hash map

* Parallelize SymtabCodeSource::init_hints

* Parallelize CodeObject::process_hints

* Fix an infinite loop in the parsing finalization stages.

* 1. Fix parallelization for CodeObject::process_hint(): a local variable declared is moved from outside loop to inside loop
2. Fix debug print crash in Parser.C
3. Some code cleanup

* ParseAPI now initialize hints in parallel. So, if the user of ParseAPI overloads the function for creating
ParseAPI::Function, that code should be thread-safe.

Change the DynCFGFactor in dyninstAPI to use mutex to be thread-safe.

* Lasily preparing range data for functions and blocks.

* Add block ranges and clean some dead code

* Add a CMake flag to enable Valgrind annotations

* Use dyn_c_vector, and disable the parallelism for ELF stuff for now.

* Rough additions of locks in various places to make things work.

* Properly initialize the Module in all constructors.

* Fix compilation

* Get rid unnecessary boost::lock_guard and use entry lookup in loop tree construction

* Use static AArch64 decoder tables. Fixes #630.

Other improvements include faster compile time (for affected files),
~1s less load time, ~7.8MiB smaller binary, and ~200KiB more memory usage.

Performance effects not yet tested.

* Should not delete unused ParseAPI::Function during parsing because CFGFactory will do delete all created functions in its destructor.

* Use exchange instead of store to keep Valgrind happy.

* Several fixes for analyzing .a files

1. Rewrite the OverlappingParseData to correct handle overlapping code regions.
2. In .o files, code starts at address 0, so address 0 can be a valid address.
   So, change indirect call target from address 0 to max address to represent indiret calls
3. Use CodeSource to check address validity, which would allow cross CodeRegions valid address;
   but use CodeRegion for raw code bytes, because using CodeSource may return code bytes
   from other regions that are overlapping.

* Fixes for gap parsing, which uses a different interface to call Parser

* Fix range data related to gap parsing

* Fix loop tree callee construction on Power

* Fix function removing in parsing finalization

* Fix crahses of symtabAPI tests on power

* Fix deadlock in constructing analysis graph for jump table analysis.

* Type refcount refactor, part 1: s/Type*/boost::shared_ptr<Type>/g

* Purge Type::refCount and all related code bits. The shared_ptr's handle it now.

Also add proper SFINAE on that one template, since now typeScalar and Type
have the same size (thank GCC's automatic bitfield construction).

* Add the backwards compatbility layer

* Fixup for a minor issue that should have popped up sooner

* CFGFactory class does not need to inherit boost_guard

* Fix compilation on ARM, and fix part of the backwards-compat layer.

* Adjust cmake file to new elfutils

* libdyninstAPI_RT.so should not link against libgomp, which would
cause crash at program startup time due to calling into uninitialized
rewritten libc.

* Cleanup OpenMP handling in build system

* Fix compilation when USE_OpenMP is set to OFF

* Fix a debug print crash

* Fix infinite recursion caused by missing stack unwind debug info

* Disable installing trampolines in instrumentation

* Stack walk should always have an increasing SP on x86

* Several fixes for parallel code parsing

1. Add a jump table finalization step. The assumption here is that different jump tables
   do not share entries. So, if one jump table runs into another one, we know that the
   entries that are overlapping with the next table are out-of-bound.
2. Remove edges and blocks for created by out-of-bound jump table entries
3. Handle problems of failing to resolve jump tables caused by out-of-bound entries from
   other jump table entries.

Co-authored-by: Jonathon Anderson <17242663+blue42u@users.noreply.github.com>
Co-authored-by: Tim Haines <thaines.astro@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant