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.

Sign up for GitHub

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

Jump to bottom

Add proper ASAN support to task switching #29489

Merged
merged 2 commits into from
Sep 13, 2019
Merged

Add proper ASAN support to task switching #29489

merged 2 commits into from
Sep 13, 2019

Conversation

vchuravy
Copy link
Member

@vchuravy vchuravy commented Oct 2, 2018

When we are using ASAN we need to inform it when we switch stacks.

WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffee525b000; bottom 0x7efe8a835000; size: 0x01005aa26000 (1101032218624)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189

Originally needed by #22631

@vtjnash any ideas for how to do this cleverly?

@vchuravy
Copy link
Member Author

vchuravy commented Oct 3, 2018 •
edited
Loading

@vtjnash would you mind if I rewrite jl_swap_fiber and related function to operate on tasks instead of contexts?, so that instead of littering the ASAN support throughout the code base it is in one place? Similar to how Folly does this facebook/folly@2ea64dd

I should also note that this is not yet complete, since ASAN is still not happy so I most likely missed a switch.

@vchuravy vchuravy requested a review from vtjnash October 3, 2018 00:57
@vtjnash
Copy link
Member

vtjnash commented Oct 3, 2018

Sure, however you feel is convenient. I settled on this API to emphasize that it is expected to only access this field. But it’s not a hard restriction.

vtjnash
vtjnash reviewed Oct 4, 2018
View reviewed changes
src/gc-debug.c Outdated Show resolved Hide resolved
@StefanKarpinski StefanKarpinski mentioned this pull request Oct 5, 2018
Merged
@vchuravy vchuravy mentioned this pull request Oct 7, 2018
Merged
@fingolfin fingolfin mentioned this pull request Oct 16, 2018
Merged
@vchuravy vchuravy closed this Jun 11, 2019
@vchuravy vchuravy deleted the vc/asan_tasks branch June 11, 2019 15:07
@vchuravy vchuravy reopened this Jun 11, 2019
@vchuravy vchuravy restored the vc/asan_tasks branch June 11, 2019 15:17
@vchuravy vchuravy changed the base branch from master to vc/asan June 11, 2019 15:19
@vchuravy vchuravy force-pushed the vc/asan_tasks branch 2 times, most recently from 1624377 to 8689c79 Compare July 29, 2019 13:04
@vchuravy vchuravy changed the base branch from vc/asan to master July 29, 2019 13:05
@vchuravy vchuravy changed the title [WIP] Add proper ASAN support to task switching Add proper ASAN support to task switching Jul 29, 2019
vchuravy
vchuravy commented Jul 29, 2019
View reviewed changes
src/task.c Outdated Show resolved Hide resolved
@vchuravy vchuravy requested a review from vtjnash July 29, 2019 13:16
@vtjnash
Copy link
Member

vtjnash commented Aug 5, 2019

Seems good

vtjnash
vtjnash reviewed Aug 5, 2019
View reviewed changes
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Seems good

@vchuravy vchuravy force-pushed the vc/asan_tasks branch 2 times, most recently from 2e831f2 to 4e897bb Compare September 13, 2019 04:32
@vchuravy
Copy link
Member Author

It turns out that the issue I was experiencing was a seqfault in

julia/src/gc-debug.c

Line 553 in 668341e

char *p = *stack_p;
called from

julia/src/gc-debug.c

Line 577 in 668341e

static void gc_scrub_task(jl_task_t *ta)

and not that we have to annotate setjmp/longjmp that we use for exception handling (since we don't change stacks).
(Nathan and I did that successfully today at 2e831f2)

@NHDaly
Copy link
Member

NHDaly commented Sep 13, 2019

Haha oops. Wellllll, it was fun anyway! 😂 And an interesting exercise!

@vchuravy vchuravy merged commit a90d92a into master Sep 13, 2019
@vchuravy vchuravy deleted the vc/asan_tasks branch September 13, 2019 19:24
@JeffBezanson
Copy link
Member

Causes this compiler warning:

In file included from /home/jeff/src/julia/src/rtutils.c:23:0:
/home/jeff/src/julia/src/julia.h:95:20: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
 static inline void sanitizer_finish_switch_fiber() {}
                    ^
/home/jeff/src/julia/src/julia.h: In function ‘sanitizer_finish_switch_fiber’:
/home/jeff/src/julia/src/julia.h:95:20: warning: old-style function definition [-Wold-style-definition]

@vchuravy
Copy link
Member Author

Sorry about that, noticed it after merging.

I have included a fix in #33259

@vchuravy vchuravy mentioned this pull request Sep 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vchuravy @vtjnash @NHDaly @JeffBezanson