Skip to content

[sanitizer_symbolizer] Add MarkupStackTracePrinter #73032

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.

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

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

avillega
Copy link

This PR is part of a stack, #73029 should be merged
and reviewed first. Please only review the last commit
of this PR.

Adds a new Implementation of StackTracePrinter that only
emits symbolizer markup. Currently this change only
affects Fuchsia OS. Should be NFC.

Andrés Villegas added 2 commits November 21, 2023 19:54
in StackTracePrinter.

Make some methods of StackTracPrinter that will have a common
implementation non virtual.
This PR is part of a stack, llvm#73029 should be merged
and reviewed first. Please only review the last commit
of this PR.

Adds a new Implementation of StackTracePrinter that only
emits symbolizer markup. Currently this change only
affects Fuchsia OS. Should be NFC.
@llvmbot
Copy link
Member

llvmbot commented Nov 21, 2023

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Andres Villegas (avillega)

Changes

This PR is part of a stack, #73029 should be merged
and reviewed first. Please only review the last commit
of this PR.

Adds a new Implementation of StackTracePrinter that only
emits symbolizer markup. Currently this change only
affects Fuchsia OS. Should be NFC.


Full diff: https://github.com/llvm/llvm-project/pull/73032.diff

3 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp (+15-10)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h (+12-20)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp (+23-21)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
index 8db321051e1a0de..e0c3943e4a9743e 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
@@ -12,6 +12,7 @@
 
 #include "sanitizer_stacktrace_printer.h"
 
+#include "sanitizer_common.h"
 #include "sanitizer_file.h"
 #include "sanitizer_flags.h"
 #include "sanitizer_fuchsia.h"
@@ -25,15 +26,13 @@ StackTracePrinter *StackTracePrinter::GetOrInit() {
   if (stacktrace_printer)
     return stacktrace_printer;
 
-  stacktrace_printer =
-      new (GetGlobalLowLevelAllocator()) FormattedStackTracePrinter();
+  stacktrace_printer = StackTracePrinter::PlatformInit();
 
   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)
@@ -62,6 +61,10 @@ const char *FormattedStackTracePrinter::StripFunctionName(
 // sanitizer_symbolizer_markup.cpp implements these differently.
 #if !SANITIZER_SYMBOLIZER_MARKUP
 
+StackTracePrinter *StackTracePrinter::PlatformInit() {
+  return new (GetGlobalLowLevelAllocator()) FormattedStackTracePrinter();
+}
+
 static const char *DemangleFunctionName(const char *function) {
   if (!common_flags()->demangle)
     return function;
@@ -324,9 +327,10 @@ void FormattedStackTracePrinter::RenderData(InternalScopedString *buffer,
 
 #endif  // !SANITIZER_SYMBOLIZER_MARKUP
 
-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) {
   if (vs_style && line > 0) {
     buffer->AppendF("%s(%d", StripPathPrefix(file, strip_path_prefix), line);
     if (column > 0)
@@ -343,9 +347,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));
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h
index 3e02333a8c8d5cc..9de09bda0062d45 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.h
@@ -25,7 +25,8 @@ 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);
 
   virtual void RenderFrame(InternalScopedString *buffer, const char *format,
                            int frame_no, uptr address, const AddressInfo *info,
@@ -34,28 +35,27 @@ 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;
+  void RenderSourceLocation(InternalScopedString *buffer, const char *file,
+                            int line, int column, bool vs_style,
+                            const char *strip_path_prefix);
 
-  virtual void RenderModuleLocation(InternalScopedString *buffer,
-                                    const char *module, uptr offset,
-                                    ModuleArch arch,
-                                    const char *strip_path_prefix) = 0;
+  void RenderModuleLocation(InternalScopedString *buffer, const char *module,
+                            uptr offset, ModuleArch arch,
+                            const char *strip_path_prefix);
   virtual void RenderData(InternalScopedString *buffer, const char *format,
                           const DataInfo *DI,
                           const char *strip_path_prefix = "") = 0;
 
+ private:
+  // To be called from StackTracePrinter::GetOrInit
+  static StackTracePrinter *PlatformInit();
+
  protected:
   ~StackTracePrinter() {}
 };
 
 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 +90,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:
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
index 6402cfd7c36e443..ef3866a4bd32ac8 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
@@ -76,27 +76,29 @@ bool Symbolizer::SymbolizeData(uptr addr, DataInfo *info) {
   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);
-}
-
-bool FormattedStackTracePrinter::RenderNeedsSymbolization(const char *format) {
-  return false;
-}
-
-// 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);
+class MarkupStackTracePrinter : public StackTracePrinter {
+  // We ignore the format argument to __sanitizer_symbolize_global.
+  void RenderData(InternalScopedString *buffer, const char *format,
+                  const DataInfo *DI, const char *strip_path_prefix) override {
+    buffer->AppendF(kFormatData, DI->start);
+  }
+
+  bool RenderNeedsSymbolization(const char *format) override { return false; }
+
+  // We don't support the stack_trace_format flag at all.
+  void RenderFrame(InternalScopedString *buffer, const char *format,
+                   int frame_no, uptr address, const AddressInfo *info,
+                   bool vs_style, const char *strip_path_prefix) override {
+    CHECK(!RenderNeedsSymbolization(format));
+    buffer->AppendF(kFormatFrame, frame_no, address);
+  }
+
+ protected:
+  ~MarkupStackTracePrinter();
+};
+
+StackTracePrinter *StackTracePrinter::PlatformInit() {
+  return new (GetGlobalLowLevelAllocator()) MarkupStackTracePrinter();
 }
 
 Symbolizer *Symbolizer::PlatformInit() {

@avillega avillega merged commit cc21287 into llvm:main Nov 21, 2023
@avillega avillega deleted the add-symbolizer-markup-2 branch November 21, 2023 21:49
aheejin added a commit to aheejin/emscripten that referenced this pull request Apr 7, 2024
Many lines that call `InternalScopedString::AppendF`, including the
below,
https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp#L31
are format-checked by
https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/compiler-rt/lib/sanitizer_common/sanitizer_common.h#L650
which checks if the formats of arguments conform to that of functions
like `printf`. But this errors out because the format strings currently
do not match their arguments.

llvm/llvm-project@9c8f888#diff-5bf0494d1b61b99e55aefb25e59e41b678f20a392c2c6d77e5fbdc97c2ca4c3f
started the effort to check `__attribute__((format))` in the codebase,
but apparently there were too many violating instances, it made the code
build with `-Wno-format` (presumably as an temporary solution):
https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/compiler-rt/lib/sanitizer_common/CMakeLists.txt#L223-L224
So this adds `-Wno-format` to our cflags as well.

The reason it was fine without it until now is, we didn't build the
whole file thanks to this line:
https://github.com/emscripten-core/emscripten/blob/a9b347bfcabda59a5edff60ee18b8a0ab70aa9dc/system/lib/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp#L15

But recent refactoring efforts including
llvm/llvm-project#73032,
llvm/llvm-project#73192, and
llvm/llvm-project#73193 moved many parts out of
that file to be built by a specific platform (e.g. fushia) and made the
file build unconditionally. I tried to add back `#if
SANITIZER_SYMBOLIZER_MARKUP` to that file but then this line
(https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp#L67)
breaks with an undefined referenc error. So just build the library with
`-Wno-format`, which is also what the upstream is effectively doing,
seems a simpler solution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants