Skip to content

[sanitizer_symbolizer] Add initial symbolizer markup #72605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

[sanitizer_symbolizer] Add initial symbolizer markup #72605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c5b1fab
[sanitizer_symbolizer] Add initial symbolizer markup
Nov 16, 2023
f8ba073
Apply git clang-format
Nov 17, 2023
2df7f3d
Fix review comments
Nov 20, 2023
1368aef
Review comment
Nov 20, 2023
b4a567a
Simplify test and add requires
Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion compiler-rt/lib/sanitizer_common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ set(SANITIZER_SYMBOLIZER_SOURCES
sanitizer_symbolizer_libcdep.cpp
sanitizer_symbolizer_mac.cpp
sanitizer_symbolizer_markup.cpp
sanitizer_symbolizer_markup_fuchsia.cpp
sanitizer_symbolizer_posix_libcdep.cpp
sanitizer_symbolizer_report.cpp
sanitizer_symbolizer_report_fuchsia.cpp
Expand Down Expand Up @@ -191,10 +192,11 @@ set(SANITIZER_IMPL_HEADERS
sanitizer_stoptheworld.h
sanitizer_suppressions.h
sanitizer_symbolizer.h
sanitizer_symbolizer_markup_constants.h
sanitizer_symbolizer_internal.h
sanitizer_symbolizer_libbacktrace.h
sanitizer_symbolizer_mac.h
sanitizer_symbolizer_markup.h
sanitizer_symbolizer_markup_constants.h
sanitizer_syscall_generic.inc
sanitizer_syscall_linux_aarch64.inc
sanitizer_syscall_linux_arm.inc
Expand Down
2 changes: 1 addition & 1 deletion compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6357,7 +6357,7 @@ INTERCEPTOR(int, dlclose, void *handle) {
COMMON_INTERCEPT_FUNCTION(dlclose);
#else
#define INIT_DLOPEN_DLCLOSE
#endif
#endif // SANITIZER_INTERCEPT_DLOPEN_DLCLOSE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't mix random fix-ups with actual patch


#if SANITIZER_INTERCEPT_GETPASS
INTERCEPTOR(char *, getpass, const char *prompt) {
Expand Down
4 changes: 4 additions & 0 deletions compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,3 +275,7 @@ COMMON_FLAG(bool, test_only_emulate_no_memorymap, false,
// program.
COMMON_FLAG(bool, test_only_replace_dlopen_main_program, false,
"TEST ONLY replace dlopen(<main program>,...) with dlopen(NULL)")

COMMON_FLAG(bool, enable_symbolizer_markup, SANITIZER_FUCHSIA,
"Use sanitizer symbolizer markup, available on Linux "
"and always set true for fuchsia.")
7 changes: 0 additions & 7 deletions compiler-rt/lib/sanitizer_common/sanitizer_platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,13 +413,6 @@
# define SANITIZER_CACHE_LINE_SIZE 64
#endif

// Enable offline markup symbolizer for Fuchsia.
#if SANITIZER_FUCHSIA
# define SANITIZER_SYMBOLIZER_MARKUP 1
#else
# define SANITIZER_SYMBOLIZER_MARKUP 0
#endif

// Enable ability to support sanitizer initialization that is
// compatible with the sanitizer library being loaded via
// `dlopen()`.
Expand Down
59 changes: 33 additions & 26 deletions compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,28 +12,14 @@

#include "sanitizer_stacktrace_printer.h"

#include "sanitizer_common.h"
#include "sanitizer_file.h"
#include "sanitizer_flags.h"
#include "sanitizer_fuchsia.h"
#include "sanitizer_symbolizer_markup.h"

namespace __sanitizer {

StackTracePrinter *StackTracePrinter::GetOrInit() {
static StackTracePrinter *stacktrace_printer;
static StaticSpinMutex init_mu;
SpinMutexLock l(&init_mu);
if (stacktrace_printer)
return stacktrace_printer;

stacktrace_printer =
new (GetGlobalLowLevelAllocator()) FormattedStackTracePrinter();

CHECK(stacktrace_printer);
return stacktrace_printer;
}

const char *FormattedStackTracePrinter::StripFunctionName(
const char *function) {
const char *StackTracePrinter::StripFunctionName(const char *function) {
if (!common_flags()->demangle)
return function;
if (!function)
Expand All @@ -59,8 +45,27 @@ const char *FormattedStackTracePrinter::StripFunctionName(
return function;
}

// sanitizer_symbolizer_markup.cpp implements these differently.
#if !SANITIZER_SYMBOLIZER_MARKUP
// sanitizer_symbolizer_markup_fuchsia.cpp implements these differently.
#if !SANITIZER_FUCHSIA

StackTracePrinter *StackTracePrinter::GetOrInit() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does wrapping this with SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA mean it won't be defined if we're not using symbolizer markup?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means it won't be used if we are compiling for fuchsia, this one was part of the motivation on renaming this definition. StackTracePrinter::GetOrInit is implemented a bit different when compiling for fuchsia compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup_fuchsia.cpp:26

static StackTracePrinter *stacktrace_printer;
static StaticSpinMutex init_mu;
SpinMutexLock l(&init_mu);
if (stacktrace_printer)
return stacktrace_printer;

if (common_flags()->enable_symbolizer_markup) {
stacktrace_printer =
new (GetGlobalLowLevelAllocator()) MarkupStackTracePrinter();
} else {
stacktrace_printer =
new (GetGlobalLowLevelAllocator()) FormattedStackTracePrinter();
}

CHECK(stacktrace_printer);
return stacktrace_printer;
}

static const char *DemangleFunctionName(const char *function) {
if (!common_flags()->demangle)
Expand Down Expand Up @@ -322,11 +327,12 @@ void FormattedStackTracePrinter::RenderData(InternalScopedString *buffer,
}
}

#endif // !SANITIZER_SYMBOLIZER_MARKUP
#endif // !SANITIZER_FUCHSIA

void FormattedStackTracePrinter::RenderSourceLocation(
InternalScopedString *buffer, const char *file, int line, int column,
bool vs_style, const char *strip_path_prefix) {
void StackTracePrinter::RenderSourceLocation(InternalScopedString *buffer,
const char *file, int line,
int column, bool vs_style,
const char *strip_path_prefix) {
Comment on lines +332 to +335
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this and RenderModuleLocation don't seem to be changed, it might be simpler/easier to read this patch if these changes were in a separate NFC patch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is that this and RenderModuleLocation have the same implementation for the FormattedStackTracePrinter and the MarkupStackTracePrinter before introducing the Markup specific code, this change made no much sense.

if (vs_style && line > 0) {
buffer->AppendF("%s(%d", StripPathPrefix(file, strip_path_prefix), line);
if (column > 0)
Expand All @@ -343,9 +349,10 @@ void FormattedStackTracePrinter::RenderSourceLocation(
}
}

void FormattedStackTracePrinter::RenderModuleLocation(
InternalScopedString *buffer, const char *module, uptr offset,
ModuleArch arch, const char *strip_path_prefix) {
void StackTracePrinter::RenderModuleLocation(InternalScopedString *buffer,
const char *module, uptr offset,
ModuleArch arch,
const char *strip_path_prefix) {
buffer->AppendF("(%s", StripPathPrefix(module, strip_path_prefix));
if (arch != kModuleArchUnknown) {
buffer->AppendF(":%s", ModuleArchToString(arch));
Expand Down
39 changes: 18 additions & 21 deletions compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#include "sanitizer_common.h"
#include "sanitizer_internal_defs.h"
#include "sanitizer_platform.h"
#include "sanitizer_symbolizer.h"

namespace __sanitizer {
Expand All @@ -25,7 +26,16 @@ class StackTracePrinter {
public:
static StackTracePrinter *GetOrInit();

virtual const char *StripFunctionName(const char *function) = 0;
// Strip interceptor prefixes from function name.
const char *StripFunctionName(const char *function);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the = 0 need to be removed? Same with RenderSourceLocation and RenderModuleLocation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not viritual functions anymore, the implementation of this functions are shared between all the implementation of StackTracePrinter abstract class.


void RenderSourceLocation(InternalScopedString *buffer, const char *file,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't move code around in separate NFC patches

int line, int column, bool vs_style,
const char *strip_path_prefix);

void RenderModuleLocation(InternalScopedString *buffer, const char *module,
uptr offset, ModuleArch arch,
const char *strip_path_prefix);

virtual void RenderFrame(InternalScopedString *buffer, const char *format,
int frame_no, uptr address, const AddressInfo *info,
Expand All @@ -34,15 +44,6 @@ class StackTracePrinter {

virtual bool RenderNeedsSymbolization(const char *format) = 0;

virtual void RenderSourceLocation(InternalScopedString *buffer,
const char *file, int line, int column,
bool vs_style,
const char *strip_path_prefix) = 0;

virtual void RenderModuleLocation(InternalScopedString *buffer,
const char *module, uptr offset,
ModuleArch arch,
const char *strip_path_prefix) = 0;
virtual void RenderData(InternalScopedString *buffer, const char *format,
const DataInfo *DI,
const char *strip_path_prefix = "") = 0;
Expand All @@ -51,11 +52,13 @@ class StackTracePrinter {
~StackTracePrinter() {}
};

// See sanitizer_symbolizer_markup.h for the markup implementation of
// StackTracePrinter. This is code is omited for targets that opt in to
// use SymbolizerMarkup only.
#if !SANITIZER_FUCHSIA
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it runs, I'd prefer we have it everywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even SANITIZER_SYMBOLIZER_MARKUP seems nicer.


class FormattedStackTracePrinter : public StackTracePrinter {
public:
// Strip interceptor prefixes from function name.
const char *StripFunctionName(const char *function) override;

// Render the contents of "info" structure, which represents the contents of
// stack frame "frame_no" and appends it to the "buffer". "format" is a
// string with placeholders, which is copied to the output with
Expand Down Expand Up @@ -90,14 +93,6 @@ class FormattedStackTracePrinter : public StackTracePrinter {

bool RenderNeedsSymbolization(const char *format) override;

void RenderSourceLocation(InternalScopedString *buffer, const char *file,
int line, int column, bool vs_style,
const char *strip_path_prefix) override;

void RenderModuleLocation(InternalScopedString *buffer, const char *module,
uptr offset, ModuleArch arch,
const char *strip_path_prefix) override;

// Same as RenderFrame, but for data section (global variables).
// Accepts %s, %l from above.
// Also accepts:
Expand All @@ -110,6 +105,8 @@ class FormattedStackTracePrinter : public StackTracePrinter {
~FormattedStackTracePrinter() {}
};

#endif // !SANITIZER_FUCHSIA

} // namespace __sanitizer

#endif // SANITIZER_STACKTRACE_PRINTER_H
2 changes: 2 additions & 0 deletions compiler-rt/lib/sanitizer_common/sanitizer_symbolizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ class Symbolizer final {

void InvalidateModuleList();

ListOfModules &GetRefreshedListOfModules();

private:
// GetModuleNameAndOffsetForPC has to return a string to the caller.
// Since the corresponding module might get unloaded later, we should create
Expand Down
14 changes: 11 additions & 3 deletions compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ Symbolizer *Symbolizer::GetOrInit() {
return symbolizer_;
}

// See sanitizer_symbolizer_markup.cpp.
#if !SANITIZER_SYMBOLIZER_MARKUP
// See sanitizer_symbolizer_markup_fuchsia.cpp.
#if !SANITIZER_FUCHSIA

const char *ExtractToken(const char *str, const char *delims, char **result) {
uptr prefix_len = internal_strcspn(str, delims);
Expand Down Expand Up @@ -191,6 +191,14 @@ void Symbolizer::RefreshModules() {
modules_fresh_ = true;
}

ListOfModules &Symbolizer::GetRefreshedListOfModules() {
if (!modules_fresh_)
RefreshModules();

CHECK(modules_fresh_);
return modules_;
}

static const LoadedModule *SearchForModule(const ListOfModules &modules,
uptr address) {
for (uptr i = 0; i < modules.size(); i++) {
Expand Down Expand Up @@ -566,6 +574,6 @@ bool SymbolizerProcess::WriteToSymbolizer(const char *buffer, uptr length) {
return true;
}

#endif // !SANITIZER_SYMBOLIZER_MARKUP
#endif // !SANITIZER_FUCHSIA

} // namespace __sanitizer
Loading