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

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?

170–171

Should this be a list of pairs

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

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

175

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.

172

This would be more pythonic:

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

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

212

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

216

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

220

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

229

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

244

Shouldn't this be non-const?

273–274

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

282

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

229

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

172

s/line/function

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

199

s/red/read

202

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

204

Should this be <= ?

209

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

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

Ah, sorry, these are GCC habits.

209

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

199

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

273–275

Note that terminator logic is still split across two classes.

282

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

199

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.

282

AFAIK, they do:

class members should end with _

Or maybe I missed something?

[samsonov]

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

199

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

199

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

199

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

244

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

244

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

170–172

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

170–172

Please address

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

273–274

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

227

You shouldn't really do anything if ReadFromSymbolizer fails.

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

234

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

236

"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.startswith('??') and file_name.startswith('??: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(('??', '??:?'))
+    return ['%s in %s %s' % (addr, line, fix_filename(file)) for (line, 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 void TrimOutputBuffer(char *buffer, const uptr read_len) {
+    buffer[read_len] = '\0';
+  }
+
  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_;

lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc

while (true) {
     if (!success || just_read == 0) {
       Report("WARNING: Can't read from symbolizer at fd %d\n", input_fd_);
       return false;
     }
     read_len += just_read;
     if (ReachedEndOfOutput(buffer, read_len))
       break;
   }
-  buffer[read_len] = '\0';
+  TrimOutputBuffer(buffer, read_len);
   return true;
 }
 
 bool SymbolizerProcess::WriteToSymbolizer(const char *buffer, uptr length) {
   if (length == 0)
     return true;
   uptr write_len = 0;
   bool success = WriteToFile(output_fd_, buffer, length, &write_len);
   if (!success || write_len != length) {
     Report("WARNING: Can't write to symbolizer at fd %d\n", output_fd_);
     return false;
   }
   return true;
 }
 
 }  // namespace __sanitizer

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

  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;
+    const size_t terminator_len = sizeof(output_terminator_) - 1;
+    // Skip, if we red just TERMINATOR_LEN 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 == terminator_len) return false;
+    // Addr2Line output should end up with OUTPUT_TERMINATOR_.
+    bool is_terminated = !internal_memcmp(buffer + length - terminator_len,
+                                          output_terminator_, terminator_len);
+    if (is_terminated) {
+      effective_length = length - terminator_len;
     }
     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;
   }
 
+  void TrimOutputBuffer(char *buffer, const uptr read_len) override {
+    buffer[effective_length] = '\0';
+  }
+
   const char *module_name_;  // Owned, leaked.
+  static size_t effective_length;
+  static const char output_terminator_[9];
 };
 
+const char Addr2LineProcess::output_terminator_[] = "??\n??:0\n";
+size_t Addr2LineProcess::effective_length;
+
 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 {
const char *SendCommand(const char *module_name, uptr module_offset) {
     }
     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);
+    internal_snprintf(buffer_, kBufferSize, "0x%zx\n%zx\n",
+                      module_offset, output_terminator_);
     return addr2line->SendCommand(buffer_);
   }
 
   static const uptr kBufferSize = 32;
   const char *addr2line_path_;
   LowLevelAllocator *allocator_;
   InternalMmapVector<Addr2LineProcess*> addr2line_pool_;
+  static const uptr output_terminator_ =
+      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