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

ASAN improvements #41675

Merged
merged 4 commits into from
Jul 27, 2021
Merged

ASAN improvements #41675

merged 4 commits into from
Jul 27, 2021

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Jul 22, 2021

  • Moves the ASAN default options to the loader executable; this is the same as 1a82590, which got lost again after we introduced a loader executable
  • This requires moving the sanitizer-related preprocessor definitions out of julia.h, which isn't included by the loader. I've put them into platform.h
  • Additionally, define the NO_RTLD environment variable that's needed for LBT to work under ASAN. Not pretty, but makes a sanitized executable work out-of-the-box without having to set stuff in your environment.

May be worth backporting to 1.7 to get a better ASAN experience there.

Copy link
Member

@tkf tkf left a comment

Choose a reason for hiding this comment

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

I think we can simplify the CI script contrib/asan/ now a bit. It's useful for testing __asan_default_options works. I just pushed my suggestion directly to the PR. I tested locally and it works for me.

It looks like the CI result will show up here: https://buildkite.com/julialang/julia-master-experimental/builds/222

contrib/asan/Make.user.asan Show resolved Hide resolved
contrib/asan/check.jl Show resolved Hide resolved
@DilumAluthge
Copy link
Member

Yeah, if you want to see the CI logs for the asan Buildkite job, they'll be available here: https://buildkite.com/julialang/julia-master-experimental

@tkf
Copy link
Member

tkf commented Jul 22, 2021

I'm a bit hesitant to support backporting too many things in the middle of the release hustle. But maybe it can be treated as a bug fix? Anyway, if we backport this, #41522 should go in, too, for updating LBT.

@DilumAluthge
Copy link
Member

DilumAluthge commented Jul 22, 2021

I'm a bit hesitant to support backporting too many things in the middle of the release hustle.

We could always backport it to 1.7.1, right? Or any other future 1.7.x release?

@DilumAluthge
Copy link
Member

Anyway, I'll take the backport label off for now.

@maleadt maleadt merged commit 8ece865 into master Jul 27, 2021
@maleadt maleadt deleted the tb/asan branch July 27, 2021 06:05
@DilumAluthge
Copy link
Member

@maleadt @vchuravy @tkf Are we going to backport this to 1.7, or no?

@maleadt
Copy link
Member Author

maleadt commented Jul 27, 2021

No need, and it would require a backport of #41522 too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants