https://bugs.kde.org/show_bug.cgi?id=452802
--- Comment #16 from Paul Floyd <pjfl...@wanadoo.fr> ---
(In reply to Mark Wielaard from comment #15)
For macos I don't know if any of these changes are applicable so I wanted to
keep the old code if #if blocks.
I'll clean up unrelated changes and push them separately.
> I wish I understood this part of the code better. But I also struggling
> a bit. Let me try to talk us through the patch to see if we agree on
> the why/what:
>
> di_notify_mmap is triggered by an actual mmap (and some startup code,
> for valgrind tools itself?). The problem we are dealing with is when
> "enough" or "the right" parts of the file have been mapped by ld.so to
> start searching for the debuginfo and setting up the offsets to map
> debug addresses to addresses in memory where the code is mapped.
Essentially that's it. "enough" is when the RX and the RW PT_LOAD(s) have been
seen.
As I understand it, di_notify_mmap can be called in 2 situations
"HOST"
1a. For the tool exe and tool/core shared libs. These are already mmap'd when
the host starts so we look at something like the /proc filesystem to get the
mapping after the event and build up the NSegments from that.
1b. Then the host loads ld.so and the guest exe. This is done in the sequence
load_client -> VG_(do_exec) -> VG_(do_exec_inner) -> exe_handlers->load_fn ( ==
VG_(load_ELF) ). This does the mmap'ing and creats the associated NSegments.
The NSegments may get merged, so there could be fewer mmaps and PT_LOADs than
there are NSegemnts.
This is around line 1893 of m_main.c:
for (i = 0; i < n_seg_starts; i++) {
anu.ull = VG_(di_notify_mmap)( seg_starts[i], True/*allow_SkFileV*/,
-1/*Don't use_fd*/);
"GUEST"
2. When the guest loads any further shared libs (libc, other dependencies,
dlopens).
There are a few variations for syswraps/platforms, but the one that we are
concerned with is syswrap-generic.c:
/* Load symbols? */
di_handle = VG_(di_notify_mmap)( (Addr)sr_Res(sres),
False/*allow_SkFileV*/, (Int)arg5 );
In this case the NSegment could possibly be merged, but that is irrelevant
because di_notify_mmap is being called based directy on the mmap result.
> The goal of di_notify_mmap is to reach di_notify_ACHIEVE_ACCEPT_STATE
> at which point the actual debuginfo is read. This is guarded by:
>
> if (di->fsm.have_rx_map && di->fsm.have_rw_map)
>
> You patch changes that to:
>
> if (di->fsm.have_rx_map &&
> rw_load_count >= 1 &&
> di->fsm.have_rw_map == rw_load_count)
>
> Now have_rw_map is slightly misnamed, but ok.
Yes, count_rw_map would be better.
>
> The rw_load_count comes from the new check_elf_and_get_rw_loads.
> Which tries to get an elf image from the file descriptor and traverses
> the phdrs looking for PT_LOAD RW mappings, with this funny counter
> example:
>
> /*
> * Hold your horses
> * Just because The ELF file contains 2 RW PT_LOAD segments it
> * doesn't mean that Valgrind will also make 2 calls to
> * VG_(di_notify_mmap). If the stars are all aligned
> * (which usually means that the ELF file is the client
> * executable with the segment offset for the
> * second PT_LOAD falls exactly on 0x1000) then the NSegements
> * will get merged and VG_(di_notify_mmap) only gets called once. */
> if (*rw_load_count == 2 &&
> ehdr_m.e_type == ET_EXEC &&
> a_phdr.p_offset == VG_PGROUNDDN(a_phdr.p_offset) )
> {
> *rw_load_count = 1;
> }
>
> I assume in that case ld.so merges the mmap request into one?
No it's Valgrind that merges the NSegements, as above.
> I cannot say I really understand everything here, but I understand why
> you want to delay to actual debuginfo loading till all mapping are
> there.
>
> Is the above an accurate description of the proposed patch logic?
Yes, that's about it.
Lastly, I thought that the code to handle either 1 or 2 segments in
ML_(read_elf_debug_info) is a bit messy.
Do you have any ideas how it could be done more cleanly?
--
You are receiving this mail because:
You are watching all bug changes.
