This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer_common] Add StackTrace::PrintTo method
ClosedPublic

Authored by gbalats on May 19 2021, 4:26 PM.

Details

Summary

This method is like StackTrace::Print but instead of printing to stderr
it copies its output to a user-provided buffer.

Part of https://reviews.llvm.org/D102451.

Diff Detail

Event Timeline

gbalats created this revision.May 19 2021, 4:26 PM
gbalats requested review of this revision.May 19 2021, 4:26 PM
Herald added a subscriber: Restricted Project. · View Herald TranscriptMay 19 2021, 4:26 PM
This revision is now accepted and ready to land.May 19 2021, 4:33 PM
vitalybuka added 1 blocking reviewer(s): vitalybuka.May 19 2021, 5:05 PM

In general LGTM, let me take a look closer.

This revision now requires review to proceed.May 19 2021, 5:05 PM
vitalybuka added inline comments.May 19 2021, 10:04 PM
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h
64

It does not say that it's truncated on \n

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_libcdep.cpp
21

I don't believe one additional string copy is important here. This code is not expected to
be called frequently. and we have possible symbolizer call and two mmaps in InternalScopedString vectors.

So this can be simplified as this:

//===-- sanitizer_stacktrace_libcdep.cpp ----------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// This file is shared between AddressSanitizer and ThreadSanitizer
// run-time libraries.
//===----------------------------------------------------------------------===//

#include "sanitizer_common.h"
#include "sanitizer_libc.h"
#include "sanitizer_placement_new.h"
#include "sanitizer_stacktrace.h"
#include "sanitizer_stacktrace_printer.h"
#include "sanitizer_symbolizer.h"

namespace __sanitizer {

namespace {

class StackTraceTextPrinter {
 public:
  StackTraceTextPrinter(const char *stack_trace_fmt, char frame_delimeter,
                        InternalScopedString *output,
                        InternalScopedString *dedup_token)
      : stack_trace_fmt_(stack_trace_fmt),
        frame_delimeter_(frame_delimeter),
        output_(output),
        dedup_token_(dedup_token) {}

  bool ProcessAddressFrames(uptr pc) {
    bool symbolize = RenderNeedsSymbolization(stack_trace_fmt_);
    SymbolizedStack *frames = symbolize
                                  ? Symbolizer::GetOrInit()->SymbolizePC(pc)
                                  : SymbolizedStack::New(pc);
    if (!frames)
      return false;

    for (SymbolizedStack *cur = frames; cur; cur = cur->next) {
      uptr prev_len = output_->length();
      RenderFrame(output_, stack_trace_fmt_, frame_num_++, cur->info.address,
                  symbolize ? &cur->info : nullptr,
                  common_flags()->symbolize_vs_style,
                  common_flags()->strip_path_prefix);

      if (prev_len != output_->length())
        output_->append("%c", frame_delimeter_);

      if (!dedup_token_)
        continue;
      if (dedup_frames_-- > 0) {
        if (dedup_token_->length())
          dedup_token_->append("--");
        if (cur->info.function != nullptr)
          dedup_token_->append(cur->info.function);
      }
    }
    frames->ClearAll();
    return true;
  }

 private:
  const char *stack_trace_fmt_;
  char frame_delimeter_;
  int dedup_frames_ = common_flags()->dedup_token_length;
  uptr frame_num_ = 0;
  InternalScopedString *output_;
  InternalScopedString *dedup_token_;
};

static void CopyString(const InternalScopedString &output, char *out_buf,
                       uptr out_buf_size) {
  if (!out_buf_size)
    return;
  uptr copy_size = Min(output.length(), out_buf_size - 1);
  internal_memcpy(out_buf, output.data(), copy_size);
  out_buf[copy_size] = '\0';
}

}  // namespace

void StackTrace::PrintTo(InternalScopedString *output) const {
  CHECK(output);

  InternalScopedString dedup_token;
  StackTraceTextPrinter printer(common_flags()->stack_trace_format, '\n',
                                output, &dedup_token);

  if (trace == nullptr || size == 0) {
    output->append("    <empty stack>\n\n");
    return;
  }

  for (uptr i = 0; i < size && trace[i]; i++) {
    // PCs in stack traces are actually the return addresses, that is,
    // addresses of the next instructions after the call.
    uptr pc = GetPreviousInstructionPc(trace[i]);
    CHECK(printer.ProcessAddressFrames(pc));
  }

  // Always add a trailing empty line after stack trace.
  output->append("\n");

  // Append deduplication token, if non-empty.
  if (dedup_token.length())
    output->append("DEDUP_TOKEN: %s\n", dedup_token.data());
}

uptr StackTrace::PrintTo(char *out_buf, uptr out_buf_size) const {
  InternalScopedString output;
  PrintTo(&output);
  CopyString(output, out_buf, out_buf_size);
  if (output.length() >= out_buf_size) {
    if (char *last_n = internal_strrchr(out_buf, '\n')) {
      // Trunk after the last '\n' for convenience.
      last_n[1] = '\0';
    } else if (out_buf_size > 1) {
      out_buf[out_buf_size - 2] = '\n';
    }
  }
  return output.length();
}

void StackTrace::Print() const {
  InternalScopedString output;
  PrintTo(&output);
  Printf("%s", output.data());
}

void BufferedStackTrace::Unwind(u32 max_depth, uptr pc, uptr bp, void *context,
                                uptr stack_top, uptr stack_bottom,
                                bool request_fast_unwind) {
  // Ensures all call sites get what they requested.
  CHECK_EQ(request_fast_unwind, WillUseFastUnwind(request_fast_unwind));
  top_frame_bp = (max_depth > 0) ? bp : 0;
  // Avoid doing any work for small max_depth.
  if (max_depth == 0) {
    size = 0;
    return;
  }
  if (max_depth == 1) {
    size = 1;
    trace_buffer[0] = pc;
    return;
  }
  if (!WillUseFastUnwind(request_fast_unwind)) {
#if SANITIZER_CAN_SLOW_UNWIND
    if (context)
      UnwindSlow(pc, context, max_depth);
    else
      UnwindSlow(pc, max_depth);
    // If there are too few frames, the program may be built with
    // -fno-asynchronous-unwind-tables. Fall back to fast unwinder below.
    if (size > 2 || size >= max_depth)
      return;
#else
    UNREACHABLE("slow unwind requested but not available");
#endif
  }
  UnwindFast(pc, bp, stack_top, stack_bottom, max_depth);
}

static int GetModuleAndOffsetForPc(uptr pc, char *module_name,
                                   uptr module_name_len, uptr *pc_offset) {
  const char *found_module_name = nullptr;
  bool ok = Symbolizer::GetOrInit()->GetModuleNameAndOffsetForPC(
      pc, &found_module_name, pc_offset);

  if (!ok)
    return false;

  if (module_name && module_name_len) {
    internal_strncpy(module_name, found_module_name, module_name_len);
    module_name[module_name_len - 1] = '\x00';
  }
  return true;
}

}  // namespace __sanitizer
using namespace __sanitizer;

extern "C" {
SANITIZER_INTERFACE_ATTRIBUTE
void __sanitizer_symbolize_pc(uptr pc, const char *fmt, char *out_buf,
                              uptr out_buf_size) {
  if (!out_buf_size)
    return;

  pc = StackTrace::GetPreviousInstructionPc(pc);

  InternalScopedString output;
  StackTraceTextPrinter printer(fmt, '\0', &output, nullptr);
  if (!printer.ProcessAddressFrames(pc)) {
    output.clear();
    output.append("<can't symbolize>");
  }
  CopyString(output, out_buf, out_buf_size);
}

SANITIZER_INTERFACE_ATTRIBUTE
void __sanitizer_symbolize_global(uptr data_addr, const char *fmt,
                                  char *out_buf, uptr out_buf_size) {
  if (!out_buf_size)
    return;
  out_buf[0] = 0;
  DataInfo DI;
  if (!Symbolizer::GetOrInit()->SymbolizeData(data_addr, &DI))
    return;
  InternalScopedString data_desc;
  RenderData(&data_desc, fmt, &DI, common_flags()->strip_path_prefix);
  internal_strncpy(out_buf, data_desc.data(), out_buf_size);
  out_buf[out_buf_size - 1] = 0;
}

SANITIZER_INTERFACE_ATTRIBUTE
int __sanitizer_get_module_and_offset_for_pc(uptr pc, char *module_name,
                                             uptr module_name_len,
                                             uptr *pc_offset) {
  return __sanitizer::GetModuleAndOffsetForPc(pc, module_name, module_name_len,
                                              pc_offset);
}
}  // extern "C"
compiler-rt/lib/sanitizer_common/tests/sanitizer_stacktrace_test.cpp
170–173

This will modify process state and other tests.
if we execute projects/compiler-rt/lib/sanitizer_common/tests/Sanitizer-x86_64-Test in single process mode all subsequent tests will see this override.
As-is there are no other tests which care about exact format, but it's still not nice.

Please save/restore in flags in:
virtual void SetUp();
virtual void TearDown();

207

StackPrintTo prefix looks redundant

225

there is no test which check if we trunk at existing \n, to avoid incomplete frames.

262
EXPECT_DEATH(trace.PrintTo(buf, 0), "");  // This one should not crash. E.g. EXPECT_GT(trace.PrintTo(buf, 0), 0u);
EXPECT_DEATH(trace.PrintTo(NULL, 100), "");  // This should. Maybe (NULL, 0) is also OK
gbalats updated this revision to Diff 346861.May 20 2021, 2:43 PM

Address review comments.

gbalats marked 5 inline comments as done.May 20 2021, 2:58 PM
gbalats added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h
64

Removed this behavior and updated tests. See comment below.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_libcdep.cpp
21

Thanks! This is much nicer. I didn't know how performance critical this code was so I tried to avoid extra computation.

I made a few changes based on this code:

  • some non-functional refactoring changes (e.g., 2nd ctor, const fields, etc)
  • re-introduce symbolize_ field to avoid calling RenderNeedsSymbolization multiple times at each ProcessAddressFrames call
  • I removed the truncation-at-newline behavior since I don't think we actually need it and one can always do it at the call-site by processing the output buffer (whereas if one wants to maximize trace contents into buffer, it will be impossible).
compiler-rt/lib/sanitizer_common/tests/sanitizer_stacktrace_test.cpp
170–173

To clarify, do you think is preferable to

  1. rely on default format (which would make the test break when it is changed) and remove these lines?
  2. loosen test expectation and only count lines?
  3. keep test as is but simply save/restore flags?

I think (1) would make the test a bit more ugly (since there are no function names, which are by default included in the trace) and more prone to breaking.

Also, I wasn't aware that tests can be executed in single process mode. Does this mean that the death test below will make all tests fail, or are they somehow ignored in that mode?

207

Removed.

225

Removed this feature, so no need to add test for it.

262

Placed first expectation on separate non-death test.

gbalats marked 5 inline comments as done.May 20 2021, 2:58 PM
gbalats updated this revision to Diff 346892.May 20 2021, 4:56 PM

Save/restore flags in SetUp/TearDown.

vitalybuka accepted this revision.May 20 2021, 5:41 PM
vitalybuka added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_libcdep.cpp
25

This code is internal, called exactly two times. So I would prefer a single constructor with a little bit heavier argument list at two call sites.

30

Not very strongly but I tried to reduce state and remove it from members. But I am OK if we keep it.

36

This one is expected to be called right after constructor.
I prefer to have corresponding constructor parameter instead of this method.

compiler-rt/lib/sanitizer_common/tests/sanitizer_stacktrace_test.cpp
170–173
  1. LGTM

"single process mode" was wrong set of words.
All these test are part of some GTEST binary. Usually llvm lit runs this binary as multiple parallel processes but each process execute exactly one TEST() using --gtest_filter. In this case none of them will affect each other.

However user can find that binary and directly execute it so all test will be execute in the same process.

And are right about death tests, in any case the are executes with fork() or similar to make sure that parent test does not crash.

225

I didn't mind to have the feature. But either way is fine.

This revision is now accepted and ready to land.May 20 2021, 5:41 PM
gbalats updated this revision to Diff 346901.May 20 2021, 6:21 PM
gbalats marked 2 inline comments as done.

Change to single constructor.

gbalats marked 2 inline comments as done.May 20 2021, 7:06 PM
gbalats added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_libcdep.cpp
36

The reason I had it this way is that it would be easy for one to accidentally swap the dedup token and the output parameter since they have the same type.

gbalats updated this revision to Diff 346908.May 20 2021, 7:37 PM

Fix string.h include.

This revision was landed with ongoing or failed builds.May 20 2021, 7:39 PM
This revision was automatically updated to reflect the committed changes.