| Summary: | Handle mold linker split RW PT_LOAD segments correctly | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | Philippe Waroquiers <philippe.waroquiers> |
| Component: | general | Assignee: | Julian Seward <jseward> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | philippe.waroquiers, pjfloyd |
| Priority: | NOR | ||
| Version First Reported In: | 3.21.0 | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
| Attachments: | change the condition to detect 2 PT_LOADs will be merged | ||
|
Description
Philippe Waroquiers
2023-08-30 13:33:16 UTC
(In reply to Philippe Waroquiers from comment #0) I really don't understand what linker writers are doing. First they split the RW PT_LOAD into 2 which I thought was for performance, putting infrequently accessed stuff in a separate page from the frequently accessed stuff. But that uses more memory, so they allow the segments to overlap pages. I don't see what the benefit of having split+overlapped. Why not just have the one segment? > So, clearly we have 2 different segments. > The condition > 'a_phdr.p_offset == VG_PGROUNDDN(a_phdr.p_offset)' > in readelf.c to consider that the 2nd segment is glued with the first one is > very bizarre. > The above condition only ensures that p_offset (offset in the file) is on a > 0x1000 page boundary ???? As I understand it the split RW PT_LOAD segments used to always start on page boundaries, meaning that there would be padding after the first one. I think padding is required between segments with different protection since the granularity of the protection is the page size. Without overlapping the only time that VG_(di_notify_mmap) would only be called once for two PT_LOAD segments merged into one NSegement is if the first one ends exactly before a page boundary. If looks like overlapping segments started to be allowed back around 2019 https://reviews.llvm.org/D64906 https://reviews.llvm.org/rG0264950697e54505e5fd266b364c83e294fc86d9 I don't know what default options linkers are using, but id does look like they are starting to default to allowing overlapping. See also this bugzilla item which might be related to this https://bugs.kde.org/show_bug.cgi?id=472409 > The condition to determine that the first rw segment will be merged by > aspacemgr with the second > should be something like: > virtaddr seg1 + (some size) == virtaddr seg2 > ???? > The '(some size)' to use is not very clear to me. > In the readelf output, we have the FileSiz and the MemSiz > It looks like the mapping is done with FileSiz rounded up to the page size. No it's not very clear and there are numerous places where the code is impacted. With the patch I get one regression failure
helgrind/tests/pth_destroy_cond (stderr)
There is missing source information, presumably due to a failure reading debuginfo.
The first aspacem map is
--54635:1: aspacem <<< SHOW_SEGMENTS: Memory layout at client startup (32 segments)
--54635:1: aspacem 3 segment names in 3 slots
--54635:1: aspacem freelist is empty
--54635:1: aspacem (0,4,7) /usr/home/paulf/scratch/valgrind/none/none-amd64-freebsd
--54635:1: aspacem (1,65,6) /usr/home/paulf/scratch/valgrind/helgrind/tests/pth_destroy_cond
--54635:1: aspacem (2,134,7) /libexec/ld-elf.so.1
--54635:1: aspacem 0: RSVN 0000000000-00001fffff 2097152 ----- SmFixed
--54635:1: aspacem 1: file 0000200000-0000200fff 4096 r---- d=0x6d8ca7de696e301b i=2440208 o=0 (1,65)
--54635:1: aspacem 2: file 0000201000-0000201fff 4096 r-x-- d=0x6d8ca7de696e301b i=2440208 o=0 (1,65)
--54635:1: aspacem 3: file 0000202000-0000203fff 8192 rw--- d=0x6d8ca7de696e301b i=2440208 o=0 (1,65)
--54635:1: aspacem 4: RSVN 0000204000-0003ffffff 61m ----- SmFixed
--54635:1: aspacem 5: file 0004000000-0004006fff 28672 r---- d=0x28a8dde4190bc5c i=1049059 o=0 (2,134)
--54635:1: aspacem 6: file 0004007000-000401cfff 90112 r-x-- d=0x28a8dde4190bc5c i=1049059 o=24576 (2,134)
--54635:1: aspacem 7: file 000401d000-000401dfff 4096 rw--- d=0x28a8dde4190bc5c i=1049059 o=110592 (2,134)
--54635:1: aspacem 8: file 000401e000-000401efff 4096 rw--- d=0x28a8dde4190bc5c i=1049059 o=110592 (2,134)
--54635:1: aspacem 9: anon 000401f000-000401ffff 4096 rw---
--54635:1: aspacem 10: anon 0004020000-0004020fff 4096 rwx--
--54635:1: aspacem 11: RSVN 0004021000-000481ffff 8384512 ----- SmLower
--54635:1: aspacem 12: 0004820000-0037ffffff 823m
--54635:1: aspacem 13: FILE 0038000000-00380abfff 704512 r---- d=0x696e301b i=1844040 o=0 (0,4)
--54635:1: aspacem 14: FILE 00380ac000-0038141fff 614400 r-x-- d=0x696e301b i=1844040 o=700416 (0,4)
--54635:1: aspacem 15: file 0038142000-0038142fff 4096 r-x-- d=0x696e301b i=1844040 o=1314816 (0,4)
--54635:1: aspacem 16: FILE 0038143000-003821bfff 888832 r-x-- d=0x696e301b i=1844040 o=1318912 (0,4)
--54635:1: aspacem 17: FILE 003821c000-003821cfff 4096 rw--- d=0x696e301b i=1844040 o=2203648 (0,4)
objdump:
paulf> objdump -p pth_destroy_cond
pth_destroy_cond: file format elf64-x86-64-freebsd
Program Header:
PHDR off 0x0000000000000040 vaddr 0x0000000000200040 paddr 0x0000000000200040 align 2**3
filesz 0x0000000000000268 memsz 0x0000000000000268 flags r--
INTERP off 0x00000000000002a8 vaddr 0x00000000002002a8 paddr 0x00000000002002a8 align 2**0
filesz 0x0000000000000015 memsz 0x0000000000000015 flags r--
LOAD off 0x0000000000000000 vaddr 0x0000000000200000 paddr 0x0000000000200000 align 2**12
filesz 0x00000000000008ec memsz 0x00000000000008ec flags r--
LOAD off 0x00000000000008f0 vaddr 0x00000000002018f0 paddr 0x00000000002018f0 align 2**12
filesz 0x0000000000000590 memsz 0x0000000000000590 flags r-x
LOAD off 0x0000000000000e80 vaddr 0x0000000000202e80 paddr 0x0000000000202e80 align 2**12
filesz 0x0000000000000180 memsz 0x0000000000000180 flags rw-
LOAD off 0x0000000000001000 vaddr 0x0000000000203000 paddr 0x0000000000203000 align 2**12
filesz 0x0000000000000098 memsz 0x00000000000000c8 flags rw-
And the trace when running with a couple more I added
--56884-- ++*rw_load_count to 2 for /usr/home/paulf/scratch/valgrind/helgrind/tests/pth_destroy_cond
--56884-- offset 1000 offset roundup 1000
--56884-- prev + size 203e80 addr 203000
If I change the condition to
if (previous_rw_a_phdr.p_memsz > 0 &&
ehdr_m.e_type == ET_EXEC &&
previous_rw_a_phdr.p_vaddr + previous_rw_a_phdr.p_filesz == a_phdr.p_vaddr)
then it works.
(In reply to Paul Floyd from comment #2) > With the patch I get one regression failure > helgrind/tests/pth_destroy_cond (stderr) > > There is missing source information, presumably due to a failure reading > debuginfo. > > The first aspacem map is > > --54635:1: aspacem <<< SHOW_SEGMENTS: Memory layout at client startup (32 > segments) > --54635:1: aspacem 3 segment names in 3 slots > --54635:1: aspacem freelist is empty > --54635:1: aspacem (0,4,7) > /usr/home/paulf/scratch/valgrind/none/none-amd64-freebsd > --54635:1: aspacem (1,65,6) > /usr/home/paulf/scratch/valgrind/helgrind/tests/pth_destroy_cond > --54635:1: aspacem (2,134,7) /libexec/ld-elf.so.1 > --54635:1: aspacem 0: RSVN 0000000000-00001fffff 2097152 ----- SmFixed > --54635:1: aspacem 1: file 0000200000-0000200fff 4096 r---- > d=0x6d8ca7de696e301b i=2440208 o=0 (1,65) > --54635:1: aspacem 2: file 0000201000-0000201fff 4096 r-x-- > d=0x6d8ca7de696e301b i=2440208 o=0 (1,65) > --54635:1: aspacem 3: file 0000202000-0000203fff 8192 rw--- > d=0x6d8ca7de696e301b i=2440208 o=0 (1,65) > --54635:1: aspacem 4: RSVN 0000204000-0003ffffff 61m ----- SmFixed > --54635:1: aspacem 5: file 0004000000-0004006fff 28672 r---- > d=0x28a8dde4190bc5c i=1049059 o=0 (2,134) > --54635:1: aspacem 6: file 0004007000-000401cfff 90112 r-x-- > d=0x28a8dde4190bc5c i=1049059 o=24576 (2,134) > --54635:1: aspacem 7: file 000401d000-000401dfff 4096 rw--- > d=0x28a8dde4190bc5c i=1049059 o=110592 (2,134) > --54635:1: aspacem 8: file 000401e000-000401efff 4096 rw--- > d=0x28a8dde4190bc5c i=1049059 o=110592 (2,134) > --54635:1: aspacem 9: anon 000401f000-000401ffff 4096 rw--- > --54635:1: aspacem 10: anon 0004020000-0004020fff 4096 rwx-- > --54635:1: aspacem 11: RSVN 0004021000-000481ffff 8384512 ----- SmLower > --54635:1: aspacem 12: 0004820000-0037ffffff 823m > --54635:1: aspacem 13: FILE 0038000000-00380abfff 704512 r---- > d=0x696e301b i=1844040 o=0 (0,4) > --54635:1: aspacem 14: FILE 00380ac000-0038141fff 614400 r-x-- > d=0x696e301b i=1844040 o=700416 (0,4) > --54635:1: aspacem 15: file 0038142000-0038142fff 4096 r-x-- > d=0x696e301b i=1844040 o=1314816 (0,4) > --54635:1: aspacem 16: FILE 0038143000-003821bfff 888832 r-x-- > d=0x696e301b i=1844040 o=1318912 (0,4) > --54635:1: aspacem 17: FILE 003821c000-003821cfff 4096 rw--- > d=0x696e301b i=1844040 o=2203648 (0,4) > > objdump: > paulf> objdump -p pth_destroy_cond > > > pth_destroy_cond: file format elf64-x86-64-freebsd > > Program Header: > PHDR off 0x0000000000000040 vaddr 0x0000000000200040 paddr > 0x0000000000200040 align 2**3 > filesz 0x0000000000000268 memsz 0x0000000000000268 flags r-- > INTERP off 0x00000000000002a8 vaddr 0x00000000002002a8 paddr > 0x00000000002002a8 align 2**0 > filesz 0x0000000000000015 memsz 0x0000000000000015 flags r-- > LOAD off 0x0000000000000000 vaddr 0x0000000000200000 paddr > 0x0000000000200000 align 2**12 > filesz 0x00000000000008ec memsz 0x00000000000008ec flags r-- > LOAD off 0x00000000000008f0 vaddr 0x00000000002018f0 paddr > 0x00000000002018f0 align 2**12 > filesz 0x0000000000000590 memsz 0x0000000000000590 flags r-x > LOAD off 0x0000000000000e80 vaddr 0x0000000000202e80 paddr > 0x0000000000202e80 align 2**12 > filesz 0x0000000000000180 memsz 0x0000000000000180 flags rw- > LOAD off 0x0000000000001000 vaddr 0x0000000000203000 paddr > 0x0000000000203000 align 2**12 > filesz 0x0000000000000098 memsz 0x00000000000000c8 flags rw- > > And the trace when running with a couple more I added > --56884-- ++*rw_load_count to 2 for > /usr/home/paulf/scratch/valgrind/helgrind/tests/pth_destroy_cond > --56884-- offset 1000 offset roundup 1000 > --56884-- prev + size 203e80 addr 203000 > > If I change the condition to > > if (previous_rw_a_phdr.p_memsz > 0 && > ehdr_m.e_type == ET_EXEC && > previous_rw_a_phdr.p_vaddr + previous_rw_a_phdr.p_filesz > == a_phdr.p_vaddr) > > then it works. Thanks for the testing. The above condition also works for the executables linked by mold 1.5.1 in my setup (RHEL 8.6) (in my case, the condition has to ensure the decrement is not done as the 2 segments are not merged). I will finalize the patch (e.g. to put some traces corresponding to what you added) and push. Thanks Philippe I also think that it's better to have less hard coded assumptions about the number of PT_LOAD segments. This makes one part more flexible. Fixed in c0b2c786d |