Re: [PATCH] libdwfl: Special case linux-gate/vdso.so in core_file_read_eagerly.

[Mark Wielaard <mjw at redhat dot com>]

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 <mjw@redhat.com>
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 <mjw@redhat.com>

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  <mjw@redhat.com>
+
+	* core-file.c (core_file_read_eagerly): Special case small images.
+
 2015-01-26  Mark Wielaard  <mjw@redhat.com>
 
 	* 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)