[PATCH] D12153: Support inline functions symbolization in Addr2Line symbolizer.

[Yury Gribov via llvm-commits]

ygribov added inline comments.

================
Comment at: lib/asan/scripts/asan_symbolize.py:138
@@ -137,2 +137,3 @@
     self.pipe = self.open_addr2line()
+    self.output_terminator = "0xffffffffffffffff"
 
----------------
Perhaps make short version for 32-bit targets? Same for C++ code below.

================
Comment at: lib/asan/scripts/asan_symbolize.py:163
@@ +162,3 @@
+      frame = 0
+      while True:
+        function_name = self.pipe.stdout.readline().rstrip()
----------------
Perhaps add a guard here to avoid endless loop?

================
Comment at: lib/asan/scripts/asan_symbolize.py:167
@@ +166,3 @@
+        if function_name.startswith('??'):
+          break
+        lines.append((function_name, file_name));
----------------
Shouldn't you eat following ??:? as well? Also what if symbolization fails for the first (non-terminator) address? I guess you'll skip response for terminator and thus break following symbolizations.

================
Comment at: lib/asan/scripts/asan_symbolize.py:171
@@ -166,1 +170,3 @@
+      lines.append(('??', '??:?'))
+    return ['%s in %s %s' % (addr, frame[0], fix_filename(frame[1])) for frame in lines]
 
----------------
This would be more pythonic:
  % (addr, line, fix_filename(file)) for (line, file) in lines

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:198
@@ -197,10 +197,3 @@
   bool ReachedEndOfOutput(const char *buffer, uptr length) const override {
-    // Output should consist of two lines.
-    int num_lines = 0;
-    for (uptr i = 0; i < length; ++i) {
-      if (buffer[i] == '\n')
-        num_lines++;
-      if (num_lines >= 2)
-        return true;
-    }
-    return false;
+    const int terminator_size = sizeof(output_terminator_) - 1;
+    // Skip, if we red just the OUTPUT_TERMINATOR_, because we should also read
----------------
Please use size_t. Also it makes more sense to call this terminator_len.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:202
@@ +201,3 @@
+    if (length == terminator_size &&
+        !internal_memcmp(buffer, output_terminator_, terminator_size))
+      return false;
----------------
Hm, shouldn't this always be false if length == terminator_size?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:206
@@ -207,1 +205,3 @@
+    return !internal_memcmp(buffer + length - terminator_size,
+                            output_terminator_, terminator_size);
   }
----------------
The two internal_memcmps are duplicating each other. Can you compute the second one and cache result in a var with descriptive name (say is_terminated)?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:219
@@ -218,2 +218,3 @@
   const char *module_name_;  // Owned, leaked.
+  static const char output_terminator_[9];
 };
----------------
You don't need 9, neither here, nor in definition.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:232
@@ -228,3 +231,3 @@
   bool SymbolizePC(uptr addr, SymbolizedStack *stack) override {
     if (const char *buf =
             SendCommand(stack->info.module, stack->info.module_offset)) {
----------------
Shouldn't this be non-const?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:263
@@ -257,2 +262,3 @@
     char buffer_[kBufferSize];
-    internal_snprintf(buffer_, kBufferSize, "0x%zx\n", module_offset);
+    internal_snprintf(buffer_, kBufferSize, "0x%zx\n0xffffffffffffffff\n",
+                      module_offset);
----------------
-1 should be a define somewhere. Also splitting termination logic across Addr2LinePool and Addr2LineProcess looks really weird. Can we move it all to one class?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:272
@@ -265,2 +271,3 @@
   InternalMmapVector<Addr2LineProcess*> addr2line_pool_;
+  static const char output_terminator_[9];
 };
----------------
I'm sure we can avoid code dup.


http://reviews.llvm.org/D12153