Skip to content
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

[sanitizer_symbolizer] Add initial symbolizer markup support for linux. #73193

Conversation

avillega
Copy link
Contributor

This is part of a stack of PRs to add support for symbolizer
markup in linux.

You can check the symbolizer markup specification at:
https://llvm.org/docs/SymbolizerMarkupFormat.html

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 23, 2023

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

Author: Andres Villegas (avillega)

Changes

This is part of a stack of PRs to add support for symbolizer
markup in linux.

You can check the symbolizer markup specification at:
https://llvm.org/docs/SymbolizerMarkupFormat.html


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

5 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_flags.inc (+4)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp (+4)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp (+19)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.h (+23)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp (+7)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc b/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
index 949bdbd148b6b89..7d0c7c4b63c9162 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, SANITIZER_FUCHSIA,
+            "Use sanitizer symbolizer markup, available on Linux "
+            "and always set true for fuchsia.")
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
index 88f186b9c20c105..748d832ccc211d0 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp
@@ -16,6 +16,7 @@
 #include "sanitizer_file.h"
 #include "sanitizer_flags.h"
 #include "sanitizer_fuchsia.h"
+#include "sanitizer_symbolizer_markup.h"
 
 namespace __sanitizer {
 
@@ -62,6 +63,9 @@ const char *StackTracePrinter::StripFunctionName(const char *function) {
 #if !SANITIZER_SYMBOLIZER_MARKUP
 
 StackTracePrinter *StackTracePrinter::NewStackTracePrinter() {
+  if (common_flags()->enable_symbolizer_markup)
+    return new (GetGlobalLowLevelAllocator()) MarkupStackTracePrinter();
+
   return new (GetGlobalLowLevelAllocator()) FormattedStackTracePrinter();
 }
 
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
index c364e1e300225b9..1627908185f2009 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp
@@ -45,4 +45,23 @@ void MarkupStackTracePrinter::RenderFrame(InternalScopedString *buffer,
   buffer->AppendF(kFormatFrame, frame_no, address);
 }
 
+bool MarkupSymbolizerTool::SymbolizePC(uptr addr, SymbolizedStack *stack) {
+  char buffer[kFormatFunctionMax];
+  internal_snprintf(buffer, sizeof(buffer), kFormatFunction, addr);
+  stack->info.function = internal_strdup(buffer);
+  return true;
+}
+
+bool MarkupSymbolizerTool::SymbolizeData(uptr addr, DataInfo *info) {
+  info->Clear();
+  info->start = addr;
+  return true;
+}
+
+const char *MarkupSymbolizerTool::Demangle(const char *name) {
+  static char buffer[kFormatDemangleMax];
+  internal_snprintf(buffer, sizeof(buffer), kFormatDemangle, name);
+  return buffer;
+}
+
 }  // namespace __sanitizer
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.h b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.h
index 7cebe520e9bebb4..07630d0b3bdf417 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.h
@@ -16,6 +16,7 @@
 #include "sanitizer_common.h"
 #include "sanitizer_stacktrace_printer.h"
 #include "sanitizer_symbolizer.h"
+#include "sanitizer_symbolizer_internal.h"
 
 namespace __sanitizer {
 
@@ -40,6 +41,28 @@ class MarkupStackTracePrinter : public StackTracePrinter {
   ~MarkupStackTracePrinter() {}
 };
 
+class MarkupSymbolizerTool final : public SymbolizerTool {
+ public:
+  // 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.
+  bool SymbolizePC(uptr addr, SymbolizedStack *stack) override;
+
+  // Always claim we succeeded, so that RenderDataInfo will be called.
+  bool SymbolizeData(uptr addr, DataInfo *info) override;
+
+  // May return NULL if demangling failed.
+  // 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 *Demangle(const char *name) override;
+};
+
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_SYMBOLIZER_MARKUP_H
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
index d92349c04fffabd..28f11352a6b5bbe 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_platform.h"
+#include "sanitizer_symbolizer_markup.h"
 #if SANITIZER_POSIX
 #  include <dlfcn.h>  // for dlsym()
 #  include <errno.h>
@@ -475,6 +476,12 @@ static void ChooseSymbolizerTools(IntrusiveList<SymbolizerTool> *list,
     VReport(2, "Symbolizer is disabled.\n");
     return;
   }
+  if (common_flags()->enable_symbolizer_markup) {
+    VReport(2, "Using symbolizer markup");
+    SymbolizerTool *tool = new (*allocator) MarkupSymbolizerTool();
+    CHECK(tool);
+    list->push_back(tool);
+  }
   if (IsAllocatorOutOfMemory()) {
     VReport(2, "Cannot use internal symbolizer: out of memory\n");
   } else if (SymbolizerTool *tool = InternalSymbolizer::get(allocator)) {

Copy link
Collaborator

@vitalybuka vitalybuka left a comment

Choose a reason for hiding this comment

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

LGTM. but looks like it can be squashed with sanitizer_common test


COMMON_FLAG(bool, enable_symbolizer_markup, SANITIZER_FUCHSIA,
"Use sanitizer symbolizer markup, available on Linux "
"and always set true for fuchsia.")
Copy link
Member

Choose a reason for hiding this comment

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

Super minor nit, but Fuchsia should be capitalized.

@avillega
Copy link
Contributor Author

LGTM. but looks like it can be squashed with sanitizer_common test

The tests also test Emitting contextual elements, so I will squash the tests with the RenderContextual elements PR.

avillega and others added 4 commits November 28, 2023 00:44
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@avillega avillega changed the base branch from users/avillega/main.sanitizer_symbolizer-add-initial-symbolizer-markup-support-for-linux to main November 28, 2023 01:35
@avillega avillega merged commit 8b3944c into main Nov 28, 2023
5 checks passed
@avillega avillega deleted the users/avillega/sanitizer_symbolizer-add-initial-symbolizer-markup-support-for-linux branch November 28, 2023 01:36
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.

None yet

5 participants