[tracing] Smaps provider uses fscanf instead of istream

base/trace_event/process_memory_maps_dump_provider.cc

Index: base/trace_event/process_memory_maps_dump_provider.cc
diff --git a/base/trace_event/process_memory_maps_dump_provider.cc b/base/trace_event/process_memory_maps_dump_provider.cc
index 38b2573e6b6b3a7ccbf232780d915415b500f940..4508273828929f62b71dacfff98c4933f9d3bcc5 100644
--- a/base/trace_event/process_memory_maps_dump_provider.cc
+++ b/base/trace_event/process_memory_maps_dump_provider.cc
@@ -4,35 +4,38 @@
 
 #include "base/trace_event/process_memory_maps_dump_provider.h"
 
-#include <cctype>
-#include <fstream>
-
 #include "base/logging.h"
-#include "base/process/process_metrics.h"
 #include "base/trace_event/process_memory_dump.h"
+
+#if defined(OS_LINUX) || defined(OS_ANDROID)

[Primiano Tucci (use gerrit), 2015/10/06 16:37:31]

Don't really need to hide these. Either you mask the entire file, or just leave
the ifdef guards where needed

[ssid, 2015/10/06 18:47:05]

Done.

+#include "base/format_macros.h"
+#include "base/strings/string_util.h"
 #include "base/trace_event/process_memory_maps.h"
+#endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
 namespace base {
 namespace trace_event {
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
 // static
-std::istream* ProcessMemoryMapsDumpProvider::proc_smaps_for_testing = nullptr;
+FILE* ProcessMemoryMapsDumpProvider::proc_smaps_for_testing = nullptr;
 
 namespace {
 
 const uint32 kMaxLineSize = 4096;
 
-bool ParseSmapsHeader(std::istream* smaps,
-                      ProcessMemoryMaps::VMRegion* region) {
+bool ParseSmapsHeader(char* header_line, ProcessMemoryMaps::VMRegion* region) {

[Primiano Tucci (use gerrit), 2015/10/06 16:37:31]

+const -> const char*

[ssid, 2015/10/06 18:47:05]

Done.

   // e.g., "00400000-00421000 r-xp 00000000 fc:01 1234  /foo.so\n"
   bool res = true;  // Whether this region should be appended or skipped.
-  uint64 end_addr;
-  std::string protection_flags;
-  std::string ignored;
-  *smaps >> std::hex >> region->start_address;
-  smaps->ignore(1);
-  *smaps >> std::hex >> end_addr;
+  uint64 end_addr = 0;
+  char protection_flags[5] = {0};
+  char mapped_file[kMaxLineSize];
+
+  if (sscanf(header_line, "%" SCNx64 "-%" SCNx64 " %4c %*s %*s %*s%4095[^\n]\n",
+             &region->start_address, &end_addr, protection_flags,
+             mapped_file) != 4)
+    return false;
+
   if (end_addr > region->start_address) {
     region->size_in_bytes = end_addr - region->start_address;
   } else {
@@ -42,8 +45,6 @@ bool ParseSmapsHeader(std::istream* smaps,
   }
 
   region->protection_flags = 0;
-  *smaps >> protection_flags;
-  CHECK_EQ(4UL, protection_flags.size());
   if (protection_flags[0] == 'r') {
     region->protection_flags |=
         ProcessMemoryMaps::VMRegion::kProtectionFlagsRead;
@@ -56,84 +57,70 @@ bool ParseSmapsHeader(std::istream* smaps,
     region->protection_flags |=
         ProcessMemoryMaps::VMRegion::kProtectionFlagsExec;
   }
-  *smaps >> ignored;  // Ignore mapped file offset.
-  *smaps >> ignored;  // Ignore device maj-min (fc:01 in the example above).
-  *smaps >> ignored;  // Ignore inode number (1234 in the example above).
 
-  while (smaps->peek() == ' ')
-    smaps->ignore(1);
-  char mapped_file[kMaxLineSize];
-  smaps->getline(mapped_file, sizeof(mapped_file));
   region->mapped_file = mapped_file;
+  TrimWhitespaceASCII(region->mapped_file, TRIM_ALL, &region->mapped_file);
 
   return res;
 }
 
-uint64 ReadCounterBytes(std::istream* smaps) {
+uint64 ReadCounterBytes(char* counter_line) {
   uint64 counter_value = 0;
-  *smaps >> std::dec >> counter_value;
+  int res = sscanf(counter_line, "%*s %" SCNu64 " kB", &counter_value);
+  CHECK_EQ(1, res);

[Primiano Tucci (use gerrit), 2015/10/06 16:37:31]

You can probably relax this to be a DCHECK, no need to hard crash if you can't
parse something here.

[ssid, 2015/10/06 18:47:04]

Done.

   return counter_value * 1024;
 }
 
-uint32 ParseSmapsCounter(std::istream* smaps,
+uint32 ParseSmapsCounter(char* counter_line,
                          ProcessMemoryMaps::VMRegion* region) {
   // A smaps counter lines looks as follows: "RSS:  0 Kb\n"
   uint32 res = 1;
-  std::string counter_name;
-  *smaps >> counter_name;
-
-  // TODO(primiano): "Swap" should also be accounted as resident. Check
-  // whether Rss isn't already counting swapped and fix below if that is
-  // the case.
-  if (counter_name == "Pss:") {
-    region->byte_stats_proportional_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Private_Dirty:") {
-    region->byte_stats_private_dirty_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Private_Clean:") {
-    region->byte_stats_private_clean_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Shared_Dirty:") {
-    region->byte_stats_shared_dirty_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Shared_Clean:") {
-    region->byte_stats_shared_clean_resident = ReadCounterBytes(smaps);
-  } else if (counter_name == "Swap:") {
-    region->byte_stats_swapped = ReadCounterBytes(smaps);
+  const uint32 kMaxCounterNameSize = 20;

[Primiano Tucci (use gerrit), 2015/10/06 16:37:31]

Actually doesn't seem you use this anywhere else, just put the 20 below

[ssid, 2015/10/06 18:47:05]

Done.

+  char counter_name[kMaxCounterNameSize];
+  int did_read = sscanf(counter_line, "%s", counter_name);

[Primiano Tucci (use gerrit), 2015/10/06 16:37:31]

replace %s with %19[^\n]s, so this won't overflow if you get  a name larger than
20 chars

[ssid, 2015/10/06 18:47:04]

Done.

+  DCHECK_EQ(1, did_read);
+
+  if (strcmp(counter_name, "Pss:") == 0) {
+    region->byte_stats_proportional_resident = ReadCounterBytes(counter_line);
+  } else if (strcmp(counter_name, "Private_Dirty:") == 0) {
+    region->byte_stats_private_dirty_resident = ReadCounterBytes(counter_line);
+  } else if (strcmp(counter_name, "Private_Clean:") == 0) {
+    region->byte_stats_private_clean_resident = ReadCounterBytes(counter_line);
+  } else if (strcmp(counter_name, "Shared_Dirty:") == 0) {
+    region->byte_stats_shared_dirty_resident = ReadCounterBytes(counter_line);
+  } else if (strcmp(counter_name, "Shared_Clean:") == 0) {
+    region->byte_stats_shared_clean_resident = ReadCounterBytes(counter_line);
+  } else if (strcmp(counter_name, "Swap:") == 0) {
+    region->byte_stats_swapped = ReadCounterBytes(counter_line);
   } else {
     res = 0;
   }
 
-#ifndef NDEBUG
-  // Paranoid check against changes of the Kernel /proc interface.
-  if (res) {
-    std::string unit;
-    *smaps >> unit;
-    DCHECK_EQ("kB", unit);
-  }
-#endif
-
-  smaps->ignore(kMaxLineSize, '\n');
-
   return res;
 }
 
-uint32 ReadLinuxProcSmapsFile(std::istream* smaps, ProcessMemoryMaps* pmm) {
-  if (!smaps->good())
+uint32 ReadLinuxProcSmapsFile(FILE* smaps_file, ProcessMemoryMaps* pmm) {
+  if (!smaps_file)
     return 0;
 
+  fseek(smaps_file, 0, SEEK_SET);
+
+  char line[kMaxLineSize];
   const uint32 kNumExpectedCountersPerRegion = 6;
   uint32 counters_parsed_for_current_region = 0;
   uint32 num_valid_regions = 0;
   ProcessMemoryMaps::VMRegion region;
   bool should_add_current_region = false;
   for (;;) {
-    int next = smaps->peek();
-    if (next == std::ifstream::traits_type::eof() || next == '\n')
+    if (fgets(line, kMaxLineSize, smaps_file) == nullptr)

[Primiano Tucci (use gerrit), 2015/10/06 16:37:31]

maybe set line[0] = '\0' before the fgets so if you happen to read a blank line
the if below won't do random things in production code

[ssid, 2015/10/06 18:47:04]

Done.

       break;
-    if (isxdigit(next) && !isupper(next)) {
+    DCHECK_GT(strlen(line), 0u);
+    if (isxdigit(line[0]) && !isupper(line[0])) {
       region = ProcessMemoryMaps::VMRegion();
       counters_parsed_for_current_region = 0;
-      should_add_current_region = ParseSmapsHeader(smaps, &region);
+      should_add_current_region = ParseSmapsHeader(line, &region);
     } else {
-      counters_parsed_for_current_region += ParseSmapsCounter(smaps, &region);
+      counters_parsed_for_current_region += ParseSmapsCounter(line, &region);
       DCHECK_LE(counters_parsed_for_current_region,
                 kNumExpectedCountersPerRegion);
       if (counters_parsed_for_current_region == kNumExpectedCountersPerRegion) {
@@ -177,8 +164,8 @@ bool ProcessMemoryMapsDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
   if (UNLIKELY(proc_smaps_for_testing)) {
     res = ReadLinuxProcSmapsFile(proc_smaps_for_testing, pmd->process_mmaps());
   } else {
-    std::ifstream proc_self_smaps("/proc/self/smaps");
-    res = ReadLinuxProcSmapsFile(&proc_self_smaps, pmd->process_mmaps());
+    ScopedFILE smaps_file(fopen("/proc/self/smaps", "r"));
+    res = ReadLinuxProcSmapsFile(smaps_file.get(), pmd->process_mmaps());
   }
 #else
   LOG(ERROR) << "ProcessMemoryMaps dump provider is supported only on Linux";