Handle ntdll only emitting PUBLIC at func entry

This handles a case encountered in ntdll.dll symbols for Windows 7,
where a PUBLIC would be emitted only for the entry point to the
function. The body of the function, however, is split in a PGO-ish
fashion to another remote location in the binary. Because of this, there
were large gaps in the RVA space that would be attributed to the "last"
function that happened to have an entry point before the gap. In
practice, something like this:

0x100 Func1
0x110 Func2
0x120 Func3
0x130 Func4
...
0x800 LaterFuncs

The bodies of Func1/2/3 tend to be implemented as a fast-path check,
followed by a jmp to somewhere in the range between 0x130 and 0x800.
Because no symbols are emitted for this range, everything is attributed
to Func4, causing crash misattribution.

In this CL, the change is: after emitting the entry point symbol, also
walk in the original OMAP entries through the untranslated binary, and
for each block until we resolve to a new symbol (via the same mechanism
as we found the entry point) emit another PUBLIC indicating that there's
another block that belongs to that symbol. This effectively breaks up
the "0x130 - 0x800" range above.

R=mark@chromium.org
BUG=chromium:678874

Change-Id: Ib3741abab2e7158c81e3e34bca4340ce4d3153a1
Reviewed-on: https://chromium-review.googlesource.com/446717
Reviewed-by: Mark Mentovai <mark@chromium.org>

3 files changed, 69 insertions, 1 deletions

diff --git a/src/common/windows/omap.cc b/src/common/windows/omap.cc
index 554a57c2..ba3ce86b 100644
--- a/src/common/windows/omap.cc
+++ b/src/common/windows/omap.cc
@@ -449,6 +449,27 @@ void BuildEndpointIndexMap(ImageMap* image_map) {
   }
 }
 
+void BuildSubsequentRVAMap(const OmapData &omap_data,
+                           std::map<DWORD, DWORD> *subsequent) {
+  assert(subsequent->empty());
+  const OmapFromTable &orig2tran =
+      reinterpret_cast<const OmapFromTable &>(omap_data.omap_from);
+
+  if (orig2tran.empty())
+    return;
+
+  for (size_t i = 0; i < orig2tran.size() - 1; ++i) {
+    // Expect that orig2tran is sorted.
+    if (orig2tran[i].rva_original >= orig2tran[i + 1].rva_original) {
+      fprintf(stderr, "OMAP 'from' table unexpectedly unsorted\n");
+      subsequent->clear();
+      return;
+    }
+    subsequent->insert(std::make_pair(orig2tran[i].rva_original,
+                                      orig2tran[i + 1].rva_original));
+  }
+}
+
 // Clips the given mapped range.
 void ClipMappedRangeOriginal(const AddressRange& clip_range,
                              MappedRange* mapped_range) {
@@ -576,6 +597,7 @@ void BuildImageMap(const OmapData& omap_data, ImageMap* image_map) {
 
   BuildMapping(omap_data, &image_map->mapping);
   BuildEndpointIndexMap(image_map);
+  BuildSubsequentRVAMap(omap_data, &image_map->subsequent_rva_block);
 }
 
 void MapAddressRange(const ImageMap& image_map,
@@ -691,4 +713,4 @@ void MapAddressRange(const ImageMap& image_map,
   return;
 }
 
-}  // namespace google_breakpad
\ No newline at end of file
+}  // namespace google_breakpad
diff --git a/src/common/windows/omap_internal.h b/src/common/windows/omap_internal.h
index 3f904d7a..2a4713d9 100644
--- a/src/common/windows/omap_internal.h
+++ b/src/common/windows/omap_internal.h
@@ -35,6 +35,7 @@
 #include <windows.h>
 #include <dia2.h>
 
+#include <map>
 #include <vector>
 
 namespace google_breakpad {
@@ -130,6 +131,8 @@ struct ImageMap {
   // an interval in |mapping| that contains the endpoint. Useful for doing
   // interval intersection queries.
   EndpointIndexMap endpoint_index_map;
+
+  std::map<DWORD, DWORD> subsequent_rva_block;
 };
 
 }  // namespace google_breakpad
diff --git a/src/common/windows/pdb_source_line_writer.cc b/src/common/windows/pdb_source_line_writer.cc
index 01f4ce3b..c2e22e40 100644
--- a/src/common/windows/pdb_source_line_writer.cc
+++ b/src/common/windows/pdb_source_line_writer.cc
@@ -122,6 +122,16 @@ class AutoImage {
   PLOADED_IMAGE img_;
 };
 
+bool SymbolsMatch(IDiaSymbol* a, IDiaSymbol* b) {
+  DWORD a_section, a_offset, b_section, b_offset;
+  if (FAILED(a->get_addressSection(&a_section)) ||
+      FAILED(a->get_addressOffset(&a_offset)) ||
+      FAILED(b->get_addressSection(&b_section)) ||
+      FAILED(b->get_addressOffset(&b_offset)))
+    return false;
+  return a_section == b_section && a_offset == b_offset;
+}
+
 bool CreateDiaDataSourceInstance(CComPtr<IDiaDataSource> &data_source) {
   if (SUCCEEDED(data_source.CoCreateInstance(CLSID_DiaSource))) {
     return true;
@@ -856,6 +866,39 @@ bool PDBSourceLineWriter::PrintCodePublicSymbol(IDiaSymbol *symbol) {
             stack_param_size > 0 ? stack_param_size : 0,
             name.m_str);
   }
+
+  // Now walk the function in the original untranslated space, asking DIA
+  // what function is at that location, stepping through OMAP blocks. If
+  // we're still in the same function, emit another entry, because the
+  // symbol could have been split into multiple pieces. If we've gotten to
+  // another symbol in the original address space, then we're done for
+  // this symbol. See https://crbug.com/678874.
+  for (;;) {
+    // This steps to the next block in the original image. Simply doing
+    // rva++ would also be correct, but would emit tons of unnecessary
+    // entries.
+    rva = image_map_.subsequent_rva_block[rva];
+    if (rva == 0)
+      break;
+
+    CComPtr<IDiaSymbol> next_sym = NULL;
+    LONG displacement;
+    if (FAILED(session_->findSymbolByRVAEx(rva, SymTagPublicSymbol, &next_sym,
+                                           &displacement))) {
+      break;
+    }
+
+    if (!SymbolsMatch(symbol, next_sym))
+      break;
+
+    AddressRangeVector next_ranges;
+    MapAddressRange(image_map_, AddressRange(rva, 1), &next_ranges);
+    for (size_t i = 0; i < next_ranges.size(); ++i) {
+      fprintf(output_, "PUBLIC %x %x %ws\n", next_ranges[i].rva,
+              stack_param_size > 0 ? stack_param_size : 0, name.m_str);
+    }
+  }
+
   return true;
 }