This is an archive of the discontinued LLVM Phabricator instance.

Differential D24628

[ASAN] Pass previous stack information through __sanitizer_finish_switch_fiber
ClosedPublic

Authored by andriigrynenko on Sep 15 2016, 1:58 PM.

Details

Reviewers
kcc
filcab
dvyukov
blastrock
Summary

This patch extends __sanitizer_finish_switch_fiber method to optionally return previous stack base and size.

This solves the problem of coroutines/fibers library not knowing the original stack context from which the library is used. It's incorrect to assume that such context is always the default stack of current thread (e.g. one such library may be used from a fiber/coroutine created by another library). Bulding a separate stack tracking mechanism would not only duplicate AsanThread, but also require each coroutines/fibers library to integrate with it.

Diff Detail

Repository
rL LLVM

Event Timeline

andriigrynenko updated this revision to Diff 71550.Sep 15 2016, 1:58 PM
andriigrynenko retitled this revision from to [ASAN] Pass previous stack information through __sanitizer_finish_switch_fiber.
andriigrynenko updated this object.
andriigrynenko added reviewers: kcc, blastrock, dvyukov, filcab.
andriigrynenko set the repository for this revision to rL LLVM.
andriigrynenko added a subscriber: cfe-commits.
Herald added a subscriber: kubamracek. · View Herald TranscriptSep 15 2016, 1:58 PM
blastrock edited edge metadata.Sep 15 2016, 2:15 PM

Seems fine to me, but I think you forgot to update the tests :)

andriigrynenko planned changes to this revision.Sep 15 2016, 2:34 PM
andriigrynenko updated this revision to Diff 71677.Sep 16 2016, 11:11 AM
andriigrynenko edited edge metadata.

fix the unit test

andriigrynenko added inline comments.Sep 16 2016, 11:14 AM
test/asan/TestCases/Linux/swapcontext_annotation.cc
9 ↗(On Diff #71677)

The test was actually broken in trunk (not updated for the fakestack argument). Apparently it was never run (considered unsupported) because of wrong targets here.

dvyukov edited edge metadata.Sep 16 2016, 12:11 PM

We need a test that passes non-NULL to these arguments and shows how to use the returned values.

test/asan/TestCases/Linux/swapcontext_annotation.cc
47 ↗(On Diff #71677)

#include <stddef.h> for NULL

blastrock added inline comments.Sep 16 2016, 12:42 PM
test/asan/TestCases/Linux/swapcontext_annotation.cc
9 ↗(On Diff #71677)

Indeed, my bad, I forgot to compile and run the tests after the last modification I made to that patch. Sorry for that.

I didn't need to change this line to run them on my computer though, but if you know what you are doing with this line, it's ok. And it's strange indeed that no buildfarm caught this...

andriigrynenko updated this revision to Diff 71698.Sep 16 2016, 2:18 PM
andriigrynenko edited edge metadata.

update unit test to use non-null values

filcab edited edge metadata.Sep 16 2016, 2:19 PM

Please add some printf calls to the test, to show that you have the correct stack+size, too.

Thanks for working on this.
Filipe

lib/asan/asan_thread.cc
141 ↗(On Diff #71677)

Please make sure you're accounting for this. It seems the test is always passing nullptr to start_switch, which shouldn't be done unless you're finishing up a fiber execution, IIRC.

162 ↗(On Diff #71677)

I think the usual style is: one statement => no braces.

test/asan/TestCases/Linux/swapcontext_annotation.cc
9 ↗(On Diff #71677)

They wouldn't catch tests not being run.
Test with LLVM_LIT_ARGS=-v (instead of -sv) on your cmake config and double-check that the tests are run.

47 ↗(On Diff #71677)

nit: nullptr?

andriigrynenko updated this revision to Diff 71717.Sep 16 2016, 5:44 PM
andriigrynenko edited edge metadata.

Addressing comments.

andriigrynenko added inline comments.Sep 16 2016, 5:45 PM
test/asan/TestCases/Linux/swapcontext_annotation.cc
176–199 ↗(On Diff #71717)

This only checks the first iteration of the loop. Can I do it better with FileCheck ?

dvyukov added a comment.Sep 19 2016, 5:15 AM

We need a test that passes non-NULL to these arguments and shows how to use the returned values.

test/asan/TestCases/Linux/swapcontext_annotation.cc
176–199 ↗(On Diff #71717)

Yes, you can add CHECKs for the second iteration as well.

andriigrynenko added inline comments.Sep 26 2016, 1:29 PM
test/asan/TestCases/Linux/swapcontext_annotation.cc
176–199 ↗(On Diff #71717)

@dvyukov: What I meant is that I didn't see a way to have some kind of loop in these CHECKs. I don't want to copy-paste these same CHECKs 30 times :)

andriigrynenko updated this revision to Diff 72571.Sep 26 2016, 2:35 PM

Do CHECKs for all iterations of the loop.

dvyukov added a comment.Sep 27 2016, 12:55 AM

LGTM

Any other comments? Or I will submit it tomorrow.

test/asan/TestCases/Linux/swapcontext_annotation.cc
7 ↗(On Diff #72571)

nice

dvyukov accepted this revision.Sep 28 2016, 5:37 AM
dvyukov edited edge metadata.

Submitted in 282582.

This revision is now accepted and ready to land.Sep 28 2016, 5:37 AM
dvyukov closed this revision.Sep 28 2016, 5:37 AM

Revision Contents

PathSize
compiler-rt/
include/
sanitizer/
4 lines
lib/
asan/
3 lines
18 lines

Diff 71550

compiler-rt/include/sanitizer/common_interface_defs.h

Context not available.
// use-after-return detection. // use-after-return detection.
void __sanitizer_start_switch_fiber(void **fake_stack_save, void __sanitizer_start_switch_fiber(void **fake_stack_save,
const void *bottom, size_t size); const void *bottom, size_t size);
void __sanitizer_finish_switch_fiber(void *fake_stack_save); void __sanitizer_finish_switch_fiber(void *fake_stack_save,
const void **bottom_old,
size_t *size_old);
#ifdef __cplusplus #ifdef __cplusplus
} // extern "C" } // extern "C"
#endif #endif
Context not available.

compiler-rt/lib/asan/asan_thread.h

Context not available.
} }
void StartSwitchFiber(FakeStack **fake_stack_save, uptr bottom, uptr size); void StartSwitchFiber(FakeStack **fake_stack_save, uptr bottom, uptr size);
void FinishSwitchFiber(FakeStack *fake_stack_save); void FinishSwitchFiber(FakeStack *fake_stack_save, uptr *bottom_old,
uptr *size_old);
bool has_fake_stack() { bool has_fake_stack() {
return !atomic_load(&stack_switching_, memory_order_relaxed) && return !atomic_load(&stack_switching_, memory_order_relaxed) &&
Context not available.

compiler-rt/lib/asan/asan_thread.cc

Context not available.
current_fake_stack->Destroy(this->tid()); current_fake_stack->Destroy(this->tid());
} }
void AsanThread::FinishSwitchFiber(FakeStack *fake_stack_save) { void AsanThread::FinishSwitchFiber(FakeStack *fake_stack_save,
uptr *bottom_old,
uptr *size_old) {
if (!atomic_load(&stack_switching_, memory_order_relaxed)) { if (!atomic_load(&stack_switching_, memory_order_relaxed)) {
Report("ERROR: finishing a fiber switch that has not started\n"); Report("ERROR: finishing a fiber switch that has not started\n");
Die(); Die();
Context not available.
fake_stack_ = fake_stack_save; fake_stack_ = fake_stack_save;
} }
if (bottom_old) {
*bottom_old = stack_bottom_;
}
if (size_old) {
*size_old = stack_top_ - stack_bottom_;
}
stack_bottom_ = next_stack_bottom_; stack_bottom_ = next_stack_bottom_;
stack_top_ = next_stack_top_; stack_top_ = next_stack_top_;
atomic_store(&stack_switching_, 0, memory_order_release); atomic_store(&stack_switching_, 0, memory_order_release);
Context not available.
} }
SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_INTERFACE_ATTRIBUTE
void __sanitizer_finish_switch_fiber(void* fakestack) { void __sanitizer_finish_switch_fiber(void* fakestack,
const void **bottom_old,
uptr *size_old) {
AsanThread *t = GetCurrentThread(); AsanThread *t = GetCurrentThread();
if (!t) { if (!t) {
VReport(1, "__asan_finish_switch_fiber called from unknown thread\n"); VReport(1, "__asan_finish_switch_fiber called from unknown thread\n");
return; return;
} }
t->FinishSwitchFiber((FakeStack*)fakestack); t->FinishSwitchFiber((FakeStack*)fakestack,
(uptr*)bottom_old,
(uptr*)size_old);
} }
} }
Context not available.