From: Paul Floyd Date: Sat, 23 Sep 2023 14:11:55 +0000 (+0200) Subject: FreeBSD: fix reading debuginfo of the tool itself X-Git-Tag: VALGRIND_3_22_0~49 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6fdd59afb5e473b30e7ad1fbadcf9a397253fed4;p=thirdparty%2Fvalgrind.git FreeBSD: fix reading debuginfo of the tool itself Not sure if this was ever functional since I started working on it, but FreeBSD parse_procselfmaps doesn't handle the RW PT_LOAD correctly. Also since FreeBSD was ignoring non-fixed RO PT_LOAD mappings ML_(read_elf_object) didn't see the right number of debuginfo mappings compared to the ELF header. The code is fairly hacky and brute force. --- diff --git a/NEWS b/NEWS index 4b66f97679..cf37711fba 100644 --- a/NEWS +++ b/NEWS @@ -85,6 +85,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. 473944 Handle mold linker split RW PT_LOAD segments correctly 474332 aligned_alloc under Valgrind returns nullptr when alignment is not a multiple of sizeof(void *) n-i-bz Allow arguments with spaces in .valgrindrc files +n-i-bz FreeBSD fixed reading of Valgrind tools own debuginfo To see details of a given bug, visit https://bugs.kde.org/show_bug.cgi?id=XXXXXX diff --git a/coregrind/m_aspacemgr/aspacemgr-linux.c b/coregrind/m_aspacemgr/aspacemgr-linux.c index ba8964928e..53d0536de4 100644 --- a/coregrind/m_aspacemgr/aspacemgr-linux.c +++ b/coregrind/m_aspacemgr/aspacemgr-linux.c @@ -348,8 +348,8 @@ static Int find_nsegment_idx ( Addr a ); static void parse_procselfmaps ( void (*record_mapping)( Addr addr, SizeT len, UInt prot, - ULong dev, ULong ino, Off64T offset, - const HChar* filename ), + ULong dev, ULong ino, Off64T offset, + const HChar* filename, Bool ignore_offset ), void (*record_gap)( Addr addr, SizeT len ) ); @@ -783,8 +783,8 @@ static Bool preen_nsegments ( void ) static Bool sync_check_ok = False; static void sync_check_mapping_callback ( Addr addr, SizeT len, UInt prot, - ULong dev, ULong ino, Off64T offset, - const HChar* filename ) + ULong dev, ULong ino, Off64T offset, + const HChar* filename, Bool ignore_offset ) { Int iLo, iHi, i; Bool sloppyXcheck, sloppyRcheck; @@ -1502,7 +1502,7 @@ static void init_nsegment ( /*OUT*/NSegment* seg ) seg->hasR = seg->hasW = seg->hasX = seg->hasT = seg->isCH = False; #if defined(VGO_freebsd) - seg->isFF = False; + seg->ignore_offset = False; #endif } @@ -1529,7 +1529,7 @@ static void init_resvn ( /*OUT*/NSegment* seg, Addr start, Addr end ) static void read_maps_callback ( Addr addr, SizeT len, UInt prot, ULong dev, ULong ino, Off64T offset, - const HChar* filename ) + const HChar* filename, Bool ignore_offset ) { NSegment seg; init_nsegment( &seg ); @@ -1538,6 +1538,9 @@ static void read_maps_callback ( Addr addr, SizeT len, UInt prot, seg.dev = dev; seg.ino = ino; seg.offset = offset; +#if defined(VGO_freebsd) + seg.ignore_offset = ignore_offset; +#endif seg.hasR = toBool(prot & VKI_PROT_READ); seg.hasW = toBool(prot & VKI_PROT_WRITE); seg.hasX = toBool(prot & VKI_PROT_EXEC); @@ -3442,12 +3445,15 @@ static void read_procselfmaps_into_buf ( void ) mapped file device and inode offset in file, or zero if no file filename, zero terminated, or NULL if no file + ignore_offset, when the exact offset cannot be + obtained So the sig of the called fn might be void (*record_mapping)( Addr start, SizeT size, UInt prot, UInt dev, UInt info, - ULong foffset, UChar* filename ) + ULong foffset, UChar* filename, + Bool ignore_offset ) Note that the supplied filename is transiently stored; record_mapping should make a copy if it wants to keep it. @@ -3458,7 +3464,7 @@ static void read_procselfmaps_into_buf ( void ) static void parse_procselfmaps ( void (*record_mapping)( Addr addr, SizeT len, UInt prot, ULong dev, ULong ino, Off64T offset, - const HChar* filename ), + const HChar* filename, Bool ignore_offset ), void (*record_gap)( Addr addr, SizeT len ) ) { @@ -3609,7 +3615,7 @@ static void parse_procselfmaps ( if (record_mapping && start < endPlusOne) (*record_mapping) ( start, endPlusOne-start, prot, dev, ino, - foffset, filename ); + foffset, filename, False ); if ('\0' != tmp) { filename[i_eol - i] = tmp; @@ -3636,7 +3642,7 @@ static void parse_procselfmaps ( (*record_mapping)( commpage_start, commpage_end1 - commpage_start, VKI_PROT_READ|VKI_PROT_EXEC, 0/*dev*/, 0/*ino*/, 0/*foffset*/, - NULL); + NULL, False); gapStart = commpage_end1; } } @@ -3667,7 +3673,7 @@ static UInt stats_machcalls = 0; static void parse_procselfmaps ( void (*record_mapping)( Addr addr, SizeT len, UInt prot, ULong dev, ULong ino, Off64T offset, - const HChar* filename ), + const HChar* filename, Bool ignore_offset ), void (*record_gap)( Addr addr, SizeT len ) ) { @@ -3705,7 +3711,7 @@ static void parse_procselfmaps ( } if (record_mapping) { (*record_mapping)(addr, size, mach2vki(info.protection), - 0, 0, info.offset, NULL); + 0, 0, info.offset, NULL, False); } last = addr + size; } @@ -3898,6 +3904,60 @@ Bool VG_(get_changed_segments)( /*------BEGIN-procmaps-parser-for-Freebsd------------------------*/ #elif defined(VGO_freebsd) +/* + * PJF 2023-09-23 + * + * This function is somewhat badly named for FreeBSD, where the + * /proc filesystem is optional so we can't count on users + * having it. Instead we use the KERN_PROC_VMMAP syscall. + * So far so good. + * + * This function is used in two contexts. The heaviest use is from + * VG_(am_do_sync_check) as a sanity check that the contents of the + * global nsegments array is consistent with what the OS reports + * as being memory maps. No known problems with that. + * + * The other use is at startup in order to get the mapping for the + * tool itself. In this case we have a fairly big problem. There is + * a difference in the mapping used when the kernel loads an exe + * and when the link loader ldrt (or Valgrind which does the same + * job for the guest exe. In the case of ldrt, all ELF PT_LOAD + * sections get mmap'd. The kernel, however, does _not_ mmap + * the RW PT_LOAD. + * + * For instance, objdump -p for memcheck-amd64-freebsd contains + * LOAD off 0x0000000000000000 vaddr 0x0000000038000000 paddr 0x0000000038000000 align 2**12 + * filesz 0x00000000000c5124 memsz 0x00000000000c5124 flags r-- + * LOAD off 0x00000000000c5130 vaddr 0x00000000380c6130 paddr 0x00000000380c6130 align 2**12 + * filesz 0x00000000001b10df memsz 0x00000000001b10df flags r-x + * LOAD off 0x0000000000276210 vaddr 0x0000000038278210 paddr 0x0000000038278210 align 2**12 + * filesz 0x0000000000000a90 memsz 0x00000000025dd000 flags rw- + * + * Running procstat -v on a running instance gives + * 44814 0x38000000 0x380c6000 r-- 198 2558 2 0 CN--- vn /usr/home/paulf/scratch/valgrind/memcheck/memcheck-amd64-freebsd + * 44814 0x380c6000 0x38278000 r-x 434 2558 2 0 CN--- vn /usr/home/paulf/scratch/valgrind/memcheck/memcheck-amd64-freebsd + * 44814 0x38278000 0x3a856000 rw- 4590 4590 1 0 ----- df + * + * Instead of mmap'ing the RW PT_LOAD the kernel has mmap'd anonymous swap and copied from the exe file. + * See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=273956 + * + * So what can we do? We can reuse most of the info from the previous 'r-x' mapping. + * The filename, dev and ino are all the same. That leaves the offset. We can + * make a rough estimate of the value as being previous offset + previous size. + * Since the addresses in memory will be page aligned it's not possible to + * obtain the original offset. It isn't good enough for ML_(read_elf_object) + * in readelf.c + * + * As a hack of last resort we force ML_(read_elf_object) to accept this + * mapping by adding an "ignore offset" flag. We can't be wrong that + * there is something mapped roughly there - it's the global data of the + * code that is executing on the CPU! Furthermore, this is not frequently + * used. The main benefit is for Valgrind developers. Without this hack, + * if Valgrind crashes or asserts it will print its own stack without + * debuginfo, which is mostly useless. See the above FreeBSD bugzilla item + * for an example. + */ + /* Size of a smallish table used to read /proc/self/map entries. */ #define M_PROCMAP_BUF 10485760 /* 10M */ @@ -3907,67 +3967,95 @@ static char procmap_buf[M_PROCMAP_BUF]; static void parse_procselfmaps ( void (*record_mapping)( Addr addr, SizeT len, UInt prot, ULong dev, ULong ino, Off64T offset, - const HChar* filename ), + const HChar* filename, Bool ignore_offset ), void (*record_gap)( Addr addr, SizeT len ) ) { - Addr start, endPlusOne, gapStart; - char* filename; - char *p; - UInt prot; - ULong foffset, dev, ino; - struct vki_kinfo_vmentry *kve; - vki_size_t len; - Int oid[4]; - SysRes sres; - - foffset = ino = 0; /* keep gcc-4.1.0 happy */ - - oid[0] = VKI_CTL_KERN; - oid[1] = VKI_KERN_PROC; - oid[2] = VKI_KERN_PROC_VMMAP; - oid[3] = sr_Res(VG_(do_syscall0)(__NR_getpid)); - len = sizeof(procmap_buf); - - sres = VG_(do_syscall6)(__NR___sysctl, (UWord)oid, 4, (UWord)procmap_buf, - (UWord)&len, 0, 0); - if (sr_isError(sres)) { - VG_(debugLog)(0, "procselfmaps", "sysctl %lu\n", sr_Err(sres)); - ML_(am_exit)(1); - } - gapStart = Addr_MIN; - p = procmap_buf; - while (p < (char *)procmap_buf + len) { - kve = (struct vki_kinfo_vmentry *)p; - start = (UWord)kve->kve_start; - endPlusOne = (UWord)kve->kve_end; - foffset = kve->kve_offset; - filename = kve->kve_path; - dev = kve->kve_vn_fsid_freebsd11; - ino = kve->kve_fileid; - if (filename[0] != '/') { + Addr start, endPlusOne, gapStart; + char* filename; + char *p; + UInt prot; + ULong foffset, dev, ino; + struct vki_kinfo_vmentry *kve; + vki_size_t len; + Int oid[4]; + SysRes sres; + Int map_count = 0; + // could copy the whole kinfo_vmentry but it is 1160 bytes + char *rx_filename = NULL; + ULong rx_dev = 0U; + ULong rx_ino = 0U; + ULong rx_foffset = 0U; + Bool tool_read_maps = (record_mapping == read_maps_callback); + + foffset = ino = 0; /* keep gcc-4.1.0 happy */ + + oid[0] = VKI_CTL_KERN; + oid[1] = VKI_KERN_PROC; + oid[2] = VKI_KERN_PROC_VMMAP; + oid[3] = sr_Res(VG_(do_syscall0)(__NR_getpid)); + len = sizeof(procmap_buf); + + sres = VG_(do_syscall6)(__NR___sysctl, (UWord)oid, 4, (UWord)procmap_buf, + (UWord)&len, 0, 0); + if (sr_isError(sres)) { + VG_(debugLog)(0, "procselfmaps", "sysctl %lu\n", sr_Err(sres)); + ML_(am_exit)(1); + } + gapStart = Addr_MIN; + p = procmap_buf; + while (p < (char *)procmap_buf + len) { + kve = (struct vki_kinfo_vmentry *)p; + start = (UWord)kve->kve_start; + endPlusOne = (UWord)kve->kve_end; + foffset = kve->kve_offset; + filename = kve->kve_path; + dev = kve->kve_vn_fsid_freebsd11; + ino = kve->kve_fileid; + if (filename[0] != '/') { filename = NULL; foffset = 0; - } - - prot = 0; - if (kve->kve_protection & VKI_KVME_PROT_READ) prot |= VKI_PROT_READ; - if (kve->kve_protection & VKI_KVME_PROT_WRITE) prot |= VKI_PROT_WRITE; - if (kve->kve_protection & VKI_KVME_PROT_EXEC) prot |= VKI_PROT_EXEC; + } - if (record_gap && gapStart < start) - (*record_gap) ( gapStart, start-gapStart ); + prot = 0; + if (kve->kve_protection & VKI_KVME_PROT_READ) prot |= VKI_PROT_READ; + if (kve->kve_protection & VKI_KVME_PROT_WRITE) prot |= VKI_PROT_WRITE; + if (kve->kve_protection & VKI_KVME_PROT_EXEC) prot |= VKI_PROT_EXEC; + + map_count = (p - (char *)procmap_buf)/kve->kve_structsize; + + if (tool_read_maps && map_count == 2) { + aspacem_assert((prot & (VKI_PROT_READ | VKI_PROT_WRITE)) == (VKI_PROT_READ | VKI_PROT_WRITE)); + filename = rx_filename; + dev = rx_dev; + ino = rx_ino; + foffset = rx_foffset; + } - if (record_mapping && start < endPlusOne) - (*record_mapping) ( start, endPlusOne-start, - prot, dev, ino, - foffset, filename ); - gapStart = endPlusOne; - p += kve->kve_structsize; - } + if (record_gap && gapStart < start) + (*record_gap) ( gapStart, start-gapStart ); + + if (record_mapping && start < endPlusOne) { + (*record_mapping) ( start, endPlusOne-start, + prot, dev, ino, + foffset, filename, tool_read_maps && map_count == 2 ); + } + + if (tool_read_maps && map_count == 1) { + aspacem_assert((prot & (VKI_PROT_READ | VKI_PROT_EXEC)) == (VKI_PROT_READ | VKI_PROT_EXEC)); + rx_filename = filename; + rx_dev = dev; + rx_ino = ino; + /* this is only accurate to the page alignment */ + rx_foffset = foffset + endPlusOne - start; + } + + gapStart = endPlusOne; + p += kve->kve_structsize; + } - if (record_gap && gapStart < Addr_MAX) - (*record_gap) ( gapStart, Addr_MAX - gapStart + 1 ); + if (record_gap && gapStart < Addr_MAX) + (*record_gap) ( gapStart, Addr_MAX - gapStart + 1 ); } /*------END-procmaps-parser-for-Freebsd--------------------------*/ @@ -4135,7 +4223,7 @@ static Mapping *next_rmap(const HChar *buffer, SizeT entries, SizeT *idx, static void parse_procselfmaps ( void (*record_mapping)( Addr addr, SizeT len, UInt prot, ULong dev, ULong ino, Off64T offset, - const HChar *filename ), + const HChar *filename, Bool ignore_offset ), void (*record_gap)( Addr addr, SizeT len ) ) { @@ -4201,7 +4289,7 @@ static void parse_procselfmaps ( (*record_mapping)(xmap->addr, xmap->size, xmap->prot, xmap->dev, xmap->ino, xmap->foffset, (xmap->filename[0] != '\0') ? - xmap->filename : NULL); + xmap->filename : NULL, False); start = xmap->addr + xmap->size; advance_xmap = True; @@ -4215,7 +4303,7 @@ static void parse_procselfmaps ( size = xmap->addr - start; if (record_mapping != NULL) - (*record_mapping)(start, size, rmap->prot, 0, 0, 0, NULL); + (*record_mapping)(start, size, rmap->prot, 0, 0, 0, NULL, False); start += size; } else { @@ -4242,7 +4330,7 @@ static UInt found_prot; /* Reports a new mapping into variables above. */ static void new_segment_found_callback(Addr addr, SizeT len, UInt prot, - ULong dev, ULong ino, Off64T offset, const HChar *filename) + ULong dev, ULong ino, Off64T offset, const HChar *filename, Bool ignore_offset) { aspacem_assert(addr <= addr + len - 1); diff --git a/coregrind/m_debuginfo/debuginfo.c b/coregrind/m_debuginfo/debuginfo.c index ba66fb9c69..0ffab17922 100644 --- a/coregrind/m_debuginfo/debuginfo.c +++ b/coregrind/m_debuginfo/debuginfo.c @@ -549,6 +549,31 @@ static Bool ranges_overlap (Addr s1, SizeT len1, Addr s2, SizeT len2 ) return True; } +/* + * PJF 2023-09-23 + * + * FreeBSD can perform a temporary mapping when loading exes + * and shared libraries. This is seen as a single page mapped + * before the ro/rx/rw mappings from the ELF file itself. More + * importantly, FreeBSD can reuse that same page when loading + * subsequent shared libraries. That means that we see this + * page as an overlap. Previously we noted that the mapping + * was not fixed and ignored it by returning early from + * VG_(di_notify_mmap). + * + * That works OK in general, but not for the tool itself. + * In order to read symbols for the tool, ML_(read_elf_object) + * needs to match up the ELF headers with the DebugInfo maps + * (populated from the global nsegments array). + * + * Two possible solutions would be to hack parse_procselfmaps + * even more so that it doesn't record the ro segment (is + * that info in kve_flags?). The other, which was also my + * original fix for this problem, is to just ignore identical + * ro mappings for different files on FreeBSD. I'm not certain + * that the size is always one page - that could be used to + * tighten the check even more. + */ /* Do the basic mappings of the two DebugInfos overlap in any way? */ static Bool do_DebugInfos_overlap ( const DebugInfo* di1, const DebugInfo* di2 ) @@ -560,8 +585,19 @@ static Bool do_DebugInfos_overlap ( const DebugInfo* di1, const DebugInfo* di2 ) const DebugInfoMapping* map1 = VG_(indexXA)(di1->fsm.maps, i); for (j = 0; j < VG_(sizeXA)(di2->fsm.maps); j++) { const DebugInfoMapping* map2 = VG_(indexXA)(di2->fsm.maps, j); - if (ranges_overlap(map1->avma, map1->size, map2->avma, map2->size)) + if (ranges_overlap(map1->avma, map1->size, map2->avma, map2->size)) { +#if defined(VGO_freebsd) + if (di1 != di2 && map1->ro && map2->ro && + map1->avma == map2->avma && map1->size == map2->size) { + if (VG_(debugLog_getLevel)() >= 3) { + VG_(dmsg)("do_DebugInfos_overlap-0: identical ro mappings from files %s and %s\n", + di1->fsm.filename, di2->fsm.filename); + } + continue; + } +#endif return True; + } } } @@ -1297,8 +1333,9 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) #if defined(VGO_freebsd) /* Ignore non-fixed read-only mappings. The dynamic linker may be * mapping something for its own transient purposes. */ - if (!seg->isFF && is_ro_map) - return 0; + if (!seg->isFF && is_ro_map && debug) { + VG_(dmsg)("di_notify_mmap-4: non-fixed ro map\n"); + } #endif #if defined(VGO_darwin) @@ -1411,6 +1448,9 @@ ULong VG_(di_notify_mmap)( Addr a, Bool allow_SkFileV, Int use_fd ) map.avma = seg->start; map.size = seg->end + 1 - seg->start; map.foff = seg->offset; +#if defined(VGO_freebsd) + map.ignore_foff = seg->ignore_offset; +#endif map.rx = is_rx_map; map.rw = is_rw_map; map.ro = is_ro_map; diff --git a/coregrind/m_debuginfo/priv_storage.h b/coregrind/m_debuginfo/priv_storage.h index 912edd435a..441b379d2e 100644 --- a/coregrind/m_debuginfo/priv_storage.h +++ b/coregrind/m_debuginfo/priv_storage.h @@ -577,6 +577,9 @@ typedef struct SizeT size; /* and map address of each mapping */ OffT foff; Bool rx, rw, ro; /* memory access flags for this mapping */ +#if defined(VGO_freebsd) + Bool ignore_foff; +#endif } DebugInfoMapping; struct _DebugInfoFSM diff --git a/coregrind/m_debuginfo/readelf.c b/coregrind/m_debuginfo/readelf.c index 56a9ce6b23..5456fafb02 100644 --- a/coregrind/m_debuginfo/readelf.c +++ b/coregrind/m_debuginfo/readelf.c @@ -2142,12 +2142,22 @@ Bool ML_(read_elf_object) ( struct _DebugInfo* di ) Bool loaded = False; for (j = 0; j < VG_(sizeXA)(di->fsm.maps); j++) { const DebugInfoMapping* map = VG_(indexXA)(di->fsm.maps, j); + Bool offset_checks = a_phdr.p_offset >= map->foff + && a_phdr.p_offset < map->foff + map->size + && a_phdr.p_offset + a_phdr.p_filesz <= map->foff + map->size; +#if defined(VGO_freebsd) + /* + * One special case where we can't get an accurate value + * for the offset - the RW segment of the tool itself. + * See aspacemgr-linux.c parse_procselfmaps() + */ + if (map->ignore_foff) { + offset_checks = True; + } +#endif if ( (map->rx || map->rw || map->ro) && map->size > 0 /* stay sane */ - && a_phdr.p_offset >= map->foff - && a_phdr.p_offset < map->foff + map->size - && a_phdr.p_offset + a_phdr.p_filesz - <= map->foff + map->size) { + && offset_checks) { RangeAndBias item; item.svma_base = a_phdr.p_vaddr; item.svma_limit = a_phdr.p_vaddr + a_phdr.p_memsz; diff --git a/include/pub_tool_aspacemgr.h b/include/pub_tool_aspacemgr.h index 163c385d14..479a71d9f4 100644 --- a/include/pub_tool_aspacemgr.h +++ b/include/pub_tool_aspacemgr.h @@ -112,6 +112,7 @@ typedef Bool isCH; // True --> is client heap (SkAnonC ONLY) #if defined(VGO_freebsd) Bool isFF; // True --> is a fixed file mapping + Bool ignore_offset; // True --> we can't work out segment offset #endif } NSegment;