On Thu, 2015-03-19 at 13:51 -0700, Roland McGrath wrote: > The XXX comment right above there says what we really want. It should have > the read-from-core option in its back pocket and do that only when locating > a disk file by build-id failed. I've lost track now of how much > refactoring would be required to do that.
Yes, that would be nice. But it looks like a fairly large rewrite of dwfl_core_file_report and dwfl_build_id_find_elf. Because the second is called "lazily" at a much later time, we'll have to keep track of what dwfl_core_file_report already did so dwfl_build_id_find_elf can pick it up later. > At least in Fedora/RH-flavored distros the file generally is available on > disk (/lib/modules/V-R.A/vdso/*.so). But I guess you still don't have a > pointer to that keyed on build-id unless you've installed the debuginfo > package. Yes, but only on newer versions, not on other distros and for some reason not for linux-gate, only for "real" vdsos. > (I said years ago that someone should integrate some build-id > knowledge magic into rpm or something, so that you can readily look up by > build-id every binary installed via your package system.) It is still a good idea. > But I can't think of a real downside to using the in-core image when it is > the whole file. That logic applies at least as well when the data is > mmap'd, so the core->map_address check should move before the build-id > check, shouldn't it? In the case we mmap'd the code at the top of the function would already have triggered and returned the ELF image because the whole file would already been read in/mmap'd in memory. So if we get to the build-id check we know that either the file is big and incomplete, or not eagerly read in already. It is the second case we want to check for. So the core->map_address check should not be moved before the build-id check, because we still only want to use the (big) mmap'd (partial) memory when no build-id is found. > And I think you should rewrite the comment: > > if (whole > MAX_EAGER_COST && mod->build_id_len > 0) > /* We can't cheaply read the whole file here, so we'd > be using a partial file. But there is a build ID that could > help us find the whole file, which might be more useful than > what we have. We'll just rely on that. */ > return false; That is nicer. I used that. Thanks, Mark
commit 414e04c90f1a7d1e18b76e849ffb763e5af5bdc7 Author: Mark Wielaard <[email protected]> Date: Mon Mar 16 16:52:04 2015 +0100 libdwfl: Special case core_file_read_eagerly for small ELF images. Small ELF images, like linux-gate or linux-vdso, might be available in the core file, but not on disk, even if we have a build-id. If the whole image is small enough try to read them in from the core file to make sure symbols and unwind information are always available for them. We would already map them in if the core file was opened with ELF_C_READ_MMAP. https://bugzilla.redhat.com/show_bug.cgi?id=1129756 Signed-off-by: Mark Wielaard <[email protected]> diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog index d40dbae..2cb74a4 100644 --- a/libdwfl/ChangeLog +++ b/libdwfl/ChangeLog @@ -1,3 +1,7 @@ +2015-03-16 Mark Wielaard <[email protected]> + + * core-file.c (core_file_read_eagerly): Special case small images. + 2015-01-26 Mark Wielaard <[email protected]> * dwfl_module_getdwarf.c (find_symtab): Explicitly clear symdata, diff --git a/libdwfl/core-file.c b/libdwfl/core-file.c index 50031ae..1f14c4b 100644 --- a/libdwfl/core-file.c +++ b/libdwfl/core-file.c @@ -1,5 +1,5 @@ /* Core file handling. - Copyright (C) 2008-2010, 2013 Red Hat, Inc. + Copyright (C) 2008-2010, 2013, 2015 Red Hat, Inc. This file is part of elfutils. This file is free software; you can redistribute it and/or modify @@ -212,10 +212,11 @@ core_file_read_eagerly (Dwfl_Module *mod, requires find_elf hook re-doing the magic to fall back if no file found */ - if (mod->build_id_len > 0) - /* There is a build ID that could help us find the whole file, - which might be more useful than what we have. - We'll just rely on that. */ + if (whole > MAX_EAGER_COST && mod->build_id_len > 0) + /* We can't cheaply read the whole file here, so we'd + be using a partial file. But there is a build ID that could + help us find the whole file, which might be more useful than + what we have. We'll just rely on that. */ return false; if (core->map_address != NULL)
