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

Turn @inbounds into a no-op for debug builds #5926

Closed
wants to merge 1 commit into from
Closed

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Feb 24, 2014

There are a number of outstanding weird bugs at the moment (at least #5885, #5106, JuliaGraphics/Gtk.jl#69, JuliaImages/Images.jl#35). In the hopes of reducing the number of times we need to don the rubber gloves, I decided to look into what would be required to conveniently disable @inbounds. This seems like a particularly simple solution.

@@ -153,7 +153,8 @@ macro boundscheck(yesno,blk)
end

macro inbounds(blk)
:(@boundscheck false $(esc(blk)))
isdebug = ccall(:jl_is_debugbuild, Bool, ())
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The return type should be Cint. Also, we could just return esc(blk) and avoid the bounds checking stuff entirely in debug builds.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I tried that. If you make it Cint, then you need to say ccall(:jl_is_debugbuild, Bool, ()) == 1, and the problem I ran into is that it seems that == isn't yet defined at this stage of the boot process. Declaring the return type as Bool fixed this. This was easier than what I started to do, changing the return type to jl_value_t*.

FYI in util.jl there's also a ccall to jl_is_debugbuild that declares the return type as Bool, which is where I got the idea from.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Thank you for pointing that out. That needs to be fixed too. The type simply has to be correct, otherwise there is no way we can be sure to generate correct code for calling the function and horrible things can happen.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I get it now; this only works because it's a little endian machine (a Bool is a Uint8), and there might be memory issues.

OK, so unless there's a better way I'll resurrect my branch that changes the return type to a jl_value_t*.

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 24, 2014

Sorry, the Images link should have been JuliaImages/Images.jl#59

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 24, 2014

OK, see if this is better.

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 24, 2014

I'm guessing that because of precompilation, if you want to see whether base itself is a problem, you presumably have to force a full rebuild (just rebuilding libjulia is not good enough). Is this a major concern?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 24, 2014

I don't like the idea that the macro depends upon a runtime parameter (debugbuild), but I also don't see a feasible way of avoiding it

@JeffBezanson
Copy link
Sponsor Member

Strictly speaking, this should be independent of whether julia is built in
debug mode. I think there should be a command line switch, possibly in
addition to this. Of course, it can't affect precompiled code no matter
what.
On Feb 23, 2014 9:26 PM, "Jameson Nash" notifications@github.com wrote:

I don't like the idea that the macro depends upon a runtime parameter
(debugbuild), but I also don't see a feasible way of avoiding it

Reply to this email directly or view it on GitHubhttps://github.com//pull/5926#issuecomment-35853130
.

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 24, 2014

Not quite sure what you have in mind. Feel free to run with this if you prefer.

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 24, 2014

Re the precompilation issue, I guess my main concern is that, currently on this branch, there are actually three scenarios:

  1. @inbounds does its normal thing
  2. @inbounds is a no-op for user-code, but does its normal thing for base julia code (this is what you get currently on this branch if you say make debug but don't need to rebuild julia)
  3. @inbounds is a no-op for everything

In some ways it's nice having these all available; on the other hand, users might think they're operating in scenario 3 when really they're in scenario 2, and hence haven't tested things as thoroughly as they expect.

I suppose one option is to force julia to rebuild. I'm a Makefile noob, but it seems we could use http://stackoverflow.com/a/7643966/1939814 and create a boundscheck target. If I were doing it, I'd probably set this up to trigger a force-rebuild of both libjulia and base each time (i.e., it wouldn't be very smart). And would we need to be able to combine this setting with release/debug?

Alternatively, we could just document this, and tell people to touch base/base.jl before make debug if they want scenario 3.

Regarding your command-line flag, my confusion was stemming from wanting option 3. I do see how to do that with option 2, and in a way that's independent of a debug build. I can implement that if you want.

However, perhaps this is getting complex enough that it's becoming worse than a different solution: pure documentation. "If you want to disable @inbounds, edit the definition of macro inbounds in base/base.jl to ... and rebuild julia."

@timholy
Copy link
Sponsor Member Author

timholy commented Apr 29, 2014

Looks like Jeff or someone implemented this already, closing.

@timholy timholy closed this Apr 29, 2014
@timholy timholy deleted the teh/debugbuild branch July 24, 2014 00:09
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

3 participants