[sanitizer_symbolizer] Add initial symbolizer markup #72605
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
for linux Adds initial support of symbolizer markup for linux. This change adds a runtime sanitizer common flag `enable_symbolizer_markup` that controls the instance of the StackTracePrinter to be used, besides that, it also controls the usage of the implementation of SymbolizerTool that produces symbolizer markup `MarkupSymbolizerTool`. Comming in future PRs: - There is some repetition of code between the implementation of the fuchsia symbolizer markup and the more generic symbolizer markup. Making these changes in a different PR will guarantee it is an NFC. - The RenderContext code uses the Symbolizer list of modules to render the list of modules, according to the symbolizer markup spec. This dependency between the Renderer and Symbolizer can be removed by using a ListOfModules directly and hooking into the right interceptors. Also defering it to make sure it is an NFC.
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Andres Villegas (avillega) Changesfor linux Adds initial support of symbolizer markup for linux. This change adds a runtime sanitizer common flag Comming in future PRs:
Patch is 33.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/72605.diff 15 Files Affected:
diff --git a/compiler-rt/lib/sanitizer_common/CMakeLists.txt b/compiler-rt/lib/sanitizer_common/CMakeLists.txt
index 61e832a30eb3767f..99883bd1d54d050f 100644
--- a/compiler-rt/lib/sanitizer_common/CMakeLists.txt
+++ b/compiler-rt/lib/sanitizer_common/CMakeLists.txt
@@ -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
@@ -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
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
index 607ecae6808b72af..255bf263208373b9 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
@@ -6357,7 +6357,7 @@ INTERCEPTOR(int, dlclose, void *handle) {
COMMON_INTERCEPT_FUNCTION(dlclose);
#else
#define INIT_DLOPEN_DLCLOSE
-#endif
+#endif // SANITIZER_INTERCEPT_DLOPEN_DLCLOSE
#if SANITIZER_INTERCEPT_GETPASS
INTERCEPTOR(char *, getpass, const char *prompt) {
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc b/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
index 949bdbd148b6b894..490b7d7ecdc4b75a 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
@@ -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, false,
+ "Use sanitizer symbolizer markup, available on Linux "
+ "and always set true for fuchsia.")
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
index 3e1b078a0212f5e2..d5afde7d627800eb 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
@@ -415,9 +415,9 @@
// Enable offline markup symbolizer for Fuchsia.
#if SANITIZER_FUCHSIA
-# define SANITIZER_SYMBOLIZER_MARKUP 1
+# define SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA 1
#else
-# define SANITIZER_SYMBOLIZER_MARKUP 0
+# define SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA 0
#endif
// Enable ability to support sanitizer initialization that is
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
index 8db321051e1a0de0..7a3e4827a7e0eeb8 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
@@ -12,28 +12,15 @@
#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)
@@ -59,8 +46,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_SYMBOLIZER_MARKUP_FUCHSIA
+
+StackTracePrinter *StackTracePrinter::GetOrInit() {
+ 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)
@@ -322,9 +328,9 @@ void FormattedStackTracePrinter::RenderData(InternalScopedString *buffer,
}
}
-#endif // !SANITIZER_SYMBOLIZER_MARKUP
+#endif // !SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA
-void FormattedStackTracePrinter::RenderSourceLocation(
+void StackTracePrinter::RenderSourceLocation(
InternalScopedString *buffer, const char *file, int line, int column,
bool vs_style, const char *strip_path_prefix) {
if (vs_style && line > 0) {
@@ -343,7 +349,7 @@ void FormattedStackTracePrinter::RenderSourceLocation(
}
}
-void FormattedStackTracePrinter::RenderModuleLocation(
+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));
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h
index 3e02333a8c8d5cc2..a3d2307e87b656ce 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h
@@ -12,6 +12,7 @@
#ifndef SANITIZER_STACKTRACE_PRINTER_H
#define SANITIZER_STACKTRACE_PRINTER_H
+#include "sanitizer_platform.h"
#include "sanitizer_common.h"
#include "sanitizer_internal_defs.h"
#include "sanitizer_symbolizer.h"
@@ -25,7 +26,18 @@ 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);
+
+ void RenderSourceLocation(InternalScopedString *buffer,
+ const char *file, 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,
@@ -34,15 +46,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;
@@ -51,11 +54,14 @@ 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_SYMBOLIZER_MARKUP_FUCHSIA
+
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
@@ -90,14 +96,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:
@@ -110,6 +108,8 @@ class FormattedStackTracePrinter : public StackTracePrinter {
~FormattedStackTracePrinter() {}
};
+#endif // !SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA
+
} // namespace __sanitizer
#endif // SANITIZER_STACKTRACE_PRINTER_H
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer.h b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer.h
index 7fb7928dce0d8d4c..40636e9722c461d1 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer.h
@@ -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
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
index 81141023386ea01e..b32ca05eadc38654 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
@@ -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_SYMBOLIZER_MARKUP_FUCHSIA
const char *ExtractToken(const char *str, const char *delims, char **result) {
uptr prefix_len = internal_strcspn(str, delims);
@@ -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++) {
@@ -566,6 +574,6 @@ bool SymbolizerProcess::WriteToSymbolizer(const char *buffer, uptr length) {
return true;
}
-#endif // !SANITIZER_SYMBOLIZER_MARKUP
+#endif // !SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA
} // namespace __sanitizer
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
index 6402cfd7c36e443e..87217595375622e9 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
@@ -8,103 +8,166 @@
//
// This file is shared between various sanitizers' runtime libraries.
//
-// Implementation of offline markup symbolizer.
+// This generic support for offline symbolizing is based on the
+// Fuchsia port. We don't do any actual symbolization per se.
+// Instead, we emit text containing raw addresses and raw linkage
+// symbol names, embedded in Fuchsia's symbolization markup format.
+// See the spec at: https://llvm.org/docs/SymbolizerMarkupFormat.html
//===----------------------------------------------------------------------===//
-#include "sanitizer_platform.h"
-
-#if SANITIZER_SYMBOLIZER_MARKUP
+#include "sanitizer_symbolizer_markup.h"
-# include "sanitizer_common.h"
-# include "sanitizer_stacktrace_printer.h"
-# include "sanitizer_symbolizer.h"
-# include "sanitizer_symbolizer_markup_constants.h"
+#include "sanitizer_common.h"
+#include "sanitizer_libc.h"
+#include "sanitizer_platform.h"
+#include "sanitizer_symbolizer.h"
+#include "sanitizer_symbolizer_markup_constants.h"
namespace __sanitizer {
-// This generic support for offline symbolizing is based on the
-// Fuchsia port. We don't do any actual symbolization per se.
-// Instead, we emit text containing raw addresses and raw linkage
-// symbol names, embedded in Fuchsia's symbolization markup format.
-// Fuchsia's logging infrastructure emits enough information about
-// process memory layout that a post-processing filter can do the
-// symbolization and pretty-print the markup. See the spec at:
-// https://fuchsia.googlesource.com/zircon/+/master/docs/symbolizer_markup.md
-
-// This is used by UBSan for type names, and by ASan for global variable names.
-// It's expected to return a static buffer that will be reused on each call.
-const char *Symbolizer::Demangle(const char *name) {
- static char buffer[kFormatDemangleMax];
- internal_snprintf(buffer, sizeof(buffer), kFormatDemangle, name);
- return buffer;
+void MarkupStackTracePrinter::RenderData(InternalScopedString *buffer,
+ const char *format, const DataInfo *DI,
+ const char *strip_path_prefix) {
+ RenderContext(buffer);
+ buffer->AppendF(kFormatData, DI->start);
}
-// This is used mostly for suppression matching. Making it work
-// would enable "interceptor_via_lib" suppressions. It's also used
-// once in UBSan to say "in module ..." in a message that also
-// includes an address in the module, so post-processing can already
-// pretty-print that so as to indicate the module.
-bool Symbolizer::GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
- uptr *module_address) {
+bool MarkupStackTracePrinter::RenderNeedsSymbolization(const char *format) {
return false;
}
-// This is mainly used by hwasan for online symbolization. This isn't needed
-// since hwasan can always just dump stack frames for offline symbolization.
-bool Symbolizer::SymbolizeFrame(uptr addr, FrameInfo *info) { return false; }
-
-// This is used in some places for suppression checking, which we
-// don't really support for Fuchsia. It's also used in UBSan to
-// identify a PC location to a function name, so we always fill in
-// the function member with a string containing markup around the PC
-// value.
-// TODO(mcgrathr): Under SANITIZER_GO, it's currently used by TSan
-// to render stack frames, but that should be changed to use
-// RenderStackFrame.
-SymbolizedStack *Symbolizer::SymbolizePC(uptr addr) {
- SymbolizedStack *s = SymbolizedStack::New(addr);
+void MarkupStackTracePrinter::RenderFrame(InternalScopedString *buffer,
+ const char *format, int frame_no,
+ uptr address, const AddressInfo *info,
+ bool vs_style,
+ const char *strip_path_prefix) {
+ CHECK(!RenderNeedsSymbolization(format));
+ RenderContext(buffer);
+ buffer->AppendF(kFormatFrame, frame_no, address);
+}
+
+bool MarkupSymbolizerTool::SymbolizePC(uptr addr, SymbolizedStack *stack) {
char buffer[kFormatFunctionMax];
internal_snprintf(buffer, sizeof(buffer), kFormatFunction, addr);
- s->info.function = internal_strdup(buffer);
- return s;
+ stack->info.function = internal_strdup(buffer);
+ return true;
}
-// Always claim we succeeded, so that RenderDataInfo will be called.
-bool Symbolizer::SymbolizeData(uptr addr, DataInfo *info) {
+bool MarkupSymbolizerTool::SymbolizeData(uptr addr, DataInfo *info) {
info->Clear();
info->start = addr;
return true;
}
-// We ignore the format argument to __sanitizer_symbolize_global.
-void FormattedStackTracePrinter::RenderData(InternalScopedString *buffer,
- const char *format,
- const DataInfo *DI,
- const char *strip_path_prefix) {
- buffer->AppendF(kFormatData, DI->start);
+// This is used by UBSan for type names, and by ASan for global variable names.
+// It's expected to return a static buffer that will be reused on each call.
+const char *MarkupSymbolizerTool::Demangle(const char *name) {
+ static char buffer[kFormatDemangleMax];
+ internal_snprintf(buffer, sizeof(buffer), kFormatDemangle, name);
+ return buffer;
}
-bool FormattedStackTracePrinter::RenderNeedsSymbolization(const char *format) {
- return false;
+// Fuchsia's implementation of symbolizer markup doesn't need to emit contextual
+// elements at this point.
+// Fuchsia's logging infrastructure emits enough information about
+// process memory layout that a post-processing filter can do the
+// symbolization and pretty-print the markup.
+#if !SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA
+
+// Simplier view of a LoadedModule. It only holds information necessary to
+// identify unique modules.
+struct RenderedModule {
+ char *full_name;
+ uptr base_address;
+ u8 uuid[kModuleUUIDSize]; // BuildId
+};
+
+static bool ModulesEq(const LoadedModule &module,
+ const RenderedModule &renderedModule) {
+ return module.base_address() == renderedModule.base_address &&
+ internal_memcmp(module.uuid(), renderedModule.uuid,
+ module.uuid_size()) == 0 &&
+ internal_strcmp(module.full_name(), renderedModule.full_name) == 0;
}
-// We don't support the stack_trace_format flag at all.
-void FormattedStackTracePrinter::RenderFrame(InternalScopedString *buffer,
- const char *format, int frame_no,
- uptr address,
- const AddressInfo *info,
- bool vs_style,
- const char *strip_path_prefix) {
- CHECK(!RenderNeedsSymbolization(format));
- buffer->AppendF(kFormatFrame, frame_no, address);
+static bool ModuleHasBeenRendered(
+ const LoadedModule &module,
+ const InternalMmapVectorNoCtor<RenderedModule> &renderedModules) {
+ for (const auto &renderedModule : renderedModules) {
+ if (ModulesEq(module, renderedModule)) {
+ return true;
+ }
+ }
+ return false;
}
-Symbolizer *Symbolizer::PlatformInit() {
- return new (symbolizer_allocator_) Symbolizer({});
+static void RenderModule(InternalScopedString *buffer,
+ const LoadedModule &module, uptr moduleId) {
+ buffer->AppendF("{{{module:%d:%s:elf:", moduleId, module.full_name());
+ for (uptr i = 0; i < module.uuid_size(); i++) {
+ buffer->AppendF("%02x", module.uuid()[i]);
+ }
+ buffer->Append("}}}\n");
}
-void Symbolizer::LateInitialize() { Symbolizer::GetOrInit(); }
+static void RenderMmaps(InternalScopedString *buffer,
+ const LoadedModule &module, uptr moduleId) {
+ for (const auto &range : module.ranges()) {
+ //{{{mmap:starting_addr:size_in_hex:load:module_Id:r(w|x):relative_addr}}}
+ buffer->Ap...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
PiJoules
reviewed
Nov 17, 2023
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
Outdated
Show resolved
Hide resolved
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
Outdated
Show resolved
Hide resolved
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
Outdated
Show resolved
Hide resolved
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
Outdated
Show resolved
Hide resolved
| @@ -415,9 +415,9 @@ | |||
|
|
|||
| // Enable offline markup symbolizer for Fuchsia. | |||
| #if SANITIZER_FUCHSIA | |||
| # define SANITIZER_SYMBOLIZER_MARKUP 1 | |||
| # define SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA 1 | |||
There was a problem hiding this comment.
Rather than renaming this, could this just remain SANITIZER_SYMBOLIZER_MARKUP and anything that's fuchsia-specific below check for SANITIZER_FUCHSIA instead?
There was a problem hiding this comment.
This is always guarding fuchsia specific code or code we don't want to compile for fuchsia, hence the rename. I could get rid of this definition and just use SANITIZER_FUCHSIA if that is more clear.
| // sanitizer_symbolizer_markup_fuchsia.cpp implements these differently. | ||
| #if !SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA | ||
|
|
||
| StackTracePrinter *StackTracePrinter::GetOrInit() { |
There was a problem hiding this comment.
Does wrapping this with SANITIZER_SYMBOLIZER_MARKUP_FUCHSIA mean it won't be defined if we're not using symbolizer markup?
There was a problem hiding this comment.
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
Comment on lines
+332
to
+335
| void StackTracePrinter::RenderSourceLocation(InternalScopedString *buffer, | ||
| const char *file, int line, | ||
| int column, bool vs_style, | ||
| const char *strip_path_prefix) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
Does the = 0 need to be removed? Same with RenderSourceLocation and RenderModuleLocation.
There was a problem hiding this comment.
These are not viritual functions anymore, the implementation of this functions are shared between all the implementation of StackTracePrinter abstract class.
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
Outdated
Show resolved
Hide resolved
|
I've fixed most of the comments that I think are actionable. Please take a look. |
vitalybuka
reviewed
Nov 20, 2023
There was a problem hiding this comment.
Seems like a lot of interdependent changes
There is a proposal to use https://github.com/getcord/spr for stacked review
It tried, it works for stacked review.
Would you like to try?
There was a problem hiding this comment.
That's not good reason to enabled only aarch64. Unless you know why this does not work for other, it should not be constrained.
| @@ -6357,7 +6357,7 @@ INTERCEPTOR(int, dlclose, void *handle) { | |||
| COMMON_INTERCEPT_FUNCTION(dlclose); | |||
| #else | |||
| #define INIT_DLOPEN_DLCLOSE | |||
| #endif | |||
| #endif // SANITIZER_INTERCEPT_DLOPEN_DLCLOSE | |||
There was a problem hiding this comment.
Please don't mix random fix-ups with actual patch
| // Strip interceptor prefixes from function name. | ||
| const char *StripFunctionName(const char *function); | ||
|
|
||
| void RenderSourceLocation(InternalScopedString *buffer, const char *file, |
There was a problem hiding this comment.
Please don't move code around in separate NFC patches
| // 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 |
There was a problem hiding this comment.
If it runs, I'd prefer we have it everywhere.
There was a problem hiding this comment.
Even SANITIZER_SYMBOLIZER_MARKUP seems nicer.
|
I will try to create a chain of stacked prs for this change, per Vitaly's recommendation. |
|
Will close in favor of a stack of PR that has the same changes, the stack starts at #73192 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
for linux
Adds initial support of symbolizer markup for linux. This change adds a runtime sanitizer common flag
enable_symbolizer_markupthat controls the instance of the StackTracePrinter to be used, besides that, it also controls the usage of the implementation of SymbolizerTool that produces symbolizer markupMarkupSymbolizerTool.Comming in future PRs: