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

fix UInt underflow in line indices #4782

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

fix UInt underflow in line indices #4782

wants to merge 11 commits into from

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Feb 12, 2025 •
edited
Loading

Description

Fixes an integer underflow in GLMakie as is happening in #4771 (comment). May solve #4373 (but I have no way to confirm that)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@SimonDanisch SimonDanisch mentioned this pull request Feb 12, 2025
Open
3 tasks
@MakieBot
Copy link
Collaborator

MakieBot commented Feb 12, 2025 •
edited
Loading

Benchmark Results

SHA: ca93c83739a943e8533bed339f660bc1eaf4dff8

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

@chriswaudby
Copy link

Afraid it doesn't appear to solve #4373.

I'm quite new so just to be clear what I did to test this:

activate --temp
add Makie#ff/fix-uint-underflow
add NMRAnalysis
using NMRAnalysis
relaxation2d(.....) # open interface
# seg fault

@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 12, 2025

I don't think you'll get the GLMakie changes from that. You probably need add GLMakie#ff/fix-uint-underflow

@chriswaudby
Copy link

chriswaudby commented Feb 12, 2025 via email

@SimonDanisch
Copy link
Member

@chriswaudby, maybe this fixes it? #4779

@chriswaudby
Copy link

chriswaudby commented Feb 12, 2025 via email

ffreyer
ffreyer commented Feb 14, 2025
View reviewed changes
GLMakie/src/GLAbstraction/GLRender.jl
Comment on lines 116 to 120
for elem in to_value(vao.indices)
# TODO: Should this exclude last(elem), i.e. shift a:b to (a-1):(b-1)
# instead of (a-1):b?
vao_boundscheck(N_vert, last(elem), vao)
glDrawArrays(mode, max(first(elem) - 1, 0), length(elem) + 1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I basically just want to highlight the TODO I added here. I think this should just shift indices from 1-based to 0-based but with length(elem) + 1 that's not what's happening. Is this a mistake?

ffreyer
ffreyer commented Feb 14, 2025
View reviewed changes
GLMakie/src/GLAbstraction/GLTypes.jl
Comment on lines +193 to +196
function length(vao::GLVertexArray)
isempty(vao.buffers) && return -1
return mapreduce(length, min, values(vao.buffers))
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also want to highlight this change. Taking the min length may remove (unavailable) vertices when rendering triggers between buffer updates (if that's even possible). And it may trigger errors in checks. Should also be safer.
Not sure if the no-buffers case can ever trigger. I thought maybe it was triggering with voxels but they do have quad vertices.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 14, 2025

@chriswaudby There are some more changes here that could fix the segfault if it's caused by bad vertex indexing. If you set GLMakie.GLAbstraction.GLMAKIE_DEBUG[] = true you should also get errors if vertex indices are out of bounds.

@chriswaudby
Copy link

My first impression is this may fix the problem - I'll check with my student as she was also seeing this bug and confirm.

With GLMAKIE_DEBUG[] switched on, I do get a string of error messages once I quit Julia - don't know if this is connected but I'm copying them below in case.

Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Texture{Float16, 2} has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLVertexArray{Int64} has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLVertexArray{Int64} has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLVertexArray{Int64} has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.GLVertexArray{Int64} has not been freed.
Error: GLMakie.GLAbstraction.GLProgram has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Shader has not been freed.
Error: GLMakie.GLAbstraction.Texture{FixedPointNumbers.Normed{UInt8, 8}, 2} has not been freed.
Error: GLMakie.GLAbstraction.Texture{ColorTypes.RGBA{Float16}, 2} has not been freed.
Error: GLMakie.GLAbstraction.Texture{GLMakie.GLAbstraction.DepthStencil_24_8, 2} has not been freed.
Error: GLMakie.GLAbstraction.Texture{GeometryBasics.Vec{2, UInt32}, 2} has not been freed.
Error: GLMakie.GLAbstraction.Texture{ColorTypes.RGBA{FixedPointNumbers.Normed{UInt8, 8}}, 2} has not been freed.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 14, 2025

Those errors should be fine if they don't pop up at runtime. (E.g. between closing and opening new windows, from calling GLMakie.closeall() or GC.gc()) We shouldn't need to do that cleanup explicitly when the process exits because OpenGL already does it. Would probably be better to do it though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
PR review
Status: Work in progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ffreyer @MakieBot @chriswaudby @SimonDanisch