Excessive calls to iterate_phdr during exception handling

[Ryan Johnson]

On 28/05/2013 11:05 PM, Alan Modra wrote:
> On Tue, May 28, 2013 at 09:19:48PM -0400, Ryan Johnson wrote:
>> On 28/05/2013 8:47 PM, Ian Lance Taylor wrote:
>>> On Mon, May 27, 2013 at 3:20 PM, Ryan Johnson
>>> <ryan.johnson@cs.utoronto.ca> wrote:
>>>> I'm bringing the issue up here, rather than filing a bug, because I'm not
>>>> sure whether this is an oversight, a known problem that's hard to fix, or a
>>>> feature (e.g. somehow required for reliable unwinding). I suspect the
>>>> former, because _Unwind_Find_FDE tries a call to _Unwind_Find_registered_FDE
>>>> before falling back to dl_iterate_phdr, but the former never succeeds in my
>>>> trace (iterate_phdr is always called).
>>> The issue is dlclose followed by dlopen.  If we had a cache ahead of
>>> dl_iterate_phdr, we would need some way to clear out any information
>>> cached from a dlclose'd library.  Otherwise we might pick up the old
>>> information when looking up an address from a new dlopen.  So 1)
>>> locking will always be required; 2) any caching system to reduce the
>>> number of locks will require support for dlclose, somehow.  It's worth
>>> working on but there isn't going to be a simple solution.
>> I have mixed feelings on this... on the one had it would be bad to
>> risk sending unwind off to la-la land because somebody did a quick
>> dlclose/dlopen pair on code we're about to unwind through... but on
>> the other hand anybody who does a dlclose/dlopen pair on code we're
>> about to unwind through (a) is asking for trouble and (b) is
>> perfectly free to do so in spite of the mutex [1].
> Yes of course you can shoot yourself in the foot.  The mutex is there
> to stop the glibc dl_iterate_phdr list traversal running awry when
> dlopen/dlclose happens in another thread.  To be clear, I'm talking
> about a dlclose on an object that your thread doesn't access.  Such a
> dlclose shouldn't affect your thread in any way.  But if glibc's
> list of loaded objects was allowed to change while your thread was
> running dl_iterate_phdr, then dl_iterate_phdr could potentially read
> freed list entries.
Understood. It's only safe to populate a cache with header info if you 
hold the loader mutex.

... but once you have that cache, the rest of unwind is up for grabs...

For example, it should be reasonably safe to let 
__cxa_allocate_exception call dl_iterate_phdr in order to build a list 
of object headers valid at the time unwind begins. It already calls 
malloc, so allocating space for a cache (holding pointers to at most a 
few dozen object headers) wouldn't be so terrible; __cxa_free_exception 
could release the space after the dust settles. In order to optimize 
non-throw uses of unwinding, it might make sense to build the cache 
somewhere besides __cxa_allocate_exception, but the basic concept 
doesn't change. During unwind, _Unwind_find_FDE could work exclusively 
from the cache instead of calling dl_iterate_phdr, reducing call count 
for the latter from 4-6 per stack frame unwound to one per throw, which 
seems like a pretty big win [1].

A more aggressive optimization would stash the the header cache in TLS 
between uses, rebuilding only if dladd/dlsub count changes. That would 
require a lock-free method to access the two counts, however, which I 
don't think currently exists. There would be a race between reading the 
two variables, and another between read and use of either variable, but 
the whole race window arises during a time when any change that mattered 
would have dire consequences whether the cache is stale or not.

[1] I'd also move the call to _Unwind_Find_registered_FDE after the 
cache check, so it becomes a fallback (thus avoiding the libgcc mutex 
lock in the common case of "newer" executables and shared libraries). 
Unless by some cruel twist of history, there exist out there objects 
that register their FDEs manually *and* provide .eh_frame whose invalid 
entries must be ignored?

Ryan