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

Clear the IRBuilder's insertion point after emitting a function #18054

Merged
merged 2 commits into from
Aug 17, 2016

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Aug 16, 2016

The function might get finalized, invalidating the IP. However, in some cases this invalid IP may get saved and restored, accessing the invalid IP while doing so.

Example code path accessing an invalid IP:

  • jl_cfunction_object: nested_compile=true, but doesn't change IP
  • gen_cfun_wrapper
  • jl_compile_linfo: saves and restores invalid IP

Example code:

function foobar()
    return Cint(0)
end

cfunction(foobar, Cint, ())

# compilation of cfunction makes builder.BB point to an invalid BB
# compilation of foobar restores that invalid BB (as per code path explained above)

The function might get finalized, invalidating the IP.
However, in some cases this invalid IP may get saved and restored, accessing the invalid IP while doing so.

Example code path accessing an invalid IP:
 -> jl_cfunction_object (nested_compile=true, but doesn't change IP)
 -> gen_cfun_wrapper
 -> jl_compile_linfo (saves and restores invalid IP)
@JeffBezanson
Copy link
Member

Thanks, lgtm.

@tkelman
Copy link
Contributor

tkelman commented Aug 16, 2016

can that code path be tested?

@maleadt
Copy link
Member Author

maleadt commented Aug 16, 2016

I can add the snippet above to the tests, but we'd still need an ASAN-like builder to detect if it fails.

Note that this test requires a memory sanitizer (ASAN, valgrind) to detect failure.
@vtjnash
Copy link
Member

vtjnash commented Aug 16, 2016

I'm hoping we can simply eliminate all globals from codegen in the near future (and wrap almost the whole file inside the class jl_codectx_t instead of passing it manually). I think we're pretty close.

@maleadt
Copy link
Member Author

maleadt commented Aug 16, 2016

Good to know. In the meantime, this gets us a little closer again to passing all tests with ASAN enabled (I'll be pushing some devdocs on how to set it all up, because it has been a bit of a struggle).

@JeffBezanson JeffBezanson merged commit c477aab into master Aug 17, 2016
@tkelman tkelman deleted the tb/builder_clear_ip branch August 17, 2016 22:33
@tkelman
Copy link
Contributor

tkelman commented Aug 20, 2016

If you think this should be backported, do speak up.

@StefanKarpinski StefanKarpinski added bugfix This change fixes an existing bug backport pending 0.5 labels Aug 20, 2016
maleadt pushed a commit that referenced this pull request Aug 20, 2016
Clear the IRBuilder's insertion point after emitting a function
maleadt added a commit that referenced this pull request Aug 21, 2016
The function might get finalized, invalidating the IP.
However, in some cases this invalid IP may get saved and restored, accessing the invalid IP while doing so.

Example code path accessing an invalid IP:
 -> jl_cfunction_object (nested_compile=true, but doesn't change IP)
 -> gen_cfun_wrapper
 -> jl_compile_linfo (saves and restores invalid IP)

(cherry picked from commit ebd24a8, ref #18054)
maleadt added a commit that referenced this pull request Aug 21, 2016
Note that this test requires a memory sanitizer (ASAN, valgrind) to detect failure.

(cherry picked from commit a5fdd74, ref #18054)
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
Note that this test requires a memory sanitizer (ASAN, valgrind) to detect failure.
maleadt added a commit that referenced this pull request Oct 17, 2016
Similar to #18054. Proper fix would be not to have a global builder.
tkelman pushed a commit that referenced this pull request Feb 22, 2017
Similar to #18054. Proper fix would be not to have a global builder.

(cherry picked from commit 5eb07c4)
ref #18991
fcard pushed a commit to fcard/julia that referenced this pull request Feb 28, 2017
Similar to JuliaLang#18054. Proper fix would be not to have a global builder.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants