[valgrind] [Bug 473944] Handle mold linker split RW PT_LOAD segments correctly
https://bugs.kde.org/show_bug.cgi?id=473944 Philippe Waroquiers changed: What|Removed |Added Status|REPORTED|RESOLVED Resolution|--- |FIXED --- Comment #5 from Philippe Waroquiers --- Fixed in c0b2c786d -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 473944] Handle mold linker split RW PT_LOAD segments correctly
https://bugs.kde.org/show_bug.cgi?id=473944 --- Comment #4 from Paul Floyd --- 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. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 473944] Handle mold linker split RW PT_LOAD segments correctly
https://bugs.kde.org/show_bug.cgi?id=473944 Philippe Waroquiers changed: What|Removed |Added CC||philippe.waroquiers@skynet. ||be --- Comment #3 from Philippe Waroquiers --- (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 00-1f 2097152 - SmFixed > --54635:1: aspacem 1: file 20-200fff4096 r > d=0x6d8ca7de696e301b i=2440208 o=0 (1,65) > --54635:1: aspacem 2: file 201000-201fff4096 r-x-- > d=0x6d8ca7de696e301b i=2440208 o=0 (1,65) > --54635:1: aspacem 3: file 202000-203fff8192 rw--- > d=0x6d8ca7de696e301b i=2440208 o=0 (1,65) > --54635:1: aspacem 4: RSVN 204000-0003ff 61m - SmFixed > --54635:1: aspacem 5: file 000400-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-000401dfff4096 rw--- > d=0x28a8dde4190bc5c i=1049059 o=110592 (2,134) > --54635:1: aspacem 8: file 000401e000-000401efff4096 rw--- > d=0x28a8dde4190bc5c i=1049059 o=110592 (2,134) > --54635:1: aspacem 9: anon 000401f000-0004014096 rw--- > --54635:1: aspacem 10: anon 000402-0004020fff4096 rwx-- > --54635:1: aspacem 11: RSVN 0004021000-000481 8384512 - SmLower > --54635:1: aspacem 12: 000482-0037ff823m > --54635:1: aspacem 13: FILE 003800-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-0038142fff4096 r-x-- > d=0x696e301b i=1844040 o=1314816 (0,4) > --54635:1: aspacem 16: FILE 0038143000-003821bfff 32 r-x-- > d=0x696e301b i=1844040 o=1318912 (0,4) > --54635:1: aspacem 17: FILE 003821c000-003821cfff4096 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 off0x0040 vaddr 0x00200040 paddr > 0x00200040 align 2**3 > filesz 0x0268 memsz 0x0268 flags r-- > INTERP off0x02a8 vaddr 0x002002a8 paddr > 0x002002a8 align 2**0 > filesz 0x0015 memsz 0x0015 flags r-- > LOAD off0x vaddr 0x0020 paddr > 0x0020 align 2**12 > filesz 0x08ec memsz 0x08ec flags r-- > LOAD off0x08f0 vaddr 0x002018f0 paddr > 0x002018f0 align 2**12 > filesz 0x0590 memsz 0x0590 flags r-x > LOAD off0x0e80 vaddr 0x00202e80 paddr > 0x00202e80 align 2**12 > filesz 0x0180 memsz 0x0180 flags rw- > LOAD off0x1000 vaddr 0x00203000 paddr > 0x00203000 align 2**12 > filesz 0x0098 memsz 0x00c8 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 -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 473944] Handle mold linker split RW PT_LOAD segments correctly
https://bugs.kde.org/show_bug.cgi?id=473944 --- Comment #2 from Paul Floyd --- 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 00-1f 2097152 - SmFixed --54635:1: aspacem 1: file 20-200fff4096 r d=0x6d8ca7de696e301b i=2440208 o=0 (1,65) --54635:1: aspacem 2: file 201000-201fff4096 r-x-- d=0x6d8ca7de696e301b i=2440208 o=0 (1,65) --54635:1: aspacem 3: file 202000-203fff8192 rw--- d=0x6d8ca7de696e301b i=2440208 o=0 (1,65) --54635:1: aspacem 4: RSVN 204000-0003ff 61m - SmFixed --54635:1: aspacem 5: file 000400-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-000401dfff4096 rw--- d=0x28a8dde4190bc5c i=1049059 o=110592 (2,134) --54635:1: aspacem 8: file 000401e000-000401efff4096 rw--- d=0x28a8dde4190bc5c i=1049059 o=110592 (2,134) --54635:1: aspacem 9: anon 000401f000-0004014096 rw--- --54635:1: aspacem 10: anon 000402-0004020fff4096 rwx-- --54635:1: aspacem 11: RSVN 0004021000-000481 8384512 - SmLower --54635:1: aspacem 12: 000482-0037ff823m --54635:1: aspacem 13: FILE 003800-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-0038142fff4096 r-x-- d=0x696e301b i=1844040 o=1314816 (0,4) --54635:1: aspacem 16: FILE 0038143000-003821bfff 32 r-x-- d=0x696e301b i=1844040 o=1318912 (0,4) --54635:1: aspacem 17: FILE 003821c000-003821cfff4096 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 off0x0040 vaddr 0x00200040 paddr 0x00200040 align 2**3 filesz 0x0268 memsz 0x0268 flags r-- INTERP off0x02a8 vaddr 0x002002a8 paddr 0x002002a8 align 2**0 filesz 0x0015 memsz 0x0015 flags r-- LOAD off0x vaddr 0x0020 paddr 0x0020 align 2**12 filesz 0x08ec memsz 0x08ec flags r-- LOAD off0x08f0 vaddr 0x002018f0 paddr 0x002018f0 align 2**12 filesz 0x0590 memsz 0x0590 flags r-x LOAD off0x0e80 vaddr 0x00202e80 paddr 0x00202e80 align 2**12 filesz 0x0180 memsz 0x0180 flags rw- LOAD off0x1000 vaddr 0x00203000 paddr 0x00203000 align 2**12 filesz 0x0098 memsz 0x00c8 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. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 473944] Handle mold linker split RW PT_LOAD segments correctly
https://bugs.kde.org/show_bug.cgi?id=473944 --- Comment #1 from Paul Floyd --- (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. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 473944] Handle mold linker split RW PT_LOAD segments correctly
https://bugs.kde.org/show_bug.cgi?id=473944 Paul Floyd changed: What|Removed |Added CC||pjfl...@wanadoo.fr -- You are receiving this mail because: You are watching all bug changes.
