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

160–170

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

164

Why can't this be while-true loop?

171–172

Should this be a list of pairs

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

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

176

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

lib/sanitizer_common/sanitizer_symbolizer_internal.h

78

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

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

163

Perhaps add a guard here to avoid endless loop?

167

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.

173

This would be more pythonic:

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

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

196–197

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

200

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

204

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

226

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

253

Shouldn't this be non-const?

280–281

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

291

I'm sure we can avoid code dup.

[ygribov]

lib/asan/scripts/asan_symbolize.py

138

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

[m.ostapenko]

lib/asan/scripts/asan_symbolize.py

163

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

226

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

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

173

s/line/function

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

197

s/red/read

200

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

202

Should this be <= ?

207

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.

226

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

200

Ah, sorry, these are GCC habits.

207

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

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

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

Why const uptr?

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

197

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

280–284

Note that terminator logic is still split across two classes.

291

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

Sorry, size_t is undefined here.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

197

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.

291

AFAIK, they do:

class members should end with _

Or maybe I missed something?

[samsonov]

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

197

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

197

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

197

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

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

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–173

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–173

Please address

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

280–281

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

You shouldn't really do anything if ReadFromSymbolizer fails.

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

216

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

218

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

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.
     import pty
     import termios
     pid, fd = pty.fork()

lib/sanitizer_common/sanitizer_symbolizer_internal.h

 // external symbolizer program, running in a different subprocess.
 // SymbolizerProcess may not be used from two threads simultaneously.
 class SymbolizerProcess {
  public:
   explicit SymbolizerProcess(const char *path, bool use_forkpty = false);
   const char *SendCommand(const char *command);
 
  protected:
   virtual bool ReachedEndOfOutput(const char *buffer, uptr length) const {
     UNIMPLEMENTED();
   }
 
   /// The maximum number of arguments required to invoke a tool process.
   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;

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 {
     if (const char *buf =
             SendCommand(stack->info.module, stack->info.module_offset)) {
       ParseSymbolizePCOutput(buf, stack);
       return true;
     }
     return false;
   }
 
   bool SymbolizeData(uptr addr, DataInfo *info) override {
for (uptr i = 0; i < addr2line_pool_.size(); ++i) {
         break;
       }
     }
     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