This is a fix for a stackwalker_x86 issue which has to

do with FPO (frame-pointer-omission) optimized context
frames where the context frame represents a Windows
System call stub.



git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@971 4c0a9323-5329-0410-9bdc-e9ce6186880e

src/processor/basic_source_line_resolver_unittest.cc

@@ -200,6 +200,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolve)
   ASSERT_EQ(frame.source_line_base, 0x1000);
   windows_frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(windows_frame_info.get());
+  ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_FRAME_DATA);
   ASSERT_FALSE(windows_frame_info->allocates_base_pointer);
   ASSERT_EQ(windows_frame_info->program_string,
             "$eip 4 + ^ = $esp $ebp 8 + = $ebp $ebp ^ =");
@@ -219,6 +220,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolve)
   ASSERT_EQ(frame.source_line, 0);
   windows_frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(windows_frame_info.get());
+  ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_UNKNOWN);
   ASSERT_FALSE(windows_frame_info->allocates_base_pointer);
   ASSERT_TRUE(windows_frame_info->program_string.empty());
 
@@ -228,6 +230,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolve)
   ASSERT_TRUE(frame.source_file_name.empty());
   ASSERT_EQ(frame.source_line, 0);
   windows_frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
+  ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_FRAME_DATA);
   ASSERT_TRUE(windows_frame_info.get());
   ASSERT_FALSE(windows_frame_info->allocates_base_pointer);
   ASSERT_FALSE(windows_frame_info->program_string.empty());
@@ -351,6 +354,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolve)
   ASSERT_EQ(frame.source_line_base, 0x2180);
   windows_frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(windows_frame_info.get());
+  ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_FRAME_DATA);
   ASSERT_EQ(windows_frame_info->prolog_size, 1);
 
   frame.instruction = 0x216f;

src/processor/fast_source_line_resolver.cc

@@ -107,10 +107,15 @@ void FastSourceLineResolver::Module::LookupAddress(StackFrame *frame) const {
 // WFI: WindowsFrameInfo.
 // Returns a WFI object reading from a raw memory chunk of data
 WindowsFrameInfo FastSourceLineResolver::CopyWFI(const char *raw) {
-  // The first 4Bytes of int data are unused.
+  const WindowsFrameInfo::StackInfoTypes type =
-  // They corresponds to "int valid;" data member of WFI.
+     static_cast<const WindowsFrameInfo::StackInfoTypes>(
+         *reinterpret_cast<const int32_t*>(raw));
+
+  // The first 8 bytes of int data are unused.
+  // They correspond to "StackInfoTypes type_;" and "int valid;"
+  // data member of WFI.
   const u_int32_t *para_uint32 = reinterpret_cast<const u_int32_t*>(
-      raw + sizeof(int32_t));
+      raw + 2 * sizeof(int32_t));
 
   u_int32_t prolog_size = para_uint32[0];;
   u_int32_t epilog_size = para_uint32[1];
@@ -122,7 +127,8 @@ WindowsFrameInfo FastSourceLineResolver::CopyWFI(const char *raw) {
   bool allocates_base_pointer = (*boolean != 0);
   std::string program_string = boolean + 1;
 
-  return WindowsFrameInfo(prolog_size,
+  return WindowsFrameInfo(type,
+                          prolog_size,
                           epilog_size,
                           parameter_size,
                           saved_register_size,

src/processor/fast_source_line_resolver_unittest.cc

@@ -248,6 +248,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolve) {
   ASSERT_EQ(frame.source_line, 0);
   windows_frame_info.reset(fast_resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(windows_frame_info.get());
+  ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_UNKNOWN);
   ASSERT_FALSE(windows_frame_info->allocates_base_pointer);
   ASSERT_TRUE(windows_frame_info->program_string.empty());
 
@@ -258,6 +259,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolve) {
   ASSERT_EQ(frame.source_line, 0);
   windows_frame_info.reset(fast_resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(windows_frame_info.get());
+  ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_FRAME_DATA);
   ASSERT_FALSE(windows_frame_info->allocates_base_pointer);
   ASSERT_FALSE(windows_frame_info->program_string.empty());
 
@@ -380,6 +382,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolve) {
   ASSERT_EQ(frame.source_line_base, 0x2180);
   windows_frame_info.reset(fast_resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(windows_frame_info.get());
+  ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_FRAME_DATA);
   ASSERT_EQ(windows_frame_info->prolog_size, 1);
 
   frame.instruction = 0x216f;

src/processor/module_comparer.cc

@@ -240,6 +240,7 @@ bool ModuleComparer::ComparePubSymbol(const BasicPubSymbol* basic_ps,
 
 bool ModuleComparer::CompareWFI(const WindowsFrameInfo& wfi1,
                                const WindowsFrameInfo& wfi2) const {
+  ASSERT_TRUE(wfi1.type_ == wfi2.type_);
   ASSERT_TRUE(wfi1.valid == wfi2.valid);
   ASSERT_TRUE(wfi1.prolog_size == wfi2.prolog_size);
   ASSERT_TRUE(wfi1.epilog_size == wfi2.epilog_size);

src/processor/simple_serializer-inl.h

@@ -132,6 +132,7 @@ class SimpleSerializer<WindowsFrameInfo> {
  public:
   static size_t SizeOf(const WindowsFrameInfo &wfi) {
     unsigned int size = 0;
+    size += sizeof(int32_t);  // wfi.type_
     size += SimpleSerializer<int32_t>::SizeOf(wfi.valid);
     size += SimpleSerializer<u_int32_t>::SizeOf(wfi.prolog_size);
     size += SimpleSerializer<u_int32_t>::SizeOf(wfi.epilog_size);
@@ -144,6 +145,8 @@ class SimpleSerializer<WindowsFrameInfo> {
     return size;
   }
   static char *Write(const WindowsFrameInfo &wfi, char *dest) {
+    dest = SimpleSerializer<int32_t>::Write(
+        static_cast<const int32_t>(wfi.type_), dest);
     dest = SimpleSerializer<int32_t>::Write(wfi.valid, dest);
     dest = SimpleSerializer<u_int32_t>::Write(wfi.prolog_size, dest);
     dest = SimpleSerializer<u_int32_t>::Write(wfi.epilog_size, dest);

src/processor/stackwalker_x86.cc

@@ -208,10 +208,26 @@ StackFrameX86 *StackwalkerX86::GetCallerByWindowsFrameInfo(
                             last_frame_callee_parameter_size +
                             last_frame_info->local_size +
                             last_frame_info->saved_register_size;
-  u_int32_t found; // dummy value
+
+  u_int32_t raSearchStartOld = raSearchStart;
+  u_int32_t found = 0; // dummy value
   // Scan up to three words above the calculated search value, in case
   // the stack was aligned to a quadword boundary.
+  if (ScanForReturnAddress(raSearchStart, &raSearchStart, &found, 3) &&
+      last_frame->trust == StackFrame::FRAME_TRUST_CONTEXT &&
+      last_frame->windows_frame_info != NULL &&
+      last_frame_info->type_ == WindowsFrameInfo::STACK_INFO_FPO &&
+      raSearchStartOld == raSearchStart &&
+      found == last_frame->context.eip) {
+    // The context frame represents an FPO-optimized Windows system call.
+    // On the top of the stack we have a pointer to the current instruction.
+    // This means that the callee has returned but the return address is still
+    // on the top of the stack which is very atypical situaltion.
+    // Skip one slot from the stack and do another scan in order to get the
+    // actual return address.
+    raSearchStart += 4;
     ScanForReturnAddress(raSearchStart, &raSearchStart, &found, 3);
+  }
 
   // The difference between raSearch and raSearchStart is unknown,
   // but making them the same seems to work well in practice.

src/processor/stackwalker_x86_unittest.cc

@@ -69,7 +69,11 @@ class StackwalkerX86Fixture {
       // Give the two modules reasonable standard locations and names
       // for tests to play with.
       module1(0x40000000, 0x10000, "module1", "version1"),
-      module2(0x50000000, 0x10000, "module2", "version2") {
+      module2(0x50000000, 0x10000, "module2", "version2"),
+      module3(0x771d0000, 0x180000, "module3", "version3"),
+      module4(0x75f90000, 0x46000, "module4", "version4"),
+      module5(0x75730000, 0x110000, "module5", "version5"),
+      module6(0x647f0000, 0x1ba8000, "module6", "version6") {
     // Identify the system as a Linux system.
     system_info.os = "Linux";
     system_info.os_short = "linux";
@@ -83,6 +87,10 @@ class StackwalkerX86Fixture {
     // Create some modules with some stock debugging information.
     modules.Add(&module1);
     modules.Add(&module2);
+    modules.Add(&module3);
+    modules.Add(&module4);
+    modules.Add(&module5);
+    modules.Add(&module6);
 
     // By default, none of the modules have symbol info; call
     // SetModuleSymbols to override this.
@@ -122,6 +130,10 @@ class StackwalkerX86Fixture {
   MockMemoryRegion stack_region;
   MockCodeModule module1;
   MockCodeModule module2;
+  MockCodeModule module3;
+  MockCodeModule module4;
+  MockCodeModule module5;
+  MockCodeModule module6;
   MockCodeModules modules;
   MockSymbolSupplier supplier;
   BasicSourceLineResolver resolver;
@@ -196,6 +208,7 @@ TEST_F(GetCallerFrame, Traditional) {
   frames = call_stack.frames();
   ASSERT_EQ(2U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     EXPECT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -203,7 +216,9 @@ TEST_F(GetCallerFrame, Traditional) {
     EXPECT_EQ(0x4000c7a5U, frame0->context.eip);
     EXPECT_EQ(frame0_ebp.Value(), frame0->context.ebp);
     EXPECT_EQ(NULL, frame0->windows_frame_info);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_FP, frame1->trust);
     ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP
@@ -215,6 +230,7 @@ TEST_F(GetCallerFrame, Traditional) {
     EXPECT_EQ(frame1_ebp.Value(), frame1->context.ebp);
     EXPECT_EQ(NULL, frame1->windows_frame_info);
   }
+}
 
 // Walk a traditional frame, but use a bogus %ebp value, forcing a scan
 // of the stack for something that looks like a return address.
@@ -247,6 +263,7 @@ TEST_F(GetCallerFrame, TraditionalScan) {
   frames = call_stack.frames();
   ASSERT_EQ(2U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -255,7 +272,9 @@ TEST_F(GetCallerFrame, TraditionalScan) {
     EXPECT_EQ(stack_section.start().Value(), frame0->context.esp);
     EXPECT_EQ(0xd43eed6eU, frame0->context.ebp);
     EXPECT_EQ(NULL, frame0->windows_frame_info);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_SCAN, frame1->trust);
     // I'd argue that CONTEXT_VALID_EBP shouldn't be here, since the
@@ -272,6 +291,7 @@ TEST_F(GetCallerFrame, TraditionalScan) {
     EXPECT_EQ(0xd43eed6eU, frame1->context.ebp);
     EXPECT_EQ(NULL, frame1->windows_frame_info);
   }
+}
 
 // Force scanning for a return address a long way down the stack
 TEST_F(GetCallerFrame, TraditionalScanLongWay) {
@@ -304,6 +324,7 @@ TEST_F(GetCallerFrame, TraditionalScanLongWay) {
   frames = call_stack.frames();
   ASSERT_EQ(2U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -312,7 +333,9 @@ TEST_F(GetCallerFrame, TraditionalScanLongWay) {
     EXPECT_EQ(stack_section.start().Value(), frame0->context.esp);
     EXPECT_EQ(0xd43eed6eU, frame0->context.ebp);
     EXPECT_EQ(NULL, frame0->windows_frame_info);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_SCAN, frame1->trust);
     // I'd argue that CONTEXT_VALID_EBP shouldn't be here, since the
@@ -329,6 +352,7 @@ TEST_F(GetCallerFrame, TraditionalScanLongWay) {
     EXPECT_EQ(0xd43eed6eU, frame1->context.ebp);
     EXPECT_EQ(NULL, frame1->windows_frame_info);
   }
+}
 
 // Use Windows frame data (a "STACK WIN 4" record, from a
 // FrameTypeFrameData DIA record) to walk a stack frame.
@@ -371,6 +395,7 @@ TEST_F(GetCallerFrame, WindowsFrameData) {
   frames = call_stack.frames();
   ASSERT_EQ(2U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -379,7 +404,9 @@ TEST_F(GetCallerFrame, WindowsFrameData) {
     EXPECT_EQ(stack_section.start().Value(), frame0->context.esp);
     EXPECT_EQ(0xf052c1deU, frame0->context.ebp);
     EXPECT_TRUE(frame0->windows_frame_info != NULL);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CFI, frame1->trust);
     ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP
@@ -398,6 +425,7 @@ TEST_F(GetCallerFrame, WindowsFrameData) {
     EXPECT_EQ(0x630891beU, frame1->context.edi);
     EXPECT_EQ(NULL, frame1->windows_frame_info);
   }
+}
 
 // Use Windows frame data (a "STACK WIN 4" record, from a
 // FrameTypeFrameData DIA record) to walk a stack frame where the stack
@@ -438,6 +466,7 @@ TEST_F(GetCallerFrame, WindowsFrameDataAligned) {
   frames = call_stack.frames();
   ASSERT_EQ(2U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -446,7 +475,9 @@ TEST_F(GetCallerFrame, WindowsFrameDataAligned) {
     EXPECT_EQ(stack_section.start().Value(), frame0->context.esp);
     EXPECT_EQ(0xf052c1deU, frame0->context.ebp);
     EXPECT_TRUE(frame0->windows_frame_info != NULL);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CFI, frame1->trust);
     ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP
@@ -459,6 +490,7 @@ TEST_F(GetCallerFrame, WindowsFrameDataAligned) {
     EXPECT_EQ(frame1_ebp.Value(), frame1->context.ebp);
     EXPECT_EQ(NULL, frame1->windows_frame_info);
   }
+}
 
 // Use Windows frame data (a "STACK WIN 4" record, from a
 // FrameTypeFrameData DIA record) to walk a frame, and depend on the
@@ -516,6 +548,7 @@ TEST_F(GetCallerFrame, WindowsFrameDataParameterSize) {
   frames = call_stack.frames();
   ASSERT_EQ(3U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -531,8 +564,12 @@ TEST_F(GetCallerFrame, WindowsFrameDataParameterSize) {
     ASSERT_TRUE(frame0->windows_frame_info != NULL);
     EXPECT_EQ(WindowsFrameInfo::VALID_PARAMETER_SIZE,
               frame0->windows_frame_info->valid);
+    EXPECT_EQ(WindowsFrameInfo::STACK_INFO_UNKNOWN,
+              frame0->windows_frame_info->type_);
     EXPECT_EQ(12U, frame0->windows_frame_info->parameter_size);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_FP, frame1->trust);
     ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP
@@ -548,10 +585,14 @@ TEST_F(GetCallerFrame, WindowsFrameDataParameterSize) {
     EXPECT_EQ(0x5000aa85U, frame1->function_base);
     ASSERT_TRUE(frame1->windows_frame_info != NULL);
     EXPECT_EQ(WindowsFrameInfo::VALID_ALL, frame1->windows_frame_info->valid);
+    EXPECT_EQ(WindowsFrameInfo::STACK_INFO_FRAME_DATA,
+              frame1->windows_frame_info->type_);
     // This should not see the 0xbeef parameter size from the FUNC
     // record, but should instead see the STACK WIN record.
     EXPECT_EQ(4U, frame1->windows_frame_info->parameter_size);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame2 = static_cast<StackFrameX86 *>(frames->at(2));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CFI, frame2->trust);
     ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP
@@ -567,6 +608,7 @@ TEST_F(GetCallerFrame, WindowsFrameDataParameterSize) {
     EXPECT_EQ(NULL, frame2->module);
     EXPECT_EQ(NULL, frame2->windows_frame_info);
   }
+}
 
 // Use Windows frame data (a "STACK WIN 4" record, from a
 // FrameTypeFrameData DIA record) to walk a stack frame, where the
@@ -600,6 +642,7 @@ TEST_F(GetCallerFrame, WindowsFrameDataScan) {
   frames = call_stack.frames();
   ASSERT_EQ(2U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -608,7 +651,9 @@ TEST_F(GetCallerFrame, WindowsFrameDataScan) {
     EXPECT_EQ(stack_section.start().Value(), frame0->context.esp);
     EXPECT_EQ(0x2ae314cdU, frame0->context.ebp);
     EXPECT_TRUE(frame0->windows_frame_info != NULL);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_SCAN, frame1->trust);
     // I'd argue that CONTEXT_VALID_EBP shouldn't be here, since the walker
@@ -624,6 +669,7 @@ TEST_F(GetCallerFrame, WindowsFrameDataScan) {
     EXPECT_EQ(frame1_esp.Value(), frame1->context.esp);
     EXPECT_TRUE(frame1->windows_frame_info != NULL);
   }
+}
 
 // Use Windows frame data (a "STACK WIN 4" record, from a
 // FrameTypeFrameData DIA record) to walk a stack frame, where the
@@ -679,6 +725,7 @@ TEST_F(GetCallerFrame, WindowsFrameDataBadEIPScan) {
   frames = call_stack.frames();
   ASSERT_EQ(2U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -687,7 +734,9 @@ TEST_F(GetCallerFrame, WindowsFrameDataBadEIPScan) {
     EXPECT_EQ(stack_section.start().Value(), frame0->context.esp);
     EXPECT_EQ(frame0_ebp.Value(), frame0->context.ebp);
     EXPECT_TRUE(frame0->windows_frame_info != NULL);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CFI_SCAN, frame1->trust);
     // I'd argue that CONTEXT_VALID_EBP shouldn't be here, since the
@@ -704,6 +753,7 @@ TEST_F(GetCallerFrame, WindowsFrameDataBadEIPScan) {
     EXPECT_EQ(frame1_ebp.Value(), frame1->context.ebp);
     EXPECT_TRUE(frame1->windows_frame_info != NULL);
   }
+}
 
 // Use Windows FrameTypeFPO data to walk a stack frame for a function that
 // does not modify %ebp from the value it had in the caller.
@@ -742,6 +792,7 @@ TEST_F(GetCallerFrame, WindowsFPOUnchangedEBP) {
   frames = call_stack.frames();
   ASSERT_EQ(2U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -756,8 +807,12 @@ TEST_F(GetCallerFrame, WindowsFPOUnchangedEBP) {
     // produced a fully populated WindowsFrameInfo structure.
     ASSERT_TRUE(frame0->windows_frame_info != NULL);
     EXPECT_EQ(WindowsFrameInfo::VALID_ALL, frame0->windows_frame_info->valid);
+    EXPECT_EQ(WindowsFrameInfo::STACK_INFO_FPO,
+              frame0->windows_frame_info->type_);
     EXPECT_EQ(0x10U, frame0->windows_frame_info->local_size);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CFI, frame1->trust);
     ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP
@@ -772,6 +827,7 @@ TEST_F(GetCallerFrame, WindowsFPOUnchangedEBP) {
     EXPECT_EQ("", frame1->function_name);
     EXPECT_EQ(NULL, frame1->windows_frame_info);
   }
+}
 
 // Use Windows FrameTypeFPO data to walk a stack frame for a function
 // that uses %ebp for its own purposes, saving the value it had in the
@@ -812,6 +868,7 @@ TEST_F(GetCallerFrame, WindowsFPOUsedEBP) {
   frames = call_stack.frames();
   ASSERT_EQ(2U, frames->size());
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
     ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -826,9 +883,13 @@ TEST_F(GetCallerFrame, WindowsFPOUsedEBP) {
     // produced a fully populated WindowsFrameInfo structure.
     ASSERT_TRUE(frame0->windows_frame_info != NULL);
     EXPECT_EQ(WindowsFrameInfo::VALID_ALL, frame0->windows_frame_info->valid);
+    EXPECT_EQ(WindowsFrameInfo::STACK_INFO_FPO,
+              frame0->windows_frame_info->type_);
     EXPECT_EQ("", frame0->windows_frame_info->program_string);
     EXPECT_TRUE(frame0->windows_frame_info->allocates_base_pointer);
+  }
 
+  {  // To avoid reusing locals by mistake
     StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
     EXPECT_EQ(StackFrame::FRAME_TRUST_CFI, frame1->trust);
     ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP
@@ -843,6 +904,153 @@ TEST_F(GetCallerFrame, WindowsFPOUsedEBP) {
     EXPECT_EQ("", frame1->function_name);
     EXPECT_EQ(NULL, frame1->windows_frame_info);
   }
+}
+
+// This is a regression unit test which covers a bug which has to do with
+// FPO-optimized Windows system call stubs in the context frame.  There is
+// a more recent Windows system call dispatch mechanism which differs from
+// the one which is being tested here.  The newer system call dispatch
+// mechanism creates an extra context frame (KiFastSystemCallRet).
+TEST_F(GetCallerFrame, WindowsFPOSystemCall) {
+  SetModuleSymbols(&module3,  // ntdll.dll
+                   "PUBLIC 1f8ac c ZwWaitForSingleObject\n"
+                   "STACK WIN 0 1f8ac 1b 0 0 c 0 0 0 0 0\n");
+  SetModuleSymbols(&module4,  // kernelbase.dll
+                   "PUBLIC 109f9 c WaitForSingleObjectEx\n"
+                   "PUBLIC 36590 0 _except_handler4\n"
+                   "STACK WIN 4 109f9 df c 0 c c 48 0 1 $T0 $ebp = $eip "
+                   "$T0 4 + ^ = $ebp $T0 ^ = $esp $T0 8 + = $L "
+                   "$T0 .cbSavedRegs - = $P $T0 8 + .cbParams + =\n"
+                   "STACK WIN 4 36590 154 17 0 10 0 14 0 1 $T0 $ebp = $eip "
+                   "$T0 4 + ^ = $ebp $T0 ^ = $esp $T0 8 + = $L $T0 "
+                   ".cbSavedRegs - = $P $T0 8 + .cbParams + =\n");
+  SetModuleSymbols(&module5,  // kernel32.dll
+                   "PUBLIC 11136 8 WaitForSingleObject\n"
+                   "PUBLIC 11151 c WaitForSingleObjectExImplementation\n"
+                   "STACK WIN 4 11136 16 5 0 8 0 0 0 1 $T0 $ebp = $eip "
+                   "$T0 4 + ^ = $ebp $T0 ^ = $esp $T0 8 + = $L "
+                   "$T0 .cbSavedRegs - = $P $T0 8 + .cbParams + =\n"
+                   "STACK WIN 4 11151 7a 5 0 c 0 0 0 1 $T0 $ebp = $eip "
+                   "$T0 4 + ^ = $ebp $T0 ^ = $esp $T0 8 + = $L "
+                   "$T0 .cbSavedRegs - = $P $T0 8 + .cbParams + =\n");
+  SetModuleSymbols(&module6,  // chrome.dll
+                   "FILE 7038 some_file_name.h\n"
+                   "FILE 839776 some_file_name.cc\n"
+                   "FUNC 217fda 17 4 function_217fda\n"
+                   "217fda 4 102 839776\n"
+                   "FUNC 217ff1 a 4 function_217ff1\n"
+                   "217ff1 0 594 7038\n"
+                   "217ff1 a 596 7038\n"
+                   "STACK WIN 0 217ff1 a 0 0 4 0 0 0 0 0\n");
+
+  Label frame0_esp, frame1_esp;
+  Label frame1_ebp, frame2_ebp, frame3_ebp;
+  stack_section.start() = 0x002ff290;
+  stack_section
+    .Mark(&frame0_esp)
+    .D32(0x771ef8c1)    // EIP in frame 0 (system call)
+    .D32(0x75fa0a91)    // return address of frame 0
+    .Mark(&frame1_esp)
+    .D32(0x000017b0)    // args to child
+    .D32(0x00000000)
+    .D32(0x002ff2d8)
+    .D32(0x88014a2e)
+    .D32(0x002ff364)
+    .D32(0x000017b0)
+    .D32(0x00000000)
+    .D32(0x00000024)
+    .D32(0x00000001)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x9e3b9800)
+    .D32(0xfffffff7)
+    .D32(0x00000000)
+    .D32(0x002ff2a4)
+    .D32(0x64a07ff1)    // random value to be confused with a return address
+    .D32(0x002ff8dc)
+    .D32(0x75fc6590)    // random value to be confused with a return address
+    .D32(0xfdd2c6ea)
+    .D32(0x00000000)
+    .Mark(&frame1_ebp)
+    .D32(frame2_ebp)    // Child EBP
+    .D32(0x75741194)    // return address of frame 1
+    .D32(0x000017b0)    // args to child
+    .D32(0x0036ee80)
+    .D32(0x00000000)
+    .D32(0x65bc7d14)
+    .Mark(&frame2_ebp)
+    .D32(frame3_ebp)    // Child EBP
+    .D32(0x75741148)    // return address of frame 2
+    .D32(0x000017b0)    // args to child
+    .D32(0x0036ee80)
+    .D32(0x00000000)
+    .Mark(&frame3_ebp)
+    .D32(0)             // saved %ebp (stack end)
+    .D32(0);            // saved %eip (stack end)
+
+  RegionFromSection();
+  raw_context.eip = 0x771ef8c1;  // in ntdll::ZwWaitForSingleObject
+  raw_context.esp = stack_section.start().Value();
+  ASSERT_TRUE(raw_context.esp == frame0_esp.Value());
+  raw_context.ebp = frame1_ebp.Value();
+
+  StackwalkerX86 walker(&system_info, &raw_context, &stack_region, &modules,
+                        &supplier, &resolver);
+  ASSERT_TRUE(walker.Walk(&call_stack));
+  frames = call_stack.frames();
+
+  ASSERT_EQ(4U, frames->size());
+
+  {  // To avoid reusing locals by mistake
+    StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
+    EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
+    ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
+    EXPECT_EQ(0x771ef8c1U, frame0->instruction);
+    EXPECT_EQ(0x771ef8c1U, frame0->context.eip);
+    EXPECT_EQ(frame0_esp.Value(), frame0->context.esp);
+    EXPECT_EQ(frame1_ebp.Value(), frame0->context.ebp);
+    EXPECT_EQ(&module3, frame0->module);
+    EXPECT_EQ("ZwWaitForSingleObject", frame0->function_name);
+    // The STACK WIN record for module3!ZwWaitForSingleObject should have
+    // produced a fully populated WindowsFrameInfo structure.
+    ASSERT_TRUE(frame0->windows_frame_info != NULL);
+    EXPECT_EQ(WindowsFrameInfo::VALID_ALL, frame0->windows_frame_info->valid);
+    EXPECT_EQ(WindowsFrameInfo::STACK_INFO_FPO,
+              frame0->windows_frame_info->type_);
+    EXPECT_EQ("", frame0->windows_frame_info->program_string);
+    EXPECT_FALSE(frame0->windows_frame_info->allocates_base_pointer);
+  }
+
+  {  // To avoid reusing locals by mistake
+    StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
+    EXPECT_EQ(StackFrame::FRAME_TRUST_CFI, frame1->trust);
+    ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP
+               | StackFrameX86::CONTEXT_VALID_ESP
+               | StackFrameX86::CONTEXT_VALID_EBP),
+              frame1->context_validity);
+    EXPECT_EQ(0x75fa0a91U, frame1->instruction + 1);
+    EXPECT_EQ(0x75fa0a91U, frame1->context.eip);
+    EXPECT_EQ(frame1_esp.Value(), frame1->context.esp);
+    EXPECT_EQ(frame1_ebp.Value(), frame1->context.ebp);
+    EXPECT_EQ(&module4, frame1->module);
+    EXPECT_EQ("WaitForSingleObjectEx", frame1->function_name);
+    // The STACK WIN record for module4!WaitForSingleObjectEx should have
+    // produced a fully populated WindowsFrameInfo structure.
+    ASSERT_TRUE(frame1->windows_frame_info != NULL);
+    EXPECT_EQ(WindowsFrameInfo::VALID_ALL, frame1->windows_frame_info->valid);
+    EXPECT_EQ(WindowsFrameInfo::STACK_INFO_FRAME_DATA,
+              frame1->windows_frame_info->type_);
+    EXPECT_EQ("$T0 $ebp = $eip $T0 4 + ^ = $ebp $T0 ^ = $esp $T0 8 + = $L "
+              "$T0 .cbSavedRegs - = $P $T0 8 + .cbParams + =",
+              frame1->windows_frame_info->program_string);
+    EXPECT_FALSE(frame1->windows_frame_info->allocates_base_pointer);
+  }
+}
 
 struct CFIFixture: public StackwalkerX86Fixture {
   CFIFixture() {
@@ -897,6 +1105,7 @@ struct CFIFixture: public StackwalkerX86Fixture {
     frames = call_stack.frames();
     ASSERT_EQ(2U, frames->size());
 
+    {  // To avoid reusing locals by mistake
       StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
       EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
       ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
@@ -906,7 +1115,9 @@ struct CFIFixture: public StackwalkerX86Fixture {
       ASSERT_EQ(WindowsFrameInfo::VALID_PARAMETER_SIZE,
                 frame0->windows_frame_info->valid);
       ASSERT_TRUE(frame0->cfi_frame_info != NULL);
+    }
 
+    {  // To avoid reusing locals by mistake
       StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
       EXPECT_EQ(StackFrame::FRAME_TRUST_CFI, frame1->trust);
       ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP |
@@ -924,6 +1135,7 @@ struct CFIFixture: public StackwalkerX86Fixture {
       EXPECT_EQ(expected.edi, frame1->context.edi);
       EXPECT_EQ("epictetus", frame1->function_name);
     }
+  }
 
   // The values the stack walker should find for the caller's registers.
   MDRawContextX86 expected;

src/processor/windows_frame_info.h

@@ -72,7 +72,8 @@ struct WindowsFrameInfo {
     STACK_INFO_UNKNOWN = -1
   };
 
-  WindowsFrameInfo() : valid(VALID_NONE),
+  WindowsFrameInfo() : type_(STACK_INFO_UNKNOWN),
+                     valid(VALID_NONE),
                      prolog_size(0),
                      epilog_size(0),
                      parameter_size(0),
@@ -82,7 +83,8 @@ struct WindowsFrameInfo {
                      allocates_base_pointer(0),
                      program_string() {}
 
-  WindowsFrameInfo(u_int32_t set_prolog_size,
+  WindowsFrameInfo(StackInfoTypes type,
+                 u_int32_t set_prolog_size,
                  u_int32_t set_epilog_size,
                  u_int32_t set_parameter_size,
                  u_int32_t set_saved_register_size,
@@ -90,7 +92,8 @@ struct WindowsFrameInfo {
                  u_int32_t set_max_stack_size,
                  int set_allocates_base_pointer,
                  const std::string set_program_string)
-      : valid(VALID_ALL),
+      : type_(type),
+        valid(VALID_ALL),
         prolog_size(set_prolog_size),
         epilog_size(set_epilog_size),
         parameter_size(set_parameter_size),
@@ -140,7 +143,8 @@ struct WindowsFrameInfo {
       allocates_base_pointer = strtoul(tokens[10], NULL, 16);
     }
 
-    return new WindowsFrameInfo(prolog_size,
+    return new WindowsFrameInfo(static_cast<StackInfoTypes>(type),
+                                prolog_size,
                                 epilog_size,
                                 parameter_size,
                                 saved_register_size,
@@ -152,6 +156,7 @@ struct WindowsFrameInfo {
 
   // CopyFrom makes "this" WindowsFrameInfo object identical to "that".
   void CopyFrom(const WindowsFrameInfo &that) {
+    type_ = that.type_;
     valid = that.valid;
     prolog_size = that.prolog_size;
     epilog_size = that.epilog_size;
@@ -166,10 +171,13 @@ struct WindowsFrameInfo {
   // Clears the WindowsFrameInfo object so that users will see it as though
   // it contains no information.
   void Clear() {
+    type_ = STACK_INFO_UNKNOWN;
     valid = VALID_NONE;
     program_string.erase();
   }
 
+  StackInfoTypes type_;
+
   // Identifies which fields in the structure are valid.  This is of
   // type Validity, but it is defined as an int because it's not
   // possible to OR values into an enumerated type.  Users must check