From d105ad72ee1885bcf178487802b735282a92528d Mon Sep 17 00:00:00 2001 From: Benoit Jacob Date: Sat, 21 Nov 2020 00:35:38 -0500 Subject: [PATCH] Move all back to TracyProfiler.cpp. Only need the fix in HandleSymbolQuery, not DecodeSymbolAddress --- client/TracyCallstack.cpp | 166 -------------------------------------- client/TracyCallstack.hpp | 13 +-- client/TracyProfiler.cpp | 162 ++++++++++++++++++++++++++++++++++++- 3 files changed, 163 insertions(+), 178 deletions(-) diff --git a/client/TracyCallstack.cpp b/client/TracyCallstack.cpp index 0cef9124..4c87e6c7 100644 --- a/client/TracyCallstack.cpp +++ b/client/TracyCallstack.cpp @@ -45,14 +45,6 @@ extern "C" }; #endif -#ifdef __ANDROID__ -# include -# include -# include -# include -# include -#endif - namespace tracy { @@ -527,13 +519,6 @@ static void SymbolAddressErrorCb( void* data, const char* /*msg*/, int /*errnum* CallstackSymbolData DecodeSymbolAddress( uint64_t ptr ) { CallstackSymbolData sym; -#ifdef __ANDROID__ - if( !EnsureReadable(ptr) ) - { - memset(&sym, 0, sizeof sym); - return sym; - } -#endif backtrace_pcinfo( cb_bts, ptr, SymbolAddressDataCb, SymbolAddressErrorCb, &sym ); return sym; } @@ -780,154 +765,3 @@ CallstackEntryData DecodeCallstackPtr( uint64_t ptr ) } #endif - -// The below is defined independently of TRACY_HAS_CALLSTACK because it's -// also used by TracyProfiler.cpp. -#ifdef __ANDROID__ -namespace tracy { -namespace { - -// Implementation helpers of EnsureReadable. -// This is so far only needed on Android, where it is common for libraries to be mapped -// with only executable, not readable, permissions. Typical example (line from /proc/self/maps): -/* -746b63b000-746b6dc000 --xp 00042000 07:48 35 /apex/com.android.runtime/lib64/bionic/libc.so -*/ -// See https://github.com/wolfpld/tracy/issues/125 . -// To work around this, we parse /proc/self/maps and we use mprotect to set read permissions -// on any mappings that contain symbols addresses hit by HandleSymbolCodeQuery. - -// Holds some information about a single memory mapping. -struct MappingInfo { - // Start of address range. Inclusive. - uintptr_t start_address; - // End of address range. Exclusive, so the mapping is the half-open interval - // [start, end) and its length in bytes is `end - start`. As in /proc/self/maps. - uintptr_t end_address; - // Read/Write/Executable permissions. - bool perm_r, perm_w, perm_x; -}; - -// Internal implementation helper for LookUpMapping(address). -// -// Parses /proc/self/maps returning a vector. -// /proc/self/maps is assumed to be sorted by ascending address, so the resulting -// vector is sorted by ascending address too. -std::vector ParseMappings() -{ - std::vector result; - FILE* file = fopen( "/proc/self/maps", "r" ); - if( !file ) return result; - char line[1024]; - while( fgets( line, sizeof( line ), file ) ) - { - uintptr_t start_addr; - uintptr_t end_addr; - if( sscanf( line, "%lx-%lx", &start_addr, &end_addr ) != 2 ) continue; - char* first_space = strchr( line, ' ' ); - if( !first_space ) continue; - char* perm = first_space + 1; - char* second_space = strchr( perm, ' ' ); - if( !second_space || second_space - perm != 4 ) continue; - result.emplace_back(); - auto& mapping = result.back(); - mapping.start_address = start_addr; - mapping.end_address = end_addr; - mapping.perm_r = perm[0] == 'r'; - mapping.perm_w = perm[1] == 'w'; - mapping.perm_x = perm[2] == 'x'; - } - fclose( file ); - return result; -} - -// Internal implementation helper for LookUpMapping(address). -// -// Takes as input an `address` and a -// known vector `mappings`, and returns a pointer to the MappingInfo -// describing the mapping that this address belongs to, or nullptr if -// the address isn't in `mappings`. -inline MappingInfo* LookUpMapping(std::vector& mappings, uintptr_t address) -{ - // We assume mappings to be sorted by address, as /proc/self/maps seems to be. - // Construct a MappingInfo just for the purpose of using std::lower_bound. - MappingInfo needle; - needle.start_address = address; - needle.end_address = address; - // Comparison function for std::lower_bound. Returns true if all addresses in `m1` - // are lower than all addresses in `m2`. - auto Compare = []( const MappingInfo& m1, const MappingInfo& m2 ) { - // '<=' because the address ranges are half-open intervals, [start, end). - return m1.end_address <= m2.start_address; - }; - auto iter = std::lower_bound( mappings.begin(), mappings.end(), needle, Compare ); - if( iter == mappings.end() || iter->end_address <= address) { - return nullptr; - } - return &*iter; -} - -// Internal implementation helper for EnsureReadable(address). -// -// Takes as input an `address` and returns a pointer to a MappingInfo -// describing the mapping that this address belongs to, or nullptr if -// the address isn't in any known mapping. -// -// This function is stateful and not reentrant (assumes to be called from) -// only one thread. It holds a vector of mappings parsed from /proc/self/maps. -// -// Attempts to react to mappings changes by re-parsing /proc/self/maps. -inline MappingInfo* LookUpMapping(uintptr_t address) -{ - // Static state managed by this function. Not constant, we mutate that state as - // we turn some mappings readable. Initially parsed once here, updated as needed below. - static std::vector s_mappings = ParseMappings(); - MappingInfo* mapping = LookUpMapping( s_mappings, address ); - if( mapping ) return mapping; - - // This address isn't in any known mapping. Try parsing again, maybe - // mappings changed. - s_mappings = ParseMappings(); - return LookUpMapping( s_mappings, address ); -} - -// Internal implementation helper for EnsureReadable(address). -// -// Attempts to make the specified `mapping` readable if it isn't already. -// Returns true if and only if the mapping is readable. -inline bool EnsureReadable( MappingInfo& mapping ) -{ - if( mapping.perm_r ) - { - // The mapping is already readable. - return true; - } - int prot = PROT_READ; - if( mapping.perm_w ) prot |= PROT_WRITE; - if( mapping.perm_x ) prot |= PROT_EXEC; - if( mprotect( reinterpret_cast( mapping.start_address ), - mapping.end_address - mapping.start_address, prot ) == -1 ) - { - // Failed to make the mapping readable. Shouldn't happen, hasn't - // been observed yet. If it happened in practice, we should consider - // adding a bool to MappingInfo to track this to avoid retrying mprotect - // everytime on such mappings. - return false; - } - // The mapping is now readable. Update `mapping` so the next call will be fast. - mapping.perm_r = true; - return true; -} - -} // anonymous namespace - end of internal helpers for EnsureReadable(address) - -// Attempts to set the read permission on the entire mapping containing the -// specified address. -bool EnsureReadable( uintptr_t address ) -{ - MappingInfo* mapping = LookUpMapping(address); - return mapping && EnsureReadable( *mapping ); -} - -} // namespace tracy -#endif // defined __ANDROID__ diff --git a/client/TracyCallstack.hpp b/client/TracyCallstack.hpp index 31a73272..6b5e702d 100644 --- a/client/TracyCallstack.hpp +++ b/client/TracyCallstack.hpp @@ -10,6 +10,7 @@ # include #endif + #ifdef TRACY_HAS_CALLSTACK #include @@ -110,16 +111,6 @@ static tracy_force_inline void* Callstack( int depth ) } -#endif // defined TRACY_HAS_CALLSTACK - -// The below is defined independently of TRACY_HAS_CALLSTACK because it's -// also used by TracyProfiler.cpp. -#ifdef __ANDROID__ -namespace tracy { -// Attempts to set the read permission on the entire mapping containing the -// specified address. -bool EnsureReadable( uintptr_t address ); -} // namespace tracy -#endif // defined __ANDROID__ +#endif #endif diff --git a/client/TracyProfiler.cpp b/client/TracyProfiler.cpp index 1226c776..f796fb1a 100644 --- a/client/TracyProfiler.cpp +++ b/client/TracyProfiler.cpp @@ -41,6 +41,14 @@ # include "TargetConditionals.h" #endif +#ifdef __ANDROID__ +# include +# include +# include +# include +# include +#endif + #include #include #include @@ -2889,9 +2897,162 @@ void Profiler::HandleParameter( uint64_t payload ) TracyLfqCommit; } +#ifdef __ANDROID__ +namespace { +// Implementation helpers of EnsureReadable(address). +// This is so far only needed on Android, where it is common for libraries to be mapped +// with only executable, not readable, permissions. Typical example (line from /proc/self/maps): +/* +746b63b000-746b6dc000 --xp 00042000 07:48 35 /apex/com.android.runtime/lib64/bionic/libc.so +*/ +// See https://github.com/wolfpld/tracy/issues/125 . +// To work around this, we parse /proc/self/maps and we use mprotect to set read permissions +// on any mappings that contain symbols addresses hit by HandleSymbolCodeQuery. + +// Holds some information about a single memory mapping. +struct MappingInfo { + // Start of address range. Inclusive. + uintptr_t start_address; + // End of address range. Exclusive, so the mapping is the half-open interval + // [start, end) and its length in bytes is `end - start`. As in /proc/self/maps. + uintptr_t end_address; + // Read/Write/Executable permissions. + bool perm_r, perm_w, perm_x; +}; + +// Internal implementation helper for LookUpMapping(address). +// +// Parses /proc/self/maps returning a vector. +// /proc/self/maps is assumed to be sorted by ascending address, so the resulting +// vector is sorted by ascending address too. +std::vector ParseMappings() +{ + std::vector result; + FILE* file = fopen( "/proc/self/maps", "r" ); + if( !file ) return result; + char line[1024]; + while( fgets( line, sizeof( line ), file ) ) + { + uintptr_t start_addr; + uintptr_t end_addr; + if( sscanf( line, "%lx-%lx", &start_addr, &end_addr ) != 2 ) continue; + char* first_space = strchr( line, ' ' ); + if( !first_space ) continue; + char* perm = first_space + 1; + char* second_space = strchr( perm, ' ' ); + if( !second_space || second_space - perm != 4 ) continue; + result.emplace_back(); + auto& mapping = result.back(); + mapping.start_address = start_addr; + mapping.end_address = end_addr; + mapping.perm_r = perm[0] == 'r'; + mapping.perm_w = perm[1] == 'w'; + mapping.perm_x = perm[2] == 'x'; + } + fclose( file ); + return result; +} + +// Internal implementation helper for LookUpMapping(address). +// +// Takes as input an `address` and a +// known vector `mappings`, and returns a pointer to the MappingInfo +// describing the mapping that this address belongs to, or nullptr if +// the address isn't in `mappings`. +inline MappingInfo* LookUpMapping(std::vector& mappings, uintptr_t address) +{ + // We assume mappings to be sorted by address, as /proc/self/maps seems to be. + // Construct a MappingInfo just for the purpose of using std::lower_bound. + MappingInfo needle; + needle.start_address = address; + needle.end_address = address; + // Comparison function for std::lower_bound. Returns true if all addresses in `m1` + // are lower than all addresses in `m2`. + auto Compare = []( const MappingInfo& m1, const MappingInfo& m2 ) { + // '<=' because the address ranges are half-open intervals, [start, end). + return m1.end_address <= m2.start_address; + }; + auto iter = std::lower_bound( mappings.begin(), mappings.end(), needle, Compare ); + if( iter == mappings.end() || iter->end_address <= address) { + return nullptr; + } + return &*iter; +} + +// Internal implementation helper for EnsureReadable(address). +// +// Takes as input an `address` and returns a pointer to a MappingInfo +// describing the mapping that this address belongs to, or nullptr if +// the address isn't in any known mapping. +// +// This function is stateful and not reentrant (assumes to be called from) +// only one thread. It holds a vector of mappings parsed from /proc/self/maps. +// +// Attempts to react to mappings changes by re-parsing /proc/self/maps. +inline MappingInfo* LookUpMapping(uintptr_t address) +{ + // Static state managed by this function. Not constant, we mutate that state as + // we turn some mappings readable. Initially parsed once here, updated as needed below. + static std::vector s_mappings = ParseMappings(); + MappingInfo* mapping = LookUpMapping( s_mappings, address ); + if( mapping ) return mapping; + + // This address isn't in any known mapping. Try parsing again, maybe + // mappings changed. + s_mappings = ParseMappings(); + return LookUpMapping( s_mappings, address ); +} + +// Internal implementation helper for EnsureReadable(address). +// +// Attempts to make the specified `mapping` readable if it isn't already. +// Returns true if and only if the mapping is readable. +inline bool EnsureReadable( MappingInfo& mapping ) +{ + if( mapping.perm_r ) + { + // The mapping is already readable. + return true; + } + int prot = PROT_READ; + if( mapping.perm_w ) prot |= PROT_WRITE; + if( mapping.perm_x ) prot |= PROT_EXEC; + if( mprotect( reinterpret_cast( mapping.start_address ), + mapping.end_address - mapping.start_address, prot ) == -1 ) + { + // Failed to make the mapping readable. Shouldn't happen, hasn't + // been observed yet. If it happened in practice, we should consider + // adding a bool to MappingInfo to track this to avoid retrying mprotect + // everytime on such mappings. + return false; + } + // The mapping is now readable. Update `mapping` so the next call will be fast. + mapping.perm_r = true; + return true; +} + +// Attempts to set the read permission on the entire mapping containing the +// specified address. +bool EnsureReadable( uintptr_t address ) +{ + MappingInfo* mapping = LookUpMapping(address); + return mapping && EnsureReadable( *mapping ); +} + +} // anonymous namespace +#endif // defined __ANDROID__ + void Profiler::HandleSymbolQuery( uint64_t symbol ) { #ifdef TRACY_HAS_CALLSTACK +#ifdef __ANDROID__ + // On Android it's common for code to be in mappings that are only executable + // but not readable. + if( !EnsureReadable( symbol ) ) + { + return; + } +#endif const auto sym = DecodeSymbolAddress( symbol ); SendSingleString( sym.file ); @@ -2914,7 +3075,6 @@ void Profiler::HandleSymbolCodeQuery( uint64_t symbol, uint32_t size ) // but not readable. if( !EnsureReadable( symbol ) ) { - // Non-readable address: bail here, avoid segfault below. return; } #endif