From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26737 invoked by alias); 24 May 2008 19:18:17 -0000 Received: (qmail 26710 invoked by uid 367); 24 May 2008 19:18:17 -0000 Date: Sat, 24 May 2008 19:18:00 -0000 Message-ID: <20080524191817.26695.qmail@sourceware.org> From: cagney@sourceware.org To: frysk-cvs@sourceware.org Subject: [SCM] master: Push image mapping management into UnwindH.hxx. X-Git-Refname: refs/heads/master X-Git-Reftype: branch X-Git-Oldrev: d1d421d9f90c359b825250b02887eebe586be8f5 X-Git-Newrev: 1936bb8c8e85e822981a1f8e6a6a3b189447ecbb Mailing-List: contact frysk-cvs-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: frysk-cvs-owner@sourceware.org Reply-To: frysk@sourceware.org X-SW-Source: 2008-q2/txt/msg00282.txt.bz2 The branch, master has been updated via 1936bb8c8e85e822981a1f8e6a6a3b189447ecbb (commit) from d1d421d9f90c359b825250b02887eebe586be8f5 (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email. - Log ----------------------------------------------------------------- commit 1936bb8c8e85e822981a1f8e6a6a3b189447ecbb Author: Andrew Cagney Date: Sat May 24 15:03:43 2008 -0400 Push image mapping management into UnwindH.hxx. Fyi, there's a pre-existing memory-free race bug here; this patch should help to better understand it. Prior to this patch the sequence was: - frysk calls libunwind to do a step (a.k.a. unwind a frame) - libunwind's step code calls frysk to populate a proc-info describing how to unwind the current frame - frysk finds and then mmaps in the required image - frysk creates an ElfImage object from the mmap; during garbage collection ElfImage will un-map the mmapped region. - frysk passes the mmaped region to libunwind's dwarf code which 1) fills in the proc-info and 2) retains a reference to the mmaped image - frysk returns to libunwind's step code; this drops all references to the ElfImage - libunwind's dwarf step code uses the supplied unwind information including the mmaped region completing the step - libunwind's dwarf step code "releases" the proc-info locally (frysk doesn't know that the mmapped region is unused), and then returns control to frysk Notice how the ElfImage object is dropped before libunwind has finished with it - a Syste.gc() will destroy the ElfImage and hence the mmapped region before libunwind has finished with it; and that frysk doesn't know exactly when it has been finished with. This patch, consolidates some methods into UnwindH and in the process makes the race more obvious by blatently creating and then dropping the ElfImage object. frysk-sys/lib/unwind/ChangeLog 2008-05-24 Andrew Cagney * Unwind.java (createElfImageFromVDSO): Delete. (createProcInfoFromElfImage): Delete. (fillProcInfoFromVDSO): New. (fillProcInfoFromElfImage): New. * cni/UnwindH.hxx: Implement. * ProcInfo.java: Use. * ElfImage.java: Simplify. * cni/ElfImage.cxx (ElfImage::mapElfImage): Delete. ----------------------------------------------------------------------- Summary of changes: frysk-sys/lib/unwind/ChangeLog | 9 ++ frysk-sys/lib/unwind/ElfImage.java | 67 +++------- frysk-sys/lib/unwind/ProcInfo.java | 18 +-- frysk-sys/lib/unwind/Unwind.java | 26 ++-- frysk-sys/lib/unwind/cni/ElfImage.cxx | 45 +------ frysk-sys/lib/unwind/cni/UnwindH.hxx | 235 ++++++++++++++++++++------------- 6 files changed, 196 insertions(+), 204 deletions(-) First 500 lines of diff: diff --git a/frysk-sys/lib/unwind/ChangeLog b/frysk-sys/lib/unwind/ChangeLog index e3409c3..4c93d20 100644 --- a/frysk-sys/lib/unwind/ChangeLog +++ b/frysk-sys/lib/unwind/ChangeLog @@ -1,5 +1,14 @@ 2008-05-24 Andrew Cagney + * Unwind.java (createElfImageFromVDSO): Delete. + (createProcInfoFromElfImage): Delete. + (fillProcInfoFromVDSO): New. + (fillProcInfoFromElfImage): New. + * cni/UnwindH.hxx: Implement. + * ProcInfo.java: Use. + * ElfImage.java: Simplify. + * cni/ElfImage.cxx (ElfImage::mapElfImage): Delete. + * ProcInfo.java (fillNotAvailable): New. * Unwind.java (fillProcInfoNotAvailable): New. diff --git a/frysk-sys/lib/unwind/ElfImage.java b/frysk-sys/lib/unwind/ElfImage.java index e938c96..872c45d 100644 --- a/frysk-sys/lib/unwind/ElfImage.java +++ b/frysk-sys/lib/unwind/ElfImage.java @@ -1,6 +1,6 @@ // This file is part of the program FRYSK. // -// Copyright 2007, Red Hat Inc. +// Copyright 2007, 2008, Red Hat Inc. // // FRYSK is free software; you can redistribute it and/or modify it // under the terms of the GNU General Public License as published by @@ -37,53 +37,30 @@ // version and license this file solely under the GPL without // exception. - package lib.unwind; -public class ElfImage -{ - - final String name; - - long elfImage; - - long size; - - long segbase; - - long mapoff; - - int ret = 0; - - public ElfImage (String name, long elfImage, long size, long segbase, long mapoff) - { - this.name = name; - this.elfImage = elfImage; - this.size = size; - this.segbase = segbase; - this.mapoff = mapoff; - } - - public ElfImage (int ret) - { - this.name = "ERROR: " + Integer.toString(ret); - this.ret = ret; - } - - public String toString () - { - if (ret != 0) - return "Bad Elf Image, ret: " + ret; - - return "Elf Image (" + name + "): 0x" + Long.toHexString(elfImage) + " size: " + size - + " segbase: 0x" + Long.toHexString(segbase) + " mapoff: 0x" - + Long.toHexString(mapoff); - } +/** + * An object that, eventually, releases its underlying mmaped [elf] + * data during a garbage collect. + * + * Since the garbage collect will, typically, occure after libunwind's + * step has finished with the data this operation is safe. However, + * there is the real possibility of the GC occuring early, unmapping + * the data causing an NPE. + */ - public static native ElfImage mapElfImage (String elfImageName, long segbase, - long hi, long mapoff); +public class ElfImage { - // @Override - native protected void finalize () throws Throwable; + private final long image; + private final long size; + public ElfImage(long image, long size) { + this.image = image; + this.size = size; + } + // @Override + protected void finalize() { + unmap(image, size); + } + private static native void unmap(long image, long size); } diff --git a/frysk-sys/lib/unwind/ProcInfo.java b/frysk-sys/lib/unwind/ProcInfo.java index 99f85e0..a76f0db 100644 --- a/frysk-sys/lib/unwind/ProcInfo.java +++ b/frysk-sys/lib/unwind/ProcInfo.java @@ -63,13 +63,9 @@ public class ProcInfo { long addressLow, long addressHigh, long offset, long ip, boolean needUnwindInfo) { - ElfImage elfImage = unwinder.createElfImageFromVDSO(addressSpace, - addressLow, - addressHigh, - offset); - return unwinder.createProcInfoFromElfImage(addressSpace, ip, - needUnwindInfo, elfImage, - this); + return unwinder.fillProcInfoFromVDSO(unwProcInfo, ip, needUnwindInfo, + addressSpace, + addressLow, addressHigh, offset); } public int fillFromElfImage(AddressSpace addressSpace, @@ -77,11 +73,11 @@ public class ProcInfo { long addressLow, long addressHigh, long offset, long ip, boolean needUnwindInfo) { - ElfImage elfImage = ElfImage.mapElfImage(name, addressLow, addressHigh, + return unwinder.fillProcInfoFromElfImage(unwProcInfo, ip, + needUnwindInfo, + addressSpace, + name, addressLow, addressHigh, offset); - return unwinder.createProcInfoFromElfImage(addressSpace, ip, - needUnwindInfo, elfImage, - this); } public long getStartIP() { diff --git a/frysk-sys/lib/unwind/Unwind.java b/frysk-sys/lib/unwind/Unwind.java index db1d852..540888c 100644 --- a/frysk-sys/lib/unwind/Unwind.java +++ b/frysk-sys/lib/unwind/Unwind.java @@ -73,20 +73,18 @@ public abstract class Unwind { abstract int getContext(long context); abstract int fillProcInfoNotAvailable(long unwProcInfo); - - // FIXME: shouldn't be public. - public abstract int createProcInfoFromElfImage(AddressSpace addressSpace, - long ip, - boolean needUnwindInfo, - ElfImage elfImage, - ProcInfo procInfo); - - // FIXME: shouldn't be public. - public abstract ElfImage createElfImageFromVDSO(AddressSpace addressSpace, - long segbase, long hi, - long mapoff); - - + abstract int fillProcInfoFromElfImage(long unwProcInfo, + long ip, boolean needUnwindInfo, + AddressSpace addressSpace, + String name, + long addressLow, long addressHigh, + long offset); + abstract int fillProcInfoFromVDSO(long unwProcInfo, + long ip, boolean needUnwindInfo, + AddressSpace addressSpace, + long addressLow, long addressHigh, + long offset); + abstract long getProcInfo(long unwCursor); abstract void destroyProcInfo(long unwProcInfo); diff --git a/frysk-sys/lib/unwind/cni/ElfImage.cxx b/frysk-sys/lib/unwind/cni/ElfImage.cxx index 3f7fdd3..68dc3c3 100644 --- a/frysk-sys/lib/unwind/cni/ElfImage.cxx +++ b/frysk-sys/lib/unwind/cni/ElfImage.cxx @@ -1,6 +1,6 @@ // This file is part of the program FRYSK. // -// Copyright 2007, Red Hat Inc. +// Copyright 2007, 2008, Red Hat Inc. // // FRYSK is free software; you can redistribute it and/or modify it // under the terms of the GNU General Public License as published by @@ -48,46 +48,7 @@ #include "lib/unwind/ElfImage.h" -lib::unwind::ElfImage* -lib::unwind::ElfImage::mapElfImage(jstring elfImageName, jlong segbase, jlong hi, - jlong mapoff) -{ - struct stat stat; - void *image; /* pointer to mmap'd image */ - size_t size; /* (file-) size of the image */ - int nameSize = JvGetStringUTFLength(elfImageName); - char name[nameSize+1]; - //JvGetStringUTFRegion(jstring STR, jsize START, jsize LEN, char* BUF); - JvGetStringUTFRegion(elfImageName, 0, nameSize, name); - name[nameSize] = '\0'; - - int fd = open (name, O_RDONLY); - if (fd < 0) - return new lib::unwind::ElfImage((jint) fd); - - int ret = fstat (fd, &stat); - if (ret < 0) - { - close (fd); - return new lib::unwind::ElfImage((jint) ret); - } - - size = stat.st_size; - image = mmap (NULL, size, PROT_READ, MAP_PRIVATE, fd, 0); - - close (fd); - if (image == MAP_FAILED) - return new lib::unwind::ElfImage((jint) -1); - - lib::unwind::ElfImage* elfImage - = new lib::unwind::ElfImage(elfImageName, (jlong) image, (jlong) size, - (jlong) segbase, (jlong) mapoff); - - return elfImage; -} - void -lib::unwind::ElfImage::finalize() -{ - munmap((void *) elfImage, (size_t) size); +lib::unwind::ElfImage::unmap(jlong image, jlong size) { + munmap((void *) image, (size_t) size); } diff --git a/frysk-sys/lib/unwind/cni/UnwindH.hxx b/frysk-sys/lib/unwind/cni/UnwindH.hxx index eaaf681..dda792f 100644 --- a/frysk-sys/lib/unwind/cni/UnwindH.hxx +++ b/frysk-sys/lib/unwind/cni/UnwindH.hxx @@ -44,6 +44,8 @@ #include #include #include +#include +#include #include #include LIBUNWIND_TARGET_H @@ -583,11 +585,15 @@ get_eh_frame_hdr_addr(unw_proc_info_t *pi, char *image, size_t size, return hdr; } -// The following is a local address space memory accessor used by -// unw_get_unwind_table to access the eh_frame_hdr. The arg pointer -// is the base address, addr is the offset from the base address. +/** + * The following are local memory image address space memory accessors + * used by unw_get_unwind_table to access the eh_frame_hdr. The arg + * pointer is the base address, addr is the offset from the base + * address. + */ + static int -local_access_mem(unw_addr_space_t as, unw_word_t addr, +image_access_mem(unw_addr_space_t as, unw_word_t addr, unw_word_t *val, int write, void *baseAddress) { // Writing is not supported if (write) @@ -598,67 +604,75 @@ local_access_mem(unw_addr_space_t as, unw_word_t addr, return UNW_ESUCCESS; } -static unw_accessors_t local_accessors -= {NULL, NULL, NULL, local_access_mem, NULL, NULL, NULL, NULL}; - -jint -TARGET::createProcInfoFromElfImage(AddressSpace* addressSpace, - jlong ip, - jboolean needUnwindInfo, - ElfImage* elfImage, - ProcInfo* procInfo) { - if (elfImage == NULL || elfImage->ret != 0) - return -UNW_ENOINFO; - - unw_proc_info_t* unwProcInfo = (unw_proc_info_t*) procInfo->unwProcInfo; - logf(fine, this, "Pre unw_get_unwind_table"); +static unw_accessors_t image_accessors = { + NULL, NULL, NULL, image_access_mem, NULL, NULL, NULL, NULL +}; + +static jint +fillProcInfoFromImage(frysk::rsl::Log* fine, + const char* name, + jlong unwProcInfo, + jlong ip, + jboolean needUnwindInfo, + void *image, + long size, + long segbase) { + unw_proc_info_t *procInfo = (::unw_proc_info_t *) unwProcInfo; + logf(fine, "fillProcInfoFromImage" + " %s unwProcInfo %lx, ip %lx, image %p, size %ld, segBase %lx", + name, (long) unwProcInfo, (long)ip, image, size, segbase); unw_word_t peh_vaddr = 0; - char *eh_table_hdr = get_eh_frame_hdr_addr(unwProcInfo, - (char *) elfImage->elfImage, - elfImage->size, - elfImage->segbase, + char *eh_table_hdr = get_eh_frame_hdr_addr(procInfo, + (char *) image, + size, segbase, &peh_vaddr); - - //jsize length = JvGetStringUTFLength (elfImage->name); - //char buffer[length + 1]; - //JvGetStringUTFRegion (elfImage->name, 0, elfImage->name->length(), buffer); - //buffer[length] = '\0'; - //fprintf(stderr, "%s: %p\n", buffer, eh_table_hdr); - - if (eh_table_hdr == NULL) + if (eh_table_hdr == NULL) { + logf(fine, "get_eh_frame_hdr failed"); + munmap(image, size); return -UNW_ENOINFO; + } int ret; - if (unwProcInfo->format == UNW_INFO_FORMAT_REMOTE_TABLE) + if (procInfo->format == UNW_INFO_FORMAT_REMOTE_TABLE) ret = unw_get_unwind_table((unw_word_t) ip, - unwProcInfo, + procInfo, (int) needUnwindInfo, - &local_accessors, + &image_accessors, // virtual address peh_vaddr, // address adjustment eh_table_hdr - peh_vaddr); else ret = unw_get_unwind_table((unw_word_t) ip, - unwProcInfo, + procInfo, (int) needUnwindInfo, - &local_accessors, + &image_accessors, // virtual address 0, // address adjustment eh_table_hdr); - - logf(fine, this, "Post unw_get_unwind_table %d", ret); + + // FIXME: The munmap can't be done until after the step code has + // finished with the proc-info. For moment ack around this by + // creating an ElfImage object that does the unmap in the finaliser. + // Of course this A) leaves many mmaped objects around; and B) has a + // race where the object could be unmapped before step has finished + // with it. + new ElfImage((jlong) image, (jlong) size); + // munmap(image, size); + logf(fine, "Post unw_get_unwind_table %d", ret); return ret; } -ElfImage* -TARGET::createElfImageFromVDSO(AddressSpace* addressSpace, - jlong lowAddress, jlong highAddress, - jlong offset) { - logf(fine, this, - "entering segbase: 0x%lx, highAddress: 0x%lx, mapoff: 0x%lx", +jint +TARGET::fillProcInfoFromVDSO(jlong unwProcInfo, jlong ip, + jboolean needUnwindInfo, + AddressSpace* addressSpace, + jlong lowAddress, jlong highAddress, + jlong offset) { + logf(fine, this, "fillProcInfoFromVDSO" + " segbase: 0x%lx, highAddress: 0x%lx, mapoff: 0x%lx", (unsigned long) lowAddress, (unsigned long) highAddress, (unsigned long) offset); void *image; @@ -667,77 +681,114 @@ TARGET::createElfImageFromVDSO(AddressSpace* addressSpace, unw_accessors_t *a; unsigned long segbase = (unsigned long) lowAddress; unsigned long hi = (unsigned long) highAddress; - unsigned long mapoff = (unsigned long) offset; size = hi - segbase; if (size > MAX_VDSO_SIZE) - return new ElfImage((jint) -1); - + return -UNW_ENOINFO; logf(fine, this, "checked size, 0x%lx", (unsigned long) size); + + logf(fine, this, "checking access_mem"); unw_addr_space_t as = (unw_addr_space_t) addressSpace->unwAddressSpace; a = unw_get_accessors (as); if (! a->access_mem) - return new ElfImage((jint) -1); + return -UNW_ENOINFO; - logf(fine, this, "checked access_mem"); - /* Try to decide whether it's an ELF image before bringing it all - in. */ + // Try to decide whether it's an ELF image before bringing it all + // in. + logf(fine, this, "checking magic"); if (size <= EI_CLASS || size <= sizeof (magic)) - return new ElfImage((jint) -1); - - if (sizeof (magic) >= SELFMAG) - { - int ret = (*a->access_mem) (as, (unw_word_t) segbase, &magic, - 0, (void *) addressSpace); - if (ret < 0) - return new ElfImage((jint) ret); - - if (memcmp (&magic, ELFMAG, SELFMAG) != 0) - return new ElfImage((jint) -1); + return -UNW_ENOINFO; + if (sizeof (magic) >= SELFMAG) { + int ret = (*a->access_mem) (as, (unw_word_t) segbase, &magic, + 0, (void *) addressSpace); + if (ret < 0) { + logf(fine, this, "error accessing VDSO %d", ret); + return ret; } + if (memcmp (&magic, ELFMAG, SELFMAG) != 0) { + logf(fine, this, "VDSO has bad magic"); + return -UNW_ENOINFO; + } + } - logf(fine, this, "checked magic size"); - + logf(fine, this, "mapping memory for image (using mmap, so can umaped)"); image = mmap (0, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (image == MAP_FAILED) - return new ElfImage((jint) -1); - - logf(fine, this, "mapped elfImage"); - if (sizeof (magic) >= SELFMAG) - { - *(unw_word_t *)image = magic; - hi = sizeof (magic); - } - else - hi = 0; + return -UNW_ENOINFO; logf(fine, this, "checked magic"); - for (; hi < size; hi += sizeof (unw_word_t)) - { - logf(finest, this, "Reading memory segbase: 0x%lx, image: %p, hi: 0x%lx, at: 0x%lx to location: %p", - segbase , image , hi, segbase+hi, (char *) image + hi); - int ret = (*a->access_mem) (as, segbase + hi,(unw_word_t *) ((char *) image + hi), - 0, (void *) addressSpace); + if (sizeof (magic) >= SELFMAG) { + *(unw_word_t *)image = magic; + hi = sizeof (magic); + } else { + hi = 0; + } - if (ret < 0) - { - munmap (image, size); - return new ElfImage((jint) ret); - } + logf(fine, this, "reading in VDSO"); + for (; hi < size; hi += sizeof (unw_word_t)) { + logf(finest, this, "Reading memory segbase: 0x%lx, image: %p, hi: 0x%lx, at: 0x%lx to location: %p", + segbase , image , hi, segbase+hi, (char *) image + hi); + int ret = (*a->access_mem) (as, segbase + hi,(unw_word_t *) ((char *) image + hi), + 0, (void *) addressSpace); + if (ret < 0) { + logf(fine, this, "error reading vdso"); + munmap (image, size); + return ret; } + } - logf(fine, this, "read memory into elf image"); + return fillProcInfoFromImage(fine, "[vdso]", unwProcInfo, ip, needUnwindInfo, + image, size, segbase); +} - if (segbase == mapoff) - mapoff = 0; +jint +TARGET::fillProcInfoFromElfImage(jlong unwProcInfo, jlong ip, hooks/post-receive -- frysk system monitor/debugger