[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:167
@@ +166,3 @@
+          is_first_frame = False
+        elif function_name.startswith('??') and file_name.startswith('??:0'):
+          break
----------------
I'd rather assert than file_name is correct so that we fail fast on parse error. Also why startswith instead of =?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_internal.h:78
@@ -77,3 +77,3 @@
  protected:
-  virtual bool ReachedEndOfOutput(const char *buffer, uptr length) const {
+  virtual bool ReachedEndOfOutput(const char *buffer, uptr length) {
     UNIMPLEMENTED();
----------------
Rather than cache effective length in ReachedEndOfOutput and introduce implicit constraint on methods, can we recalculate effective_length inside TrimOutputBuffer? This would also allow us to keep const.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_internal.h:91
@@ -90,1 +90,3 @@
 
+  virtual void TrimOutputBuffer(char *buffer, const uptr read_len) {
+    buffer[read_len] = '\0';
----------------
Why const uptr?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:198
@@ +197,3 @@
+  bool ReachedEndOfOutput(const char *buffer, uptr length) override {
+    const size_t kTerminatorLen = 8; // sizeof(output_terminator_)
+    // Skip, if we read just kTerminatorLen bytes, because Addr2Line output
----------------
I wonder if there's a way to avoid magic const.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:272-273
@@ -257,3 +271,4 @@
     char buffer_[kBufferSize];
-    internal_snprintf(buffer_, kBufferSize, "0x%zx\n", module_offset);
+    internal_snprintf(buffer_, kBufferSize, "0x%zx\n%zx\n",
+                      module_offset, output_terminator_);
     return addr2line->SendCommand(buffer_);
----------------
Note that terminator logic is still split across two classes.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:281
@@ -265,1 +280,3 @@
   InternalMmapVector<Addr2LineProcess*> addr2line_pool_;
+  static const uptr output_terminator_ =
+      FIRST_32_SECOND_64(UINT32_MAX, UINT64_MAX);
----------------
I don't think that output_terminator but rather "dummy_address" or something like this. BTW note that statics don't take underscore.


http://reviews.llvm.org/D12153