From ea4e0a2f5d1fb19b1783d8d1243646031c87cdbe Mon Sep 17 00:00:00 2001 From: Joel Dice Date: Fri, 4 May 2012 18:35:13 -0600 Subject: [PATCH] fix a couple of subtle Thread.getStackTrace bugs The first problem was that, on x86, we failed to properly keep track of whether to expect the return address to be on the stack or not when unwinding through a frame. We were relying on a "stackLimit" pointer to tell us whether we were looking at the most recently-called frame by comparing it with the stack pointer for that frame. That was inaccurate in the case of a thread executing at the beginning of a method before a new frame is allocated, in which case the most recent two frames share a stack pointer, confusing the unwinder. The solution involves keeping track of how many frames we've looked at while walking the stack. The other problem was that compareIpToMethodBounds assumed every method was followed by at least one byte of padding before the next method started. That assumption was usually valid because we were storing the size following method code prior to the code itself. However, the last method of an AOT-compiled code image is not followed by any such method header and may instead be followed directly by native code with no intervening padding. In that case, we risk interpreting that native code as part of the preceding method, with potentially bizarre results. The reason for the compareIpToMethodBounds assumption was that methods which throw exceptions as their last instruction generate a non-returning call, which nonetheless push a return address on the stack which points past the end of the method, and the unwinder needs to know that return address belongs to that method. A better solution is to add an extra trap instruction to the end of such methods, which is what this patch does. --- src/arm.cpp | 14 ++++++-- src/assembler.h | 7 ++-- src/bootimage.cpp | 4 --- src/compile.cpp | 84 +++++++++++++++++++++++++++++------------------ src/compiler.cpp | 21 +++++++----- src/compiler.h | 2 ++ src/machine.cpp | 4 +-- src/powerpc.cpp | 14 ++++++-- src/types.def | 1 + src/x86.cpp | 27 ++++++++++----- 10 files changed, 114 insertions(+), 64 deletions(-) diff --git a/src/arm.cpp b/src/arm.cpp index af1c6398a2..9f815765b4 100644 --- a/src/arm.cpp +++ b/src/arm.cpp @@ -136,6 +136,7 @@ inline int ble(int offset) { return SETCOND(b(offset), LE); } inline int bge(int offset) { return SETCOND(b(offset), GE); } inline int blo(int offset) { return SETCOND(b(offset), CC); } inline int bhs(int offset) { return SETCOND(b(offset), CS); } +inline int bkpt() { return 0xe1200070; } // todo: macro-ify } const uint64_t MASK_LO32 = 0xffffffff; @@ -1616,6 +1617,12 @@ return_(Context* c) emit(c, bx(LinkRegister)); } +void +trap(Context* c) +{ + emit(c, bkpt()); +} + void memoryBarrier(Context*) {} @@ -1629,7 +1636,7 @@ argumentFootprint(unsigned footprint) void nextFrame(ArchitectureContext* c, uint32_t* start, unsigned size UNUSED, - unsigned footprint, void* link, void*, + unsigned footprint, void* link, bool, unsigned targetParameterFootprint UNUSED, void** ip, void** stack) { assert(c, *ip >= start); @@ -1703,6 +1710,7 @@ populateTables(ArchitectureContext* c) zo[LoadBarrier] = memoryBarrier; zo[StoreStoreBarrier] = memoryBarrier; zo[StoreLoadBarrier] = memoryBarrier; + zo[Trap] = trap; uo[index(c, LongCall, C)] = CAST1(longCallC); @@ -1922,12 +1930,12 @@ class MyArchitecture: public Assembler::Architecture { } virtual void nextFrame(void* start, unsigned size, unsigned footprint, - void* link, void* stackLimit, + void* link, bool mostRecent, unsigned targetParameterFootprint, void** ip, void** stack) { ::nextFrame(&c, static_cast(start), size, footprint, link, - stackLimit, targetParameterFootprint, ip, stack); + mostRecent, targetParameterFootprint, ip, stack); } virtual void* frameIp(void* stack) { diff --git a/src/assembler.h b/src/assembler.h index 1566df6cd7..07e02a31d5 100644 --- a/src/assembler.h +++ b/src/assembler.h @@ -32,10 +32,11 @@ enum Operation { Return, LoadBarrier, StoreStoreBarrier, - StoreLoadBarrier + StoreLoadBarrier, + Trap }; -const unsigned OperationCount = StoreLoadBarrier + 1; +const unsigned OperationCount = Trap + 1; enum UnaryOperation { Call, @@ -367,7 +368,7 @@ class Assembler { virtual unsigned alignFrameSize(unsigned sizeInWords) = 0; virtual void nextFrame(void* start, unsigned size, unsigned footprint, - void* link, void* stackLimit, + void* link, bool mostRecent, unsigned targetParameterFootprint, void** ip, void** stack) = 0; virtual void* frameIp(void* stack) = 0; diff --git a/src/bootimage.cpp b/src/bootimage.cpp index 21a1da5f22..0888fc7f81 100644 --- a/src/bootimage.cpp +++ b/src/bootimage.cpp @@ -686,10 +686,6 @@ makeCodeImage(Thread* t, Zone* zone, BootImage* image, uint8_t* code, } for (; methods; methods = pairSecond(t, methods)) { - intptr_t address = codeCompiled(t, methodCode(t, pairFirst(t, methods))); - reinterpret_cast(address)[-1] - = targetVW(reinterpret_cast(address)[-1]); - codeCompiled(t, methodCode(t, pairFirst(t, methods))) -= reinterpret_cast(code); } diff --git a/src/compile.cpp b/src/compile.cpp index 154e80f69b..395e7a3114 100644 --- a/src/compile.cpp +++ b/src/compile.cpp @@ -176,8 +176,8 @@ class MyThread: public Thread { Context(t, ip, stack, continuation, trace), t(t), link(0), - javaStackLimit(0), - next(t->traceContext) + next(t->traceContext), + methodIsMostRecent(false) { t->traceContext = this; } @@ -186,8 +186,8 @@ class MyThread: public Thread { Context(t, t->ip, t->stack, t->continuation, t->trace), t(t), link(link), - javaStackLimit(0), - next(t->traceContext) + next(t->traceContext), + methodIsMostRecent(false) { t->traceContext = this; } @@ -198,8 +198,8 @@ class MyThread: public Thread { MyThread* t; void* link; - void* javaStackLimit; TraceContext* next; + bool methodIsMostRecent; }; static void doTransition(MyThread* t, void* ip, void* stack, @@ -353,18 +353,18 @@ root(Thread* t, Root root); void setRoot(Thread* t, Root root, object value); -unsigned -compiledSize(intptr_t address) -{ - return reinterpret_cast(address)[-1]; -} - intptr_t methodCompiled(Thread* t, object method) { return codeCompiled(t, methodCode(t, method)); } +unsigned +methodCompiledSize(Thread* t, object method) +{ + return codeCompiledSize(t, methodCode(t, method)); +} + intptr_t compareIpToMethodBounds(Thread* t, intptr_t ip, object method) { @@ -374,13 +374,13 @@ compareIpToMethodBounds(Thread* t, intptr_t ip, object method) fprintf(stderr, "find %p in (%p,%p)\n", reinterpret_cast(ip), reinterpret_cast(start), - reinterpret_cast(start + compiledSize(start))); + reinterpret_cast(start + methodCompiledSize(t, method))); } if (ip < start) { return -1; } else if (ip < start + static_cast - (compiledSize(start) + TargetBytesPerWord)) + (methodCompiledSize(t, method))) { return 0; } else { @@ -427,19 +427,20 @@ alignedFrameSize(MyThread* t, object method) } void -nextFrame(MyThread* t, void** ip, void** sp, object method, object target) +nextFrame(MyThread* t, void** ip, void** sp, object method, object target, + bool mostRecent) { object code = methodCode(t, method); intptr_t start = codeCompiled(t, code); void* link; - void* javaStackLimit; + bool methodIsMostRecent; if (t->traceContext) { link = t->traceContext->link; - javaStackLimit = t->traceContext->javaStackLimit; + methodIsMostRecent = mostRecent and t->traceContext->methodIsMostRecent; } else { link = 0; - javaStackLimit = 0; + methodIsMostRecent = false; } // fprintf(stderr, "nextFrame %s.%s%s target %s.%s%s ip %p sp %p\n", @@ -458,8 +459,8 @@ nextFrame(MyThread* t, void** ip, void** sp, object method, object target) // *ip, *sp); t->arch->nextFrame - (reinterpret_cast(start), compiledSize(start), - alignedFrameSize(t, method), link, javaStackLimit, + (reinterpret_cast(start), codeCompiledSize(t, code), + alignedFrameSize(t, method), link, methodIsMostRecent, target ? methodParameterFootprint(t, target) : -1, ip, sp); // fprintf(stderr, "next frame ip %p sp %p\n", *ip, *sp); @@ -514,6 +515,7 @@ class MyStackWalker: public Processor::StackWalker { state(Start), method_(0), target(0), + count_(0), protector(this) { if (t->traceContext) { @@ -538,6 +540,7 @@ class MyStackWalker: public Processor::StackWalker { method_(w->method_), target(w->target), continuation(w->continuation), + count_(0), protector(this) { } @@ -609,13 +612,15 @@ class MyStackWalker: public Processor::StackWalker { } void next() { + expect(t, count_ <= StackSizeInWords); + switch (state) { case Continuation: continuation = continuationNext(t, continuation); break; case Method: - nextFrame(t, &ip_, &stack, method_, target); + nextFrame(t, &ip_, &stack, method_, target, count_ == 0); break; case NativeMethod: @@ -625,6 +630,8 @@ class MyStackWalker: public Processor::StackWalker { abort(t); } + ++ count_; + state = Next; } @@ -671,6 +678,7 @@ class MyStackWalker: public Processor::StackWalker { object method_; object target; object continuation; + unsigned count_; MyProtector protector; }; @@ -2065,6 +2073,7 @@ findUnwindTarget(MyThread* t, void** targetIp, void** targetFrame, } object target = t->trace->targetMethod; + bool mostRecent = true; *targetIp = 0; while (*targetIp == 0) { @@ -2075,7 +2084,7 @@ findUnwindTarget(MyThread* t, void** targetIp, void** targetFrame, if (handler) { *targetIp = handler; - nextFrame(t, &ip, &stack, method, target); + nextFrame(t, &ip, &stack, method, target, mostRecent); void** sp = static_cast(stackForFrame(t, stack, method)) + t->arch->frameReturnAddressSize(); @@ -2089,7 +2098,7 @@ findUnwindTarget(MyThread* t, void** targetIp, void** targetFrame, t->exception = 0; } else { - nextFrame(t, &ip, &stack, method, target); + nextFrame(t, &ip, &stack, method, target, mostRecent); if (t->exception) { releaseLock(t, method, stack); @@ -2137,6 +2146,8 @@ findUnwindTarget(MyThread* t, void** targetIp, void** targetFrame, *targetContinuation = continuationNext(t, c); } } + + mostRecent = false; } } @@ -2160,6 +2171,8 @@ makeCurrentContinuation(MyThread* t, void** targetIp, void** targetStack) object last = 0; PROTECT(t, last); + bool mostRecent = true; + *targetIp = 0; while (*targetIp == 0) { object method = methodForIp(t, ip); @@ -2177,7 +2190,7 @@ makeCurrentContinuation(MyThread* t, void** targetIp, void** targetStack) } void* nextIp = ip; - nextFrame(t, &nextIp, &stack, method, target); + nextFrame(t, &nextIp, &stack, method, target, mostRecent); void** bottom = static_cast(stack) + t->arch->frameReturnAddressSize(); @@ -2215,6 +2228,8 @@ makeCurrentContinuation(MyThread* t, void** targetIp, void** targetStack) *targetStack = static_cast(stack) + t->arch->frameReturnAddressSize(); } + + mostRecent = false; } expect(t, last); @@ -4295,6 +4310,10 @@ compile(MyThread* t, Frame* initialFrame, unsigned ip, 0, Compiler::VoidType, 2, c->register_(t->arch->thread()), target); + + if (ip == codeLength(t, code)) { + c->trap(); + } } return; case bipush: @@ -6999,15 +7018,14 @@ finish(MyThread* t, FixedAllocator* allocator, Context* context) // we must acquire the class lock here at the latest unsigned codeSize = c->resolve - (allocator->base + allocator->offset + TargetBytesPerWord); + (allocator->base + allocator->offset); unsigned total = pad(codeSize, TargetBytesPerWord) - + pad(c->poolSize(), TargetBytesPerWord) + TargetBytesPerWord; + + pad(c->poolSize(), TargetBytesPerWord); target_uintptr_t* code = static_cast (allocator->allocate(total, TargetBytesPerWord)); - code[0] = codeSize; - uint8_t* start = reinterpret_cast(code + 1); + uint8_t* start = reinterpret_cast(code); context->executableAllocator = allocator; context->executableStart = code; @@ -7060,7 +7078,7 @@ finish(MyThread* t, FixedAllocator* allocator, Context* context) code = makeCode (t, 0, newExceptionHandlerTable, newLineNumberTable, - reinterpret_cast(start), codeMaxStack(t, code), + reinterpret_cast(start), codeSize, codeMaxStack(t, code), codeMaxLocals(t, code), 0); set(t, context->method, MethodCode, code); @@ -7764,6 +7782,7 @@ visitStack(MyThread* t, Heap::Visitor* v) MyThread::CallTrace* trace = t->trace; object targetMethod = (trace ? trace->targetMethod : 0); object target = targetMethod; + bool mostRecent = true; while (stack) { if (targetMethod) { @@ -7776,7 +7795,7 @@ visitStack(MyThread* t, Heap::Visitor* v) PROTECT(t, method); void* nextIp = ip; - nextFrame(t, &nextIp, &stack, method, target); + nextFrame(t, &nextIp, &stack, method, target, mostRecent); visitStackAndLocals(t, v, stack, method, ip); @@ -7797,6 +7816,8 @@ visitStack(MyThread* t, Heap::Visitor* v) } else { break; } + + mostRecent = false; } } @@ -8831,7 +8852,7 @@ class MyProcessor: public Processor { // we caught the thread in Java code - use the register values c.ip = ip; c.stack = stack; - c.javaStackLimit = stack; + c.methodIsMostRecent = true; } else if (target->transition) { // we caught the thread in native code while in the middle // of updating the context fields (MyThread::stack, etc.) @@ -9492,8 +9513,7 @@ fixupMethods(Thread* t, object map, BootImage* image UNUSED, uint8_t* code) logCompile (static_cast(t), reinterpret_cast(methodCompiled(t, method)), - reinterpret_cast - (methodCompiled(t, method))[-1], + methodCompiledSize(t, method), reinterpret_cast (&byteArrayBody(t, className(t, methodClass(t, method)), 0)), reinterpret_cast diff --git a/src/compiler.cpp b/src/compiler.cpp index 737f723da4..5a89b38342 100644 --- a/src/compiler.cpp +++ b/src/compiler.cpp @@ -4576,14 +4576,14 @@ appendTranslate(Context* c, BinaryOperation type, unsigned firstSize, } } -class BarrierEvent: public Event { +class OperationEvent: public Event { public: - BarrierEvent(Context* c, Operation op): + OperationEvent(Context* c, Operation op): Event(c), op(op) { } virtual const char* name() { - return "BarrierEvent"; + return "OperationEvent"; } virtual void compile(Context* c) { @@ -4594,9 +4594,10 @@ class BarrierEvent: public Event { }; void -appendBarrier(Context* c, Operation op) +appendOperation(Context* c, Operation op) { - append(c, new (c->zone->allocate(sizeof(BarrierEvent))) BarrierEvent(c, op)); + append + (c, new (c->zone->allocate(sizeof(OperationEvent))) OperationEvent(c, op)); } class MemoryEvent: public Event { @@ -6902,16 +6903,20 @@ class MyCompiler: public Compiler { return result; } + virtual void trap() { + appendOperation(&c, Trap); + } + virtual void loadBarrier() { - appendBarrier(&c, LoadBarrier); + appendOperation(&c, LoadBarrier); } virtual void storeStoreBarrier() { - appendBarrier(&c, StoreStoreBarrier); + appendOperation(&c, StoreStoreBarrier); } virtual void storeLoadBarrier() { - appendBarrier(&c, StoreLoadBarrier); + appendOperation(&c, StoreLoadBarrier); } virtual void compile(uintptr_t stackOverflowHandler, diff --git a/src/compiler.h b/src/compiler.h index 85deed9473..acac85b436 100644 --- a/src/compiler.h +++ b/src/compiler.h @@ -184,6 +184,8 @@ class Compiler { virtual Operand* f2i(unsigned aSize, unsigned resSize, Operand* a) = 0; virtual Operand* i2f(unsigned aSize, unsigned resSize, Operand* a) = 0; + virtual void trap() = 0; + virtual void loadBarrier() = 0; virtual void storeStoreBarrier() = 0; virtual void storeLoadBarrier() = 0; diff --git a/src/machine.cpp b/src/machine.cpp index ac2c5bdd51..258c873ec2 100644 --- a/src/machine.cpp +++ b/src/machine.cpp @@ -1310,7 +1310,7 @@ parseCode(Thread* t, Stream& s, object pool) unsigned maxLocals = s.read2(); unsigned length = s.read4(); - object code = makeCode(t, pool, 0, 0, 0, maxStack, maxLocals, length); + object code = makeCode(t, pool, 0, 0, 0, 0, maxStack, maxLocals, length); s.read(&codeBody(t, code, 0), length); PROTECT(t, code); @@ -2234,7 +2234,7 @@ boot(Thread* t) m->processor->boot(t, 0, 0); - { object bootCode = makeCode(t, 0, 0, 0, 0, 0, 0, 1); + { object bootCode = makeCode(t, 0, 0, 0, 0, 0, 0, 0, 1); codeBody(t, bootCode, 0) = impdep1; object bootMethod = makeMethod (t, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, bootCode); diff --git a/src/powerpc.cpp b/src/powerpc.cpp index 7e315b1f35..f63bd270c6 100644 --- a/src/powerpc.cpp +++ b/src/powerpc.cpp @@ -130,6 +130,7 @@ inline int cmpw(int ra, int rb) { return cmp(0, ra, rb); } inline int cmplw(int ra, int rb) { return cmpl(0, ra, rb); } inline int cmpwi(int ra, int i) { return cmpi(0, ra, i); } inline int cmplwi(int ra, int i) { return cmpli(0, ra, i); } +inline int trap() { return 0x7fe00008; } // todo: macro-ify } const int64_t MASK_LO32 = 0x0ffffffff; @@ -1907,6 +1908,12 @@ return_(Context* c) emit(c, blr()); } +void +trap(Context* c) +{ + emit(c, trap()); +} + void memoryBarrier(Context* c) { @@ -1923,7 +1930,7 @@ argumentFootprint(unsigned footprint) void nextFrame(ArchitectureContext* c UNUSED, int32_t* start, unsigned size, - unsigned footprint, void* link, void*, + unsigned footprint, void* link, bool, unsigned targetParameterFootprint, void** ip, void** stack) { assert(c, *ip >= start); @@ -1988,6 +1995,7 @@ populateTables(ArchitectureContext* c) zo[LoadBarrier] = memoryBarrier; zo[StoreStoreBarrier] = memoryBarrier; zo[StoreLoadBarrier] = memoryBarrier; + zo[Trap] = trap; uo[index(c, LongCall, C)] = CAST1(longCallC); @@ -2215,12 +2223,12 @@ class MyArchitecture: public Assembler::Architecture { } virtual void nextFrame(void* start, unsigned size, unsigned footprint, - void* link, void* stackLimit, + void* link, bool mostRecent, unsigned targetParameterFootprint, void** ip, void** stack) { ::nextFrame(&c, static_cast(start), size, footprint, link, - stackLimit, targetParameterFootprint, ip, stack); + mostRecent, targetParameterFootprint, ip, stack); } virtual void* frameIp(void* stack) { diff --git a/src/types.def b/src/types.def index c95ed03a0a..9b19ccd9bc 100644 --- a/src/types.def +++ b/src/types.def @@ -77,6 +77,7 @@ (object exceptionHandlerTable) (object lineNumberTable) (intptr_t compiled) + (uint32_t compiledSize) (uint16_t maxStack) (uint16_t maxLocals) (array uint8_t body)) diff --git a/src/x86.cpp b/src/x86.cpp index 89d6f5df25..5673b9da97 100644 --- a/src/x86.cpp +++ b/src/x86.cpp @@ -645,6 +645,12 @@ return_(Context* c) opcode(c, 0xc3); } +void +trap(Context* c) +{ + opcode(c, 0xcc); +} + void ignore(Context*) { } @@ -2163,8 +2169,8 @@ divideRR(Context* c, unsigned aSize, Assembler::Register* a, assert(c, a->low != rdx); c->client->save(rdx); - - maybeRex(c, aSize, a, b); + + maybeRex(c, aSize, a, b); opcode(c, 0x99); // cdq maybeRex(c, aSize, b, a); opcode(c, 0xf7, 0xf8 + regCode(a)); @@ -2180,8 +2186,8 @@ remainderRR(Context* c, unsigned aSize, Assembler::Register* a, assert(c, a->low != rdx); c->client->save(rdx); - - maybeRex(c, aSize, a, b); + + maybeRex(c, aSize, a, b); opcode(c, 0x99); // cdq maybeRex(c, aSize, b, a); opcode(c, 0xf7, 0xf8 + regCode(a)); @@ -2552,7 +2558,7 @@ read4(uint8_t* p) void nextFrame(ArchitectureContext* c UNUSED, uint8_t* start, unsigned size UNUSED, - unsigned footprint, void*, void* stackLimit, + unsigned footprint, void*, bool mostRecent, unsigned targetParameterFootprint, void** ip, void** stack) { assert(c, *ip >= start); @@ -2570,6 +2576,7 @@ nextFrame(ArchitectureContext* c UNUSED, uint8_t* start, unsigned size UNUSED, } if (instruction <= start) { + assert(c, mostRecent); *ip = static_cast(*stack)[0]; return; } @@ -2579,6 +2586,8 @@ nextFrame(ArchitectureContext* c UNUSED, uint8_t* start, unsigned size UNUSED, start += (TargetBytesPerWord == 4 ? 3 : 4); if (instruction <= start or *instruction == 0x5d) { + assert(c, mostRecent); + *ip = static_cast(*stack)[1]; *stack = static_cast(*stack) + 1; return; @@ -2590,8 +2599,7 @@ nextFrame(ArchitectureContext* c UNUSED, uint8_t* start, unsigned size UNUSED, return; } - unsigned offset = footprint + FrameHeaderSize - - (stackLimit == *stack ? 1 : 0); + unsigned offset = footprint + FrameHeaderSize - (mostRecent ? 1 : 0); if (TailCalls) { if (argumentFootprint(targetParameterFootprint) > StackAlignmentInWords) { @@ -2642,6 +2650,7 @@ populateTables(ArchitectureContext* c) zo[LoadBarrier] = ignore; zo[StoreStoreBarrier] = ignore; zo[StoreLoadBarrier] = storeLoadBarrier; + zo[Trap] = trap; uo[index(c, Call, C)] = CAST1(callC); uo[index(c, Call, R)] = CAST1(callR); @@ -2966,12 +2975,12 @@ class MyArchitecture: public Assembler::Architecture { } virtual void nextFrame(void* start, unsigned size, unsigned footprint, - void* link, void* stackLimit, + void* link, bool mostRecent, unsigned targetParameterFootprint, void** ip, void** stack) { local::nextFrame(&c, static_cast(start), size, footprint, - link, stackLimit, targetParameterFootprint, ip, stack); + link, mostRecent, targetParameterFootprint, ip, stack); } virtual void* frameIp(void* stack) {