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

@@ -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<uint32_t*>(start), size, footprint, link,
-                stackLimit, targetParameterFootprint, ip, stack);
+                mostRecent, targetParameterFootprint, ip, stack);
   }
 
   virtual void* frameIp(void* stack) {

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;

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<target_uintptr_t*>(address)[-1]
-      = targetVW(reinterpret_cast<target_uintptr_t*>(address)[-1]);
-
     codeCompiled(t, methodCode(t, pairFirst(t, methods)))
       -= reinterpret_cast<uintptr_t>(code);
   }

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<target_uintptr_t*>(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<void*>(ip),
             reinterpret_cast<void*>(start),
-            reinterpret_cast<void*>(start + compiledSize(start)));
+            reinterpret_cast<void*>(start + methodCompiledSize(t, method)));
   }
 
   if (ip < start) {
     return -1;
   } else if (ip < start + static_cast<intptr_t>
-             (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<void*>(start), compiledSize(start),
+    (reinterpret_cast<void*>(start), codeCompiledSize(t, code),
-     alignedFrameSize(t, method), link, javaStackLimit,
+     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<void**>(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<void**>(stack)
           + t->arch->frameReturnAddressSize();
@@ -2215,6 +2228,8 @@ makeCurrentContinuation(MyThread* t, void** targetIp, void** targetStack)
       *targetStack = static_cast<void**>(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<target_uintptr_t*>
     (allocator->allocate(total, TargetBytesPerWord));
-  code[0] = codeSize;
+  uint8_t* start = reinterpret_cast<uint8_t*>(code);
-  uint8_t* start = reinterpret_cast<uint8_t*>(code + 1);
 
   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<uintptr_t>(start), codeMaxStack(t, code),
+       reinterpret_cast<uintptr_t>(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<MyThread*>(t),
                reinterpret_cast<uint8_t*>(methodCompiled(t, method)),
-               reinterpret_cast<uintptr_t*>
+               methodCompiledSize(t, method),
-               (methodCompiled(t, method))[-1],
                reinterpret_cast<char*>
                (&byteArrayBody(t, className(t, methodClass(t, method)), 0)),
                reinterpret_cast<char*>

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,

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;

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);

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<int32_t*>(start), size, footprint, link,
-                stackLimit, targetParameterFootprint, ip, stack);
+                mostRecent, targetParameterFootprint, ip, stack);
   }
 
   virtual void* frameIp(void* stack) {

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

src/x86.cpp

@@ -645,6 +645,12 @@ return_(Context* c)
   opcode(c, 0xc3);
 }
 
+void
+trap(Context* c)
+{
+  opcode(c, 0xcc);
+}
+
 void
 ignore(Context*)
 { }
@@ -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<void**>(*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<void**>(*stack)[1];
       *stack = static_cast<void**>(*stack) + 1;
       return;
@@ -2590,8 +2599,7 @@ nextFrame(ArchitectureContext* c UNUSED, uint8_t* start, unsigned size UNUSED,
     return;
   }
 
-  unsigned offset = footprint + FrameHeaderSize
+  unsigned offset = footprint + FrameHeaderSize - (mostRecent ? 1 : 0);
-    - (stackLimit == *stack ? 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<uint8_t*>(start), size, footprint,
-                     link, stackLimit, targetParameterFootprint, ip, stack);
+                     link, mostRecent, targetParameterFootprint, ip, stack);
   }
 
   virtual void* frameIp(void* stack) {