Page MenuHomePhabricator

[dfsan] Add stack-trace printing functions to dfsan interface
ClosedPublic

Authored by gbalats on Jun 11 2021, 5:57 PM.

Diff Detail

Event Timeline

gbalats requested review of this revision.Jun 11 2021, 5:57 PM
gbalats created this revision.
Herald added a subscriber: Restricted Project. · View Herald TranscriptJun 11 2021, 5:57 PM

why __sanitizer_print_stack_trace is not enough?

why __sanitizer_print_stack_trace is not enough?

We wanted to record stack traces to some databases for post-process. This is similar to the request of printing dfsan origin traces to a buffer.

compiler-rt/lib/dfsan/dfsan.cpp
845

Please replace __sanitizer_print_stack_trace by dfsan_print_stack_trace.

The only difference between them is that __sanitizer_print_stack_trace uses common_flags()->fast_unwind_on_fatal.
I think that controls fast unwind. Since DFSan always runs on linux where fast unwind can also be supported, so that flag is true.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h
142 ↗(On Diff #351611)

@vitalybuka: is fine to make this public?

If not, it is still fine for dfsan_sprint_stack_trace, because we can always post-process stack frames to fill out unrelated frames.
I think we need that kind of post-filtering anyway.

why __sanitizer_print_stack_trace is not enough?

We wanted to record stack traces to some databases for post-process. This is similar to the request of printing dfsan origin traces to a buffer.

To clarify, I understand why you need dfsan_sprint_stack_trace.
But dfsan_print_stack_trace looks very similar to sanitizer_print_stack_trace.
Please consider to adjust your use-case and just use
sanitizer_print_stack_trace.

But please don't wait for my review and proceed with @stephan.yichao.zhao if you disagree.

vitalybuka added inline comments.Jun 11 2021, 6:58 PM
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h
142 ↗(On Diff #351611)

fine with me

gbalats added inline comments.Jun 12 2021, 12:47 AM
compiler-rt/lib/dfsan/dfsan.cpp
845

Not sure what the suggestion is. You mean I should remove __sanitizer_print_stack_trace altogether? Or replace its body with a call to dfsan_print_stack_trace? Or something else?

Not sure if removing is okay, since I see a declaration provided in sanitizer/common_interface_defs.h and every *san.cpp provides its own definition. Having one call the other will insert another frame and I really don't want to use/introduce a macro like GET_FATAL_STACK_TRACE_PC_BP that hides the declaration of stack (which makes the code below harder to understand since we call many methods of stack but its type is not evident). Unless we replace it with an inlined function.

compiler-rt/lib/dfsan/dfsan.cpp
845

I suggest we keep __sanitizer_print_stack_trace, and could define it like

void __sanitizer_print_stack_trace() {
   GET_CALLER_PC_BP;
   GET_STORE_STACK_TRACE_PC_BP(pc, bp);
   stack.Print();
}

It is fine to remove GET_FATAL_STACK_TRACE_PC_BP because in our case the 4th argument is always true.
GET_STORE_STACK_TRACE_PC_BP has been shared with other code too.
That GET_CALLER_PC_BP equals to stack.PopStackFrames. So we do not need to change sanitizer_stacktrace.h.

compiler-rt/test/dfsan/stack_trace.c
52
gbalats updated this revision to Diff 351971.Jun 14 2021, 12:57 PM
gbalats marked an inline comment as done.

Remove dfsan_print_stack_trace

gbalats marked 2 inline comments as done.Jun 14 2021, 12:58 PM
gbalats added inline comments.
compiler-rt/lib/dfsan/dfsan.cpp
845

Done. Removed dfsan_print_stack_trace.

compiler-rt/test/dfsan/stack_trace.c
52

Acknowledged. By removing the leading whitespace, it's not evident why tinybuf, of size 8, can hold just these contents.

gbalats marked an inline comment as done.Jun 14 2021, 12:58 PM
This revision is now accepted and ready to land.Jun 14 2021, 12:59 PM