Bug 473944 - Handle mold linker split RW PT_LOAD segments correctly
Summary: Handle mold linker split RW PT_LOAD segments correctly
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (other bugs)
Version First Reported In: 3.21.0
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-08-30 13:33 UTC by Philippe Waroquiers
Modified: 2023-09-01 19:06 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
change the condition to detect 2 PT_LOADs will be merged (4.16 KB, text/plain)
2023-08-30 13:33 UTC, Philippe Waroquiers
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Waroquiers 2023-08-30 13:33:16 UTC
Created attachment 161282 [details]
change the condition to detect 2 PT_LOADs will be merged

This is a follow-up/similar problem as reported and fixed in bug 452802.

valgrind could not load the debug info of the main executable.
The problem is the same as 452802. The fix of 452802 does not work as the logic to detect that
the 2 PT_LOAD segments are different currently wrongly concludes that the 2 segments can be combined.

Here are the details (with some remarks/comments prefixed with #).
The attached patch solves the problem on my setup (RHEL 8.6, ld-2.28, mold 1.5.1), but as this completely changes
the condition to consider the segments to be mergeable, would be nice to (re-)validate this e.g. with lld
or other setups where split PT_LOAD are produced.

ELF Header:
  Magic:   7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00 
  Class:                             ELF64
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              EXEC (Executable file)
  Machine:                           Advanced Micro Devices X86-64
  Version:                           0x1
  Entry point address:               0x25e000
  Start of program headers:          64 (bytes into file)
  Start of section headers:          214499808 (bytes into file)
  Flags:                             0x0
  Size of this header:               64 (bytes)
  Size of program headers:           56 (bytes)
  Number of program headers:         13
  Size of section headers:           64 (bytes)
  Number of section headers:         64
  Section header string table index: 49

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .interp           PROGBITS         0000000000200318  00000318
       000000000000001c  0000000000000000   A       0     0     1
  [ 2] .note.gnu.build-i NOTE             0000000000200334  00000334
       0000000000000024  0000000000000000   A       0     0     4
  [ 3] .note.ABI-tag     NOTE             0000000000200358  00000358
       0000000000000020  0000000000000000   A       0     0     4
  [ 4] .hash             HASH             0000000000200378  00000378
       0000000000007f60  0000000000000004   A       6     0     4
  [ 5] .gnu.hash         GNU_HASH         00000000002082d8  000082d8
       00000000000002b8  0000000000000000   A       6     0     8
  [ 6] .dynsym           DYNSYM           0000000000208590  00008590
       0000000000017e08  0000000000000018   A       7     1     8
  [ 7] .dynstr           STRTAB           0000000000220398  00020398
       000000000001999a  0000000000000000   A       0     0     1
  [ 8] .gnu.version      VERSYM           0000000000239d32  00039d32
       0000000000001fd6  0000000000000002   A       6     0     2
  [ 9] .gnu.version_r    VERNEED          000000000023bd08  0003bd08
       00000000000002c0  0000000000000000   A       7     9     8
  [10] .rela.dyn         RELA             000000000023bfc8  0003bfc8
       0000000000001188  0000000000000018   A       6     0     8
  [11] .rela.plt         RELA             000000000023d150  0003d150
       0000000000013110  0000000000000018   A       6    35     8
  [12] .plt              PROGBITS         0000000000251000  00051000
       000000000000cb80  0000000000000000  AX       0     0     16
  [13] .plt.got          PROGBITS         000000000025db80  0005db80
       0000000000000230  0000000000000000  AX       0     0     16
  [14] .fini             PROGBITS         000000000025ddb0  0005ddb0
       000000000000000d  0000000000000000  AX       0     0     4
  [15] .init             PROGBITS         000000000025ddc0  0005ddc0
       0000000000000020  0000000000000000  AX       0     0     4
  [16] .text             PROGBITS         000000000025e000  0005e000
       0000000007bce0d4  0000000000000000  AX       0     0     4096
  [17] google_malloc     PROGBITS         0000000007e2c100  07c2c100
       0000000000001f1a  0000000000000000  AX       0     0     64
  [18] malloc_hook       PROGBITS         0000000007e2e020  07c2e020
       0000000000000685  0000000000000000  AX       0     0     16
  [19] text.hot          PROGBITS         0000000007e2e6b0  07c2e6b0
       00000000001b9370  0000000000000000  AX       0     0     16
  [20] text.unlikely     PROGBITS         0000000007fe7a20  07de7a20
       0000000001f7f600  0000000000000000  AX       0     0     16
  [21] .eh_frame         PROGBITS         0000000009f68000  09d68000
       00000000011bdc98  0000000000000000   A       0     0     8
  [22] .eh_frame_hdr     PROGBITS         000000000b125c98  0af25c98
       000000000028a734  0000000000000000   A       0     0     4
  [23] .gcc_except_table PROGBITS         000000000b3b03cc  0b1b03cc
       0000000000a73118  0000000000000000   A       0     0     4
  [24] .rodata           PROGBITS         000000000be23500  0bc23500
       000000000045590f  0000000000000000   A       0     0     64
  [25] .rodata.cst       PROGBITS         000000000c278e10  0c078e10
       0000000000070f50  0000000000000000   A       0     0     16
  [26] .rodata.str       PROGBITS         000000000c2e9d60  0c0e9d60
       00000000005a4940  0000000000000000   A       0     0     32
  [27] .tdata            PROGBITS         000000000c88f000  0c68f000
       0000000000000028  0000000000000000 WAT       0     0     8
  [28] .tbss             NOBITS           000000000c88f040  00000000
       000000000000b2f8  0000000000000000 WAT       0     0     64
  [29] .dynamic          DYNAMIC          000000000c88f028  0c68f028
       0000000000000430  0000000000000010  WA       7     0     8
  [30] .data.rel.ro      PROGBITS         000000000c88f460  0c68f460
       000000000051e4a8  0000000000000000  WA       0     0     32
  [31] .fini_array       FINI_ARRAY       000000000cdad908  0cbad908
       0000000000000010  0000000000000000  WA       0     0     8
  [32] .init_array       INIT_ARRAY       000000000cdad918  0cbad918
       0000000000000088  0000000000000000  WA       0     0     8
  [33] .got              PROGBITS         000000000cdad9a0  0cbad9a0
       0000000000005360  0000000000000000  WA       0     0     8
  [34] .copyrel.rel.ro   NOBITS           000000000cdb2d00  00000000
       0000000000005dc9  0000000000000000  WA       0     0     64
  [35] .got.plt          PROGBITS         000000000cdb9000  0cbb3000
       00000000000065c8  0000000000000000  WA       0     0     8
  [36] .ctors            PROGBITS         000000000cdbf5c8  0cbb95c8
       0000000000000008  0000000000000000  WA       0     0     8
  [37] .data             PROGBITS         000000000cdbf600  0cbb9600
       00000000000d1d60  0000000000000000  WA       0     0     64
  [38] .data1            PROGBITS         000000000ce91360  0cc8b360
       00000000000001e0  0000000000000000  WA       0     0     32
  [39] .tm_clone_table   PROGBITS         000000000ce91540  0cc8b540
       0000000000000000  0000000000000000  WA       0     0     8
  [40] .copyrel          NOBITS           000000000ce91540  00000000
       00000000000008a8  0000000000000000  WA       0     0     64
  [41] .bss              NOBITS           000000000ce91e00  00000000
       0000000000828f40  0000000000000000  WA       0     0     32
  [42] .common           NOBITS           000000000d6bad40  00000000
       0000000000005f28  0000000000000000  WA       0     0     32
  [43] .gnu.build.attrib NOTE             0000000000000000  0cc8b540
       00000000000012a0  0000000000000000           0     0     4
  [44] .gnu.build.attrib NOTE             0000000000000000  0cc8c7e0
       0000000000000e80  0000000000000000           0     0     4
  [45] .gnu.build.attrib NOTE             0000000000000000  0cc8d660
       0000000000000e80  0000000000000000           0     0     4
  [46] .gnu.build.attrib NOTE             0000000000000000  0cc8e4e0
       0000000000000e80  0000000000000000           0     0     4
  [47] .gnu.build.attrib NOTE             0000000000000000  0cc8f360
       0000000000000e80  0000000000000000           0     0     4
  [48] .strtab           STRTAB           0000000000000000  0cc911e0
       0000000001c76dc1  0000000000000000           0     0     1
  [49] .shstrtab         STRTAB           0000000000000000  0e907fa1
       00000000000002e9  0000000000000000           0     0     1
  [50] .symtab           SYMTAB           0000000000000000  0e908290
       000000000119c220  0000000000000018          48   765314     8
  [51] .comment          PROGBITS         0000000000000000  0faa44b0
       00000000000000b8  0000000000000000           0     0     1
  [52] .comment          PROGBITS         0000000000000000  0faa4568
       0000000000de43d0  0000000000000000           0     0     1
  [53] .debug_abbrev     PROGBITS         0000000000000000  10888938
       0000000000731fdd  0000000000000000           0     0     1
  [54] .debug_aranges    PROGBITS         0000000000000000  10fba920
       0000000000469c50  0000000000000000           0     0     16
  [55] .debug_info       PROGBITS         0000000000000000  11424570
       000000000ea9d323  0000000000000000           0     0     1
  [56] .debug_line       PROGBITS         0000000000000000  1fec1893
       00000000039f9b30  0000000000000000           0     0     1
  [57] .debug_line_str   PROGBITS         0000000000000000  238bb3c3
       00000000000a91b0  0000000000000000           0     0     1
  [58] .debug_loc        PROGBITS         0000000000000000  23964573
       00000000000c3a71  0000000000000000           0     0     1
  [59] .debug_loclists   PROGBITS         0000000000000000  23a27fe4
       000000000660eb51  0000000000000000           0     0     1
  [60] .debug_macro      PROGBITS         0000000000000000  2a036b35
       00000000004720ac  0000000000000000           0     0     1
  [61] .debug_ranges     PROGBITS         0000000000000000  2a4a8be1
       0000000000031750  0000000000000000           0     0     1
  [62] .debug_rnglists   PROGBITS         0000000000000000  2a4da331
       00000000015d85c5  0000000000000000           0     0     1
  [63] .debug_str        PROGBITS         0000000000000000  2bab28f6
       0000000002c270cd  0000000000000000           0     0     1
Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings), I (info),
  L (link order), O (extra OS processing required), G (group), T (TLS),
  C (compressed), x (unknown), o (OS specific), E (exclude),
  l (large), p (processor specific)

Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  PHDR           0x0000000000000040 0x0000000000200040 0x0000000000200040
                 0x00000000000002d8 0x00000000000002d8  R      0x8
  INTERP         0x0000000000000318 0x0000000000200318 0x0000000000200318
                 0x000000000000001c 0x000000000000001c  R      0x1
      [Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]
  NOTE           0x0000000000000334 0x0000000000200334 0x0000000000200334
                 0x0000000000000044 0x0000000000000044  R      0x4
  LOAD           0x0000000000000000 0x0000000000200000 0x0000000000200000
                 0x0000000000050260 0x0000000000050260  R      0x1000
  LOAD           0x0000000000051000 0x0000000000251000 0x0000000000251000
                 0x0000000009d16020 0x0000000009d16020  R E    0x1000
  LOAD           0x0000000009d68000 0x0000000009f68000 0x0000000009f68000
                 0x00000000029266a0 0x00000000029266a0  R      0x1000
  LOAD           0x000000000c68f000 0x000000000c88f000 0x000000000c88f000
                 0x0000000000523d00 0x0000000000529ac9  RW     0x1000
                   #             => 0x000000000cdb8ac9 = 0x000000000c88f000 + 0x0000000000529ac9
                   #                + 0x537 = 0x000000000cdb9000
                   # ??? the MemSiz is bigger than the FileSiz
                   # ??? this seems to result in a 'hole' after the mapping of the file content.
  LOAD           0x000000000cbb3000 0x000000000cdb9000 0x000000000cdb9000
                 0x00000000000d8540 0x0000000000907c68  RW     0x1000
                   #             => 0x000000000d6c0c68 = 0x000000000cdb9000 + 0x0000000000907c68
  TLS            0x000000000c68f000 0x000000000c88f000 0x000000000c88f000
                 0x0000000000000028 0x000000000000b338  R      0x40
  DYNAMIC        0x000000000c68f028 0x000000000c88f028 0x000000000c88f028
                 0x0000000000000430 0x0000000000000430  RW     0x8
  GNU_EH_FRAME   0x000000000af25c98 0x000000000b125c98 0x000000000b125c98
                 0x000000000028a734 0x000000000028a734  R      0x4
  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000  RWE    0x0
  GNU_RELRO      0x000000000c68f000 0x000000000c88f000 0x000000000c88f000
                 0x0000000000523d00 0x000000000052a000  R      0x40

The above shows that the resulting load in memory can/could give in the virtual addresses
a contiguous segment if pages are rounded to 0x1000.

Valgrind trace of the succesful (with hack) load:
  ------ name = /cm/build13/cm/ot/TACT/UIF!28.0.0.62/build_G!30.OP.L8/ada/exe/tact_uif_shared_exe

  Un-de-overlapped _DebugInfoMappings:
    [0]    avma 0x200000              size 331776      foff 0           -- -- ro
    [1]    avma 0x251000              size 164720640    foff 331776      rx -- --
    [2]    avma 0x9f68000             size 43151360    foff 165052416    -- -- ro
    [3]    avma 0xc88f000             size 5390336     foff 208203776    -- rw --
                       #    5390336 = 0x524000 = FileSiz 0x0000000000523d00 rounded up 0x1000.
    [4]    avma 0xcdb9000             size 888832      foff 213594112    -- rw --
                       #    888832 = 0xd9000 = FileSiz 0x00000000000d8540 rounded up 0x1000.

  De-overlapped DebugInfoMappings:
    [0]    avma 0x200000              size 331776      foff 0           -- -- ro
    [1]    avma 0x251000              size 164720640    foff 331776      rx -- --
    [2]    avma 0x9f68000             size 43151360    foff 165052416    -- -- ro
    [3]    avma 0xc88f000             size 5390336     foff 208203776    -- rw --
    [4]    avma 0xcdb9000             size 888832      foff 213594112    -- rw --


When the notify accept is done for the executable, the linux mappings are:
  (gdb) info proc mappings 46044
  process 46044
  Mapped address spaces:

            Start Addr           End Addr       Size     Offset  Perms  objfile
              0x200000           0x251000    0x51000        0x0  r--p   /cm/build13/cm/ot/TACT/UIF!28.0.0.62/build_G!30.OP.L8/ada/exe/tact_uif_shared_exe
              0x251000          0x9f68000  0x9d17000    0x51000  r-xp   /cm/build13/cm/ot/TACT/UIF!28.0.0.62/build_G!30.OP.L8/ada/exe/tact_uif_shared_exe
             0x9f68000          0xc88f000  0x2927000  0x9d68000  r--p   /cm/build13/cm/ot/TACT/UIF!28.0.0.62/build_G!30.OP.L8/ada/exe/tact_uif_shared_exe
             0xc88f000          0xcdb3000   0x524000  0xc68f000  rw-p   /cm/build13/cm/ot/TACT/UIF!28.0.0.62/build_G!30.OP.L8/ada/exe/tact_uif_shared_exe
             0xcdb3000          0xcdb9000     0x6000        0x0  rw-p   
             0xcdb9000          0xce92000    0xd9000  0xcbb3000  rw-p   /cm/build13/cm/ot/TACT/UIF!28.0.0.62/build_G!30.OP.L8/ada/exe/tact_uif_shared_exe
             0xce92000          0xd6c1000   0x82f000        0x0  rw-p   
             0xd6c1000          0xd6ee000    0x2d000        0x0  r-xp   /usr/lib64/ld-2.28.so
             0xd8ee000          0xd8f1000     0x3000    0x2d000  rw-p   /usr/lib64/ld-2.28.so
             0xd8f1000          0xd8f2000     0x1000        0x0  rwxp   
            0x58000000         0x58001000     0x1000        0x0  r--p   /cm/build12/cm/ot/TOOL/GNU!28.0.0.15/build_G!30.OP.L8/generated/libexec/valgrind/memcheck-amd64-linux
            0x58001000         0x58202000   0x201000     0x1000  r-xp   /cm/build12/cm/ot/TOOL/GNU!28.0.0.15/build_G!30.OP.L8/generated/libexec/valgrind/memcheck-amd64-linux
            0x58202000         0x582a0000    0x9e000   0x202000  r--p   /cm/build12/cm/ot/TOOL/GNU!28.0.0.15/build_G!30.OP.L8/generated/libexec/valgrind/memcheck-amd64-linux
            0x582a0000         0x582a5000     0x5000   0x2a0000  rw-p   /cm/build12/cm/ot/TOOL/GNU!28.0.0.15/build_G!30.OP.L8/generated/libexec/valgrind/memcheck-amd64-linux
            0x582a5000         0x59cb1000  0x1a0c000        0x0  rw-p   [heap]
          0x1002001000       0x100297c000   0x97b000        0x0  rwxp   
          0x1ffefe9000       0x1fff001000    0x18000        0x0  rwxp   
        0x7ffff7ff9000     0x7ffff7ffd000     0x4000        0x0  r--p   [vvar]
        0x7ffffffc9000     0x7ffffffff000    0x36000        0x0  rw-p   [stack]
    0xffffffffff600000 0xffffffffff601000     0x1000        0x0  r-xp   [vsyscall]
  (gdb)

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 ????

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.
Comment 1 Paul Floyd 2023-08-30 19:49:27 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.
Comment 2 Paul Floyd 2023-08-30 21:13:10 UTC
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.
Comment 3 Philippe Waroquiers 2023-08-31 06:26:27 UTC
(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
Comment 4 Paul Floyd 2023-08-31 07:53:22 UTC
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.
Comment 5 Philippe Waroquiers 2023-09-01 19:06:37 UTC
Fixed in c0b2c786d