[sanitizer_symbolizer] Symbolizer Markup for linux. #65543

avillega · 2023-09-06T22:34:44Z

This change adds support for sanitizer symbolizer markup for linux. For more informationa about symbolzier markup please visit https://llvm.org/docs/SymbolizerMarkupFormat.html

fmayer · 2023-09-07T18:27:35Z

It don't see any positive tests added, you only changed the existing ones to false.

fmayer · 2023-09-07T18:16:51Z

compiler-rt/lib/hwasan/hwasan_report.cpp

+ InternalScopedString modules_res;
+ RenderModules(&modules_res, modules,
+ /*symbolizer_markup=*/ true);
+ Printf("%s", modules_res.data());


Would it be helpful to add an example of how this looks as a comment? Makes it easier to grep for as well.

fmayer · 2023-09-07T18:17:24Z

compiler-rt/lib/hwasan/hwasan_report.cpp

 ? current_stack_allocations
 : t->stack_allocations();
- PrintStackAllocations(sa, addr_tag, untagged_addr);
+ if (common_flags()->enable_symbolizer_markup) {


I'm confused. Why is the PrintStackAllocationsMarkup in the else branch?

fmayer · 2023-09-07T18:20:46Z

compiler-rt/lib/tsan/rtl/tsan_report.cpp

+ if (const ListOfModules *modules =
+ Symbolizer::GetOrInit()->GetRefreshedListOfModules()) {
+ InternalScopedString modules_res;
+ RenderModules(&modules_res, modules,


I would prefer if this and the other caller looked like

if (common_flags()->enable_symbolizer_markup) RenderModulesMarkup(...);

The indirection doesn't add much and only obscures what is happening.

fmayer · 2023-09-07T18:24:24Z

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp

+ }
+ buffer->append("}}}\n");
+
+ const auto ranges = module.ranges();


Why do we need this copy? We don't seem to be touching module in the loop.

fmayer · 2023-09-07T18:26:14Z

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp

+ renderedModules.push_back({});
+ RenderedModule &curModule = renderedModules.back();
+ curModule.full_name = internal_strdup(module.full_name());
+ internal_memcpy(curModule.uuid, module.uuid(), module.uuid_size());


assert that curModule.uuid buffer can hold module.uuid_size?

fmayer · 2023-09-07T19:14:53Z

compiler-rt/lib/hwasan/hwasan_report.cpp

+ break;
+ uptr pc_mask = (1ULL << 48) - 1;
+ uptr pc = record & pc_mask;
+ frame_desc.append(" record_addr:0x%zx record:0x%zx",


Do we want to add a new format to the symbolizer markup to support this data?

vitalybuka

We need tests in test/sanitizer_common with the feature enabled

vitalybuka · 2023-09-08T00:03:20Z

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp

 static const char kDefaultFormat[] = " #%n %p %F %L";

 void RenderFrame(InternalScopedString *buffer, const char *format, int frame_no,
 uptr address, const AddressInfo *info, bool vs_style,


vs_style is ignored?

RenderFrame does use vs_style. Its is not used in symbolizer markup code.

why it then forwarded into RenderFrameMarkup?
if they are mutually exclusive, would it be better replace two bool flags with enum?

vitalybuka · 2023-09-08T00:13:08Z

compiler-rt/lib/hwasan/hwasan_report.cpp

+ if (const ListOfModules *modules =
+ Symbolizer::GetOrInit()->GetRefreshedListOfModules()) {
+ InternalScopedString modules_res;
+ RenderModules(&modules_res, modules,


Looks like RenderModules is unrelated change.
Can you please extract into a separate patch with justification and tests?

RenderModules is a related change, I added it as part of stacktrace_printer.h to keep a similar pattern to other Render* calls. As other reviewers have noted, it is a unnecessary indirection and I will replace in favor of calling RenderModulesMarkup directly.

vitalybuka · 2023-09-08T00:17:20Z

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp

 const void *unwind_context) {}

-#if SANITIZER_CAN_SLOW_UNWIND
+# if SANITIZER_CAN_SLOW_UNWIND


please precommit format fixes in a separate patch

vitalybuka · 2023-09-08T00:20:35Z

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_markup.cpp

 Symbolizer *Symbolizer::PlatformInit() {
- return new (symbolizer_allocator_) Symbolizer({});
+ IntrusiveList<SymbolizerTool> tools;
+ SymbolizerTool *tool = MarkupSymbolizer::get(&symbolizer_allocator_);


can this use new() as well?

and I think it's rather should be created inside of Symbolizer();

Some error logging in llvm-exegesis under the subprocess executor just prints a generic failure information rather than any details about the error as we omit printing the string version of errno. This patch adds in printing errno at all relevant points in the subprocess executor that were previously missed. Reviewed By: courbet Differential Revision: https://reviews.llvm.org/D157682

…rence CurLexer is dereferenced and should not be null in clang::Preprocessor::SkipExcludedConditionalBlock(clang::SourceLocation, clang::SourceLocation, bool, bool, clang::SourceLocation) This patch adds an assert for NULL value check of pointer CurLexer and splits up all predicates so that, when/if a failure occurs, we'll be able to tell which predicate failed. Reviewed By: tahonermann Differential Revision: https://reviews.llvm.org/D158293

This is likely a rebase artifact. libcpp-has-no-concepts is not a Lit feature we ever define anymore.

Use forward decls instead of #including the header files. Differential Revision: https://reviews.llvm.org/D159421

This time from clang/Parse. Differential Revision: https://reviews.llvm.org/D159435

This removes a comment added in D159312, which warned people to not re-add a whitespace in the `((void*)0))` expression. After discussions happened in D159312, it doesn't seem like a permanent solution. While I'd like to keep the whitespace removed for now, given that at least it can be a band-aid to some users who use musl and clang's `stddef.h` at the same time, it seems the usage of them together is not something that's officially supported, and I should not be implying this should be the permanent solution by saying so in the comments. Reviewed By: aaron.ballman, ributzka Differential Revision: https://reviews.llvm.org/D159383

These functions have been deprecated since: commit 8b1d86a Author: Guillaume Chatelet <gchatelet@google.com> Date: Mon Jan 23 10:08:01 2023 +0000 commit 355cc3f Author: Guillaume Chatelet <gchatelet@google.com> Date: Tue Jan 24 10:39:58 2023 +0000 Differential Revision: https://reviews.llvm.org/D159448

…RS (llvm#65338)

Probably missed some cases. We also probably should rename the ML stuff to use a consistent capitalization scheme

…lvm#65294)

Buildbots failed after this landed, as reported at: <llvm#65267 (comment)> This reverts commit 9191ba7.

Cygwin shares the same limitations as traditional Windows executables for dynamic library loading, so disable building the dynamic library on Cygwin targets. Differential Revision: https://reviews.llvm.org/D155796

Differential Revision: https://reviews.llvm.org/D159455

It was common to see `value.getValue().empty()` or `value.size()` instead of the idiomatic `value.empty()`. Depends on D159455 Differential Revision: https://reviews.llvm.org/D159456

Operations that can divide by zero and can have immediate undefined behaviour should be marked as `NoMemoryEffect` rather than `Pure`. Depends on D159456 Reviewed By: weiweichen Differential Revision: https://reviews.llvm.org/D159457

… also fixed

…`vector` and `deque`. Differential Revision: https://reviews.llvm.org/D150747

avillega · 2023-09-11T17:46:19Z

Will have to close this PR, seems I messed up by doing a rebase (old workflow habit) will create a new one with comments from this one fixed.

avillega requested a review from a team as a code owner September 6, 2023 22:34

github-actions bot added the compiler-rt label Sep 6, 2023

avillega requested review from PiJoules and petrhosek September 6, 2023 22:35

fmayer reviewed Sep 7, 2023

View reviewed changes

vitalybuka requested changes Sep 7, 2023

View reviewed changes

vitalybuka requested changes Sep 8, 2023

View reviewed changes

boomanaiden154 and others added 21 commits September 11, 2023 17:10

[libc++][NFC] Remove an outdated UNSUPPORTED annotation

8e0a0cc

This is likely a rebase artifact. libcpp-has-no-concepts is not a Lit feature we ever define anymore.

[NFC] Remove unneeded header includes

63652df

Use forward decls instead of #including the header files. Differential Revision: https://reviews.llvm.org/D159421

[NFC] remove unneded header includes

a67009c

This time from clang/Parse. Differential Revision: https://reviews.llvm.org/D159435

[clang] NFCI: Change returned LanguageOptions pointer to reference

d34fe79

[clang] NFCI: Change returned AnalyzerOptions smart pointer to reference

d8df928

[clang][arcmt] NFCI: Change invocation const-ness

bf4dc24

[UpdateTestChecks] Add @llvm/pr-subscribers-testing-tools to CODEOWNE…

5e54067

…RS (llvm#65338)

Add regalloc team to CODEOWNERS (llvm#65208)

21c496f

Probably missed some cases. We also probably should rename the ML stuff to use a consistent capitalization scheme

Add a GitHub team to subscribe to changes to MLIR Presburger library (l…

2316c99

…lvm#65294)

Revert "[lit] Echo full RUN lines in case of external shells"

0e8c6cc

Buildbots failed after this landed, as reported at: <llvm#65267 (comment)> This reverts commit 9191ba7.

[polly] Dynamic libraries are not supported on Cygwin

708dcdc

Cygwin shares the same limitations as traditional Windows executables for dynamic library loading, so disable building the dynamic library on Cygwin targets. Differential Revision: https://reviews.llvm.org/D155796

[mlir][ub] Fix typo in ub.poison op documentation (NFC)

7b24230

Differential Revision: https://reviews.llvm.org/D159455

[mlir] Add empty to StringAttr

44d6949

It was common to see `value.getValue().empty()` or `value.size()` instead of the idiomatic `value.empty()`. Depends on D159455 Differential Revision: https://reviews.llvm.org/D159456

[docs] Mention that function multi-versioning using target_version is…

c1c8b38

… also fixed

Add amdgpu paths to CODEOWNERS (llvm#65229)

4787671

[libc++][ranges] Add benchmarks for the from_range constructors of …

0fc2b5f

…`vector` and `deque`. Differential Revision: https://reviews.llvm.org/D150747

HighCommander4 removed request for a team September 11, 2023 17:45

avillega closed this Sep 11, 2023

dtcxzyw mentioned this pull request Sep 19, 2023

[InstCombine] Canonicalize and(zext(A), B) into select A, B & 1, 0 #66740

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[sanitizer_symbolizer] Symbolizer Markup for linux. #65543

[sanitizer_symbolizer] Symbolizer Markup for linux. #65543

Uh oh!

avillega commented Sep 6, 2023

fmayer commented Sep 7, 2023

fmayer Sep 7, 2023

fmayer Sep 7, 2023

fmayer Sep 7, 2023

fmayer Sep 7, 2023

fmayer Sep 7, 2023

fmayer Sep 7, 2023

vitalybuka left a comment

vitalybuka Sep 8, 2023

avillega Sep 8, 2023

vitalybuka Sep 8, 2023

vitalybuka Sep 8, 2023

avillega Sep 8, 2023

vitalybuka Sep 8, 2023

vitalybuka Sep 8, 2023

vitalybuka Sep 8, 2023

avillega commented Sep 11, 2023

Labels

126 participants

[sanitizer_symbolizer] Symbolizer Markup for linux. #65543

[sanitizer_symbolizer] Symbolizer Markup for linux. #65543

Uh oh!

Conversation

avillega commented Sep 6, 2023

fmayer commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitalybuka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avillega commented Sep 11, 2023

Labels

126 participants