Support inline functions symbolization in Addr2Line symbolizer.

[m.ostapenko]

Summary

Right now, Addr2Line symbolizer in asan_symbolize.py doesn't support inline functions symbolization. This might be a useful feature for using ASan on embedded systems.

Test results:

$ cat test.c

#include <sanitizer/common_interface_defs.h>

static inline void FooBarBaz() {

__sanitizer_print_stack_trace();

}

int main() {

FooBarBaz();
return 0;

}

$ clang test.c -fsanitize=address -g -O2 -o test.x && ./test.x &> /tmp/test.log
$ ./projects/compiler-rt/lib/asan/scripts/asan_symbolize.py -l /tmp/test.log

#0 0x42095e in __sanitizer_print_stack_trace _asan_rtl_
#1 0x4cec07 in FooBarBaz /home/max/build/llvm/asan/test.c:4
#2 0x4cec07 in main /home/max/build/llvm/asan/test.c:8
#3 0x7f89f0891ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287

[samsonov]

It's kind of sad you have to launch a subprocess for every line in a stack trace. Is there no way you could make "addr2line -i" read addresses from stdin, and understand how many frames in output describe every input address?

[ygribov]

Is there no way you could make "addr2line -i" read addresses from stdin,
and understand how many frames in output describe every input address?

Unfortunately addr2line is too primitive for this and there is no way to know number of frames in advance. Perhaps we could make inlining optional?

[samsonov]

Crazy suggestion: whenever you need to symbolize a PC, feed this PC followed by 0xbadbadbad to addr2line. It will print just two lines (??, ??:0) for the latter, which means all the rest describe the symbolized location of PC (possibly, inlined).

[ygribov]

Hm, this should work. Max?

[ygribov]

We probably also need to update Addr2LineProcess and LLVMSymbolizer so that output for offline/online symbolizer is consistent.

[m.ostapenko]

Crazy suggestion: whenever you need to symbolize a PC, feed this PC followed by 0xbadbadbad to addr2line. It will print just two lines (??, ??:0) for the latter, which means all the rest describe the symbolized location of PC (possibly, inlined).

Hm, this should work. Max?

Yes, this works fine in script.

We probably also need to update Addr2LineProcess and LLVMSymbolizer so that output for offline/online symbolizer is consistent.

We don't touch LLVMSymbolizer in asan_symbolize.py, so no need to modify it in library.

[m.ostapenko]

Updating following approach, suggested by Alexey. No regressions on x86_64-unknown-linux-gnu.

$ ASAN_OPTIONS=allow_addr2line=1 ./test.x 
  #0 0x41f7de in __sanitizer_print_stack_trace /home/max/src/llvm/projects/compiler-rt/lib/asan/asan_stack.cc:38
  #1 0x4cec17 in FooBarBaz /tmp/test.c:4
  #2 0x4cec17 in main /tmp/test.c:8
  #3 0x7fe58102cec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
  #4 0x417ac5 in _start (/tmp/test.x+0x417ac5)

[samsonov]

lib/asan/scripts/asan_symbolize.py

159 ↗

(On Diff #32830)

Make this a named constant. I also wonder if different value would work better (smth. that will definitely be outside of .text section).

163 ↗

(On Diff #32830)

Why can't this be while-true loop?

171 ↗

(On Diff #32830)

Should this be a list of pairs

[('??', '??:?')]

? Or just declare lines before try-catch block and call .append here

176 ↗

(On Diff #32830)

return ['%s in %s %s' % (addr, frame[0], fix_filename(frame[1])) for frame in lines]

lib/sanitizer_common/sanitizer_symbolizer_internal.h

78 ↗

(On Diff #32830)

The semantics of this function is very confusing. If GetEffectiveLenOfOutput is 0, it means that we should continue to fetch the output, which makes little sense.

Also, this is incorrect. Suppose you have passed

0x400400
0xbadbadbad

to addr2line, it had printed the following to stdout:

foo
/tmp/foo.cc:42
??
??:?

Suppose the first call to ReadFromFile in ReadFromSymbolizer has fecthed:

foo
/tmp/foo.cc:42
??

In your implementation, GetEffectiveLenOfOutput would return non-zero, you will break from the loop and fail to consume the last line.

You should fix it at the higher level - consume the output of addr2line as you do now, but change the way you handle this buffer: either teach ParseSymbolizePCOutput to drop the last couple of lines, or (better) trim the buffer in Addr2LineSymbolizer before calling that function.

[m.ostapenko]

Updating the patch according to last review. Don't break existing interfaces now.

[ygribov]

lib/asan/scripts/asan_symbolize.py

138 ↗

(On Diff #32970)

Perhaps make short version for 32-bit targets? Same for C++ code below.

163 ↗

(On Diff #32970)

Perhaps add a guard here to avoid endless loop?

167 ↗

(On Diff #32970)

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.

171 ↗

(On Diff #32970)

This would be more pythonic:

% (addr, line, fix_filename(file)) for (line, file) in lines

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

198 ↗

(On Diff #32970)

Please use size_t. Also it makes more sense to call this terminator_len.

202 ↗

(On Diff #32970)

Hm, shouldn't this always be false if length == terminator_size?

206 ↗

(On Diff #32970)

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)?

219 ↗

(On Diff #32970)

You don't need 9, neither here, nor in definition.

232 ↗

(On Diff #32970)

Shouldn't this be non-const?

263 ↗

(On Diff #32970)

-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?

272 ↗

(On Diff #32970)

I'm sure we can avoid code dup.

[ygribov]

lib/asan/scripts/asan_symbolize.py

138 ↗

(On Diff #32970)

Actually -1 seems to work for both Binutils and Elfutils.

[m.ostapenko]

lib/asan/scripts/asan_symbolize.py

163 ↗

(On Diff #32970)

I did this in my previous revision, but Alexey wrote this in previous comment:

Why can't this be while-true loop?

So, I'm a bit confused, what approach is preferable here.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

219 ↗

(On Diff #32970)

Hm, without 9 I got this:

/home/max/src/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:198:41: error: invalid application of 'sizeof' to an incomplete type 'const char []'
    const size_t terminator_len = sizeof(output_terminator_) - 1;
                                        ^~~~~~~~~~~~~~~~~~~~
1 error generated.

[ygribov]

lib/asan/scripts/asan_symbolize.py

163 ↗

(On Diff #32970)

Yeah, I'm a bit worried about corner-cases causing our primitive parser to hang but that may be an overkill.

[m.ostapenko]

Updating according to last Yura's nits.

[samsonov]

lib/asan/scripts/asan_symbolize.py

172 ↗

(On Diff #33082)

s/line/function

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

199 ↗

(On Diff #33082)

s/red/read

202 ↗

(On Diff #33082)

Please fix variable names in the comment - there are no OUTPUT_TERMINATOR_ and TERMINATOR_LEN

204 ↗

(On Diff #33082)

Should this be <= ?

209 ↗

(On Diff #33082)

class members should end with _

Use of static class variable here is really weird - it adds implicit restriction that you will *always* call ReachedEndOfOutput, followed by TrimOutputBuffer with the same buffer, and that there will be no several instances of this class. I think the latter is false even now.

229 ↗

(On Diff #33082)

I don't like hardcoded 9 either. You can just move this constant into the body of ReachedEndOfOutput

static const char kOutputTerminator[] = "??\n??:0\n";

[m.ostapenko]

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

202 ↗

(On Diff #33082)

Ah, sorry, these are GCC habits.

209 ↗

(On Diff #33082)

Ok, I can accumulate effective_length_ in object's member, but I'll need to remove "const" keyword from ReachedEndOfOutput method. Is this OK?

[m.ostapenko]

Updating according to last review. Since effective_length_ is an object's member now, ReachedEndOfOutput is no longer const.

[ygribov]

lib/asan/scripts/asan_symbolize.py

167 ↗

(On Diff #33196)

I'd rather assert than file_name is correct so that we fail fast on parse error. Also why startswith instead of =?

lib/sanitizer_common/sanitizer_symbolizer_internal.h

78 ↗

(On Diff #33196)

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.

91 ↗

(On Diff #33196)

Why const uptr?

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

198 ↗

(On Diff #33196)

I wonder if there's a way to avoid magic const.

272–273 ↗

(On Diff #33196)

Note that terminator logic is still split across two classes.

281 ↗

(On Diff #33196)

I don't think that output_terminator but rather "dummy_address" or something like this. BTW note that statics don't take underscore.

[m.ostapenko]

lib/sanitizer_common/sanitizer_symbolizer_internal.h

91 ↗

(On Diff #33196)

Sorry, size_t is undefined here.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

198 ↗

(On Diff #33196)

We can use strlen, but this will lead to recalculation on each functon call. Or maybe use "??\n??:0\n" as it to compute kTerminatorLen.

281 ↗

(On Diff #33196)

AFAIK, they do:

class members should end with _

Or maybe I missed something?

[samsonov]

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

198 ↗

(On Diff #33196)

Consider just making output_terminator_ a function-static - it's not used anywhere else in this class anyway.

[m.ostapenko]

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

198 ↗

(On Diff #33196)

Yes, this is an option, but if follow approach Yura suggested (do not accumulate effective_len, but recompute it in TrimOutputBuffer each time), we'll need output_terminator_ in TrimOutputBuffer either to find meaningful buffer length.

[ygribov]

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

198 ↗

(On Diff #33196)

This seems to work at least GCC 4.9:

struct XXX {
  static constexpr const char s[] = "??\n??:0\n";
  size_t f() {
    return sizeof(s);
  }
};

[m.ostapenko]

Updating according to last review. To completely remove termination logic from Addr2LinePool, I needed to introduce new AdjustCommandIfNeeded in SymbolizerProcess interface. Also, ReachedEndOfOutput is outline now to be able compute sizeof(output_terminator_).

[samsonov]

Sorry if I'm being overly nitpicky, but I don't understand why you've moved this logic away from Addr2LinePool - I actually preferred when the subclass was responsible for providing correct command to SendCommand and parsing its result accordingly...

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

226 ↗

(On Diff #33322)

This is wrong - how could you know if the buffer for command has sufficient size?

[m.ostapenko]

In D12153#234468, @samsonov wrote:

Sorry if I'm being overly nitpicky, but I don't understand why you've moved this logic away from Addr2LinePool - I actually preferred when the subclass was responsible for providing correct command to SendCommand and parsing its result accordingly...

Ok, I'm moving command modifying logic back to Addr2LinePool. Also, should I move TrimOutputBuffer logic to Addr2LinePool as well? I'm sorry, I'm a bit confused about this stuff.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

226 ↗

(On Diff #33322)

Hm, I increased command buffer size from 32 to 64, isn't that enough to hold offset + dummy address?

static const uptr kBufferSize = 64;

[m.ostapenko]

And the patch itself.

[ygribov]

lib/asan/scripts/asan_symbolize.py

171–172 ↗

(On Diff #33409)

Or rather '??:0'?

[m.ostapenko]

Alexey, Yura, could you please comment this?

[samsonov]

I still don't like the suggested approach. Having TrimOutputBuffer in the parent class is confusing - it's unclear when one needs to use it, and it's added for one specific implementation detail of addr2line. To make that more generic, you can make ReadFromSymbolizer virtual, and make Addr2LineProcess::ReadFromSymbolizer just call the implementation of the base class, and afterwards adjust it (search for output_terminator_ and put a terminating '\0').

lib/asan/scripts/asan_symbolize.py

171–172 ↗

(On Diff #33409)

Please address

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

277 ↗

(On Diff #33409)

While you're here, rename it to buffer, as it's a local variable.

[m.ostapenko]

Updating according to last Alexey's review. Does this look better now?

[samsonov]

Yes, IMO it looks better. Only a few minor comments.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

209 ↗

(On Diff #34349)

You shouldn't really do anything if ReadFromSymbolizer fails.

if (!ReadFromSymbolizer())
  return false;
//...
return true;

216 ↗

(On Diff #34349)

You probably don't need a dedicated function for this now.

218 ↗

(On Diff #34349)

"in case given offset is invalid"

[m.ostapenko]

Addressing last Alexey's comments.

[samsonov]

LGTM. Thank you for doing this, and sorry for the long review!

[m.ostapenko]

Great! Alexey, Yura, thanks for your help!

I've rechecked the patch once again on ARM, AARCH64 and X86_64 targets and everything looks good.
Alexey, could you please land this?

compiler-rt/trunk/lib/asan/scripts/asan_symbolize.py

def LLVMSymbolizerFactory(system, default_arch, dsym_hints=[]):
   return LLVMSymbolizer(symbolizer_path, default_arch, system, dsym_hints)
 
 
 class Addr2LineSymbolizer(Symbolizer):
   def __init__(self, binary):
     super(Addr2LineSymbolizer, self).__init__()
     self.binary = binary
     self.pipe = self.open_addr2line()
+    self.output_terminator = -1
 
   def open_addr2line(self):
     addr2line_tool = 'addr2line'
     if binutils_prefix:
       addr2line_tool = binutils_prefix + addr2line_tool
-    cmd = [addr2line_tool, '-f']
+    cmd = [addr2line_tool, '-fi']
     if demangle:
       cmd += ['--demangle']
     cmd += ['-e', self.binary]
     if DEBUG:
       print ' '.join(cmd)
     return subprocess.Popen(cmd,
                             stdin=subprocess.PIPE, stdout=subprocess.PIPE)
 
   def symbolize(self, addr, binary, offset):
     """Overrides Symbolizer.symbolize."""
     if self.binary != binary:
       return None
+    lines = []
     try:
       print >> self.pipe.stdin, offset
+      print >> self.pipe.stdin, self.output_terminator
+      is_first_frame = True
+      while True:
         function_name = self.pipe.stdout.readline().rstrip()
         file_name = self.pipe.stdout.readline().rstrip()
+        if is_first_frame:
+          is_first_frame = False
+        elif function_name == '??':
+          assert file_name == '??:0'
+          break
+        lines.append((function_name, file_name));
     except Exception:
-      function_name = ''
-      file_name = ''
-    file_name = fix_filename(file_name)
-    return ['%s in %s %s' % (addr, function_name, file_name)]
-
+      lines.append(('??', '??:0'))
+    return ['%s in %s %s' % (addr, function, fix_filename(file)) for (function, file) in lines]
 
 class UnbufferedLineConverter(object):
   """
   Wrap a child process that responds to each line of input with one line of
   output.  Uses pty to trick the child into providing unbuffered output.
   """
   def __init__(self, args, close_stderr=False):
     # Local imports so that the script can start on Windows.

compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_internal.h

protected:
   enum { kArgVMax = 6 };
 
   /// Fill in an argv array to invoke the child process.
   virtual void GetArgV(const char *path_to_binary,
                        const char *(&argv)[kArgVMax]) const {
     UNIMPLEMENTED();
   }
 
+  virtual bool ReadFromSymbolizer(char *buffer, uptr max_length);
+
  private:
   bool Restart();
   const char *SendCommandImpl(const char *command);
-  bool ReadFromSymbolizer(char *buffer, uptr max_length);
   bool WriteToSymbolizer(const char *buffer, uptr length);
   bool StartSymbolizerSubprocess();
 
   const char *path_;
   fd_t input_fd_;
   fd_t output_fd_;
 
   static const uptr kBufferSize = 16 * 1024;

compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

 class Addr2LineProcess : public SymbolizerProcess {
  public:
   Addr2LineProcess(const char *path, const char *module_name)
       : SymbolizerProcess(path), module_name_(internal_strdup(module_name)) {}
 
   const char *module_name() const { return module_name_; }
 
  private:
-  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;
-  }
-
   void GetArgV(const char *path_to_binary,
                const char *(&argv)[kArgVMax]) const override {
     int i = 0;
     argv[i++] = path_to_binary;
-    argv[i++] = "-Cfe";
+    argv[i++] = "-iCfe";
     argv[i++] = module_name_;
     argv[i++] = nullptr;
   }
 
+  bool ReachedEndOfOutput(const char *buffer, uptr length) const override;
+
+  bool ReadFromSymbolizer(char *buffer, uptr max_length) override {
+    if (!SymbolizerProcess::ReadFromSymbolizer(buffer, max_length))
+      return false;
+    // We should cut out output_terminator_ at the end of given buffer,
+    // appended by addr2line to mark the end of its meaningful output.
+    // We cannot scan buffer from it's beginning, because it is legal for it
+    // to start with output_terminator_ in case given offset is invalid. So,
+    // scanning from second character.
+    char *garbage = internal_strstr(buffer + 1, output_terminator_);
+    // This should never be NULL since buffer must end up with
+    // output_terminator_.
+    CHECK(garbage);
+    // Trim the buffer.
+    garbage[0] = '\0';
+    return true;
+  }
+
   const char *module_name_;  // Owned, leaked.
+  static const char output_terminator_[];
 };
 
+const char Addr2LineProcess::output_terminator_[] = "??\n??:0\n";
+
+bool Addr2LineProcess::ReachedEndOfOutput(const char *buffer,
+                                          uptr length) const {
+  const size_t kTerminatorLen = sizeof(output_terminator_) - 1;
+  // Skip, if we read just kTerminatorLen bytes, because Addr2Line output
+  // should consist at least of two pairs of lines:
+  // 1. First one, corresponding to given offset to be symbolized
+  // (may be equal to output_terminator_, if offset is not valid).
+  // 2. Second one for output_terminator_, itself to mark the end of output.
+  if (length <= kTerminatorLen) return false;
+  // Addr2Line output should end up with output_terminator_.
+  return !internal_memcmp(buffer + length - kTerminatorLen,
+                          output_terminator_, kTerminatorLen);
+}
+
 class Addr2LinePool : public SymbolizerTool {
  public:
   explicit Addr2LinePool(const char *addr2line_path,
                          LowLevelAllocator *allocator)
       : addr2line_path_(addr2line_path), allocator_(allocator),
         addr2line_pool_(16) {}
 
   bool SymbolizePC(uptr addr, SymbolizedStack *stack) override {
for (uptr i = 0; i < addr2line_pool_.size(); ++i) {
       }
     }
     if (!addr2line) {
       addr2line =
           new(*allocator_) Addr2LineProcess(addr2line_path_, module_name);
       addr2line_pool_.push_back(addr2line);
     }
     CHECK_EQ(0, internal_strcmp(module_name, addr2line->module_name()));
-    char buffer_[kBufferSize];
-    internal_snprintf(buffer_, kBufferSize, "0x%zx\n", module_offset);
-    return addr2line->SendCommand(buffer_);
+    char buffer[kBufferSize];
+    internal_snprintf(buffer, kBufferSize, "0x%zx\n0x%zx\n",
+                      module_offset, dummy_address_);
+    return addr2line->SendCommand(buffer);
   }
 
-  static const uptr kBufferSize = 32;
+  static const uptr kBufferSize = 64;
   const char *addr2line_path_;
   LowLevelAllocator *allocator_;
   InternalMmapVector<Addr2LineProcess*> addr2line_pool_;
+  static const uptr dummy_address_ =
+      FIRST_32_SECOND_64(UINT32_MAX, UINT64_MAX);
 };
 
 #if SANITIZER_SUPPORTS_WEAK_HOOKS
 extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
 bool __sanitizer_symbolize_code(const char *ModuleName, u64 ModuleOffset,
                                 char *Buffer, int MaxLength);
 SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE