This is the mail archive of the
mailing list for the elfutils project.
Re: [patch 2/2 v4] Fix loading core files without build-ids
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Wed, 29 May 2013 17:57:04 +0200
- Subject: Re: [patch 2/2 v4] Fix loading core files without build-ids
On Wed, 29 May 2013 11:50:26 +0200, Mark Wielaard wrote:
> On Tue, 2013-05-28 at 20:36 +0200, Jan Kratochvil wrote:
> > But when we talk about it in general, as a new patchset, I rather find a bug
> > dropping the path, when core file already contains the absolute pathname why
> > to hide it from the user?
> Right, if we can guarantee that path is the real one and not just an
> arbitrary guess.
It always was the real pathname - why it would not be? Or what have I forgot
> > $ readelf -n test-core.core
> > Start End Page Offset
> > 0x00000037ef400000 0x00000037ef420000 0x0000000000000000
> > /usr/lib64/ld-2.16.so
> Yes that is nice, we should have support for that too, in the future, in
> some separate patch. Found the ELF NOTE description here:
> Don't know if there is a more "official" documentation.
> Does gdb/gcore also add this NT_FILE note now?
Yes, for both 'info proc mappings' and 'gcore':
Author: Tom Tromey <firstname.lastname@example.org>
Date: Fri Dec 14 15:30:25 2012 +0000
> > > Actually now that I do understand, that is kind of what the comment above
> > > already says. I just didn't immediately made the connection with the
> > > offset/fixup being static. Is fixup always positive?
> > At least in the supplied testcase it is negative.
> > In fact I did not spent time thinking if it is positive or negative,
> > I was following the logic which address is safe and which unsafe above.
> OK, then I am wondering about the first check before the fixup.
+ /* Try to match up DYN_VADDR against L_LD as found in link map.
+ Segments sniffing may guess invalid address as the first read-only memory
+ mapping may not be dumped to the core file (if ELF headers are not dumped)
+ and the ELF header is dumped first with the read/write mapping of the same
+ file at higher addresses. */
I have one such example I have seen where it is helpful. But the principle
seems correct to me no matter if it is positive or negative.
> > + if (module_start <= module->l_ld && module->l_ld < module_end)
> Should the fixup not always be calculated and then the next check takes
> care of everything?
I do not find that safe; this could match unrelated L_LD from the other end of
address space just due to a "luck" it has the same '% PAGE_SIZE' offset.
IIUC MODULE_START and MODULE_END are always safely located inside the segment
(considering they have BIAS already added); the segment address may be wrong
but it only matters there cannot be other shared library located at the same
address range. L_LD is also always safe as it comes from link map. Therefore
the first conditional is always safe:
if (module_start <= module->l_ld && module->l_ld < module_end)
I cannot much imagine how the second conditional after FIXUP adjustment
&& module_start + fixup <= module->l_ld
&& module->l_ld < module_end + fixup)
could fail but it is just a sanity check of FIXUP.
This is why I believe the first conditional should be there; the second
conditional may be redundant but it cannot hurt.
Or do you see some inconsistency in my safe / unsafe considerations above?
It is a bit pity to spend time on all of this when we have NT_FILE already.
Just the current code did not work for my unwinder test cases and with some of
the fixes I was accused of regressing elfutils so I just try to make a feeling
there are no regressions after all the patches of mine.
> > + GElf_Addr fixup = module->l_ld - dyn_vaddr;
> > + if ((fixup & (dwfl->segment_align - 1)) == 0
> > + && module_start + fixup <= module->l_ld
> > + && module->l_ld < module_end + fixup)
> Since the fixup could just be zero?
FIXUP will be in 99% of cases zero, what's the problem with it? I did not get
your comment here.
> There are other ways core files are generated than with the latest kernel
> version, like with gdb/gcore, which we also like to handle.
gdb/gcore does produce NT_FILE. :-)
> Looks good to me. But please merge this commit and the code movement
> commit into one, since those are really one change.
Yes, sure, it would break git bisect as it does not build with just the first
Not checked in yet until the discussion gets finished.