Repository: brendanzab/gl-rs (language: rust)

Issue:
Static loader is unsafe
Scenario:

```
Thread 1              |  Thread 2
========              |  ========
driver1::make_current |
gl::load_with         |
                      |  driver2::make_current
                      |  gl::Begin() // crash!
```

In general, loading GL functions can be thread-specific, and only the struct loader can deal with this. Every function in the static loader should be marked unsafe, and it should be heavily discouraged.



Discussion / hints:
The struct loader is unsafe too, even if we mark it as `NoSend`.
You can make a context current, call `Gl::load_with`, then remove the current context, and all your calls will crash.

If you couple gl-rs with a library like glfw or glutin we can potentially build a completely safe api.

``` rust
// this moves the window context to be part of gl
let gl = ctx.make_current();

gl.drawElements(...);
// ect

// this unwraps the gl moving the context back into ctx
let ctx = gl.to_context();
```

We could supply a trait to do this conversion for you, but it requires the windowing library to work with it.

No, because if you create multiple windows you are still unsafe.

@tomaka the idea is that the opengl functions are queried each time you make_current.

And how would you know that the user doesn't manually call `wglMakeCurrent`?
If he does, it's _your_ code that ends up crashing. Hence the `unsafe`.

@csherratt Doesn't calling `make_current` over and over slow things down a great deal on Windows?

I am leaning towards making all functions in the static loader `unsafe`.

I think make_current should always be unsafe.

Agreed.

So the `load_with` function is safe, all gl functions are unsafe, and `make_current` is unsafe too?

If we define `unsafe` as _the safetiness of this function depends on factors not controllable by the function itself_ (in other words, _the safetiness depends on the environment_), then:
- all GL functions are unsafe because they are safe only if there is a valid context
- similarly `get_proc_addr` is unsafe because it requires a valid context
- `make_current` is safe, because the context passed to it is a Rust object and is always valid

Alternatively you could define `unsafe` as _calling an unsafe function can lead to unexpected or unsafe behaviors in functions marked as safe_ (by _an unsafe function_, I mean `make_current` here), which is probably what csherratt had in mind.


Fix patch:
diff --git a/examples/static_basic.rs b/examples/static_basic.rs
index ff8c6953..d8ad4228 100644
--- a/examples/static_basic.rs
+++ b/examples/static_basic.rs
@@ -46,8 +46,10 @@ fn main() {
         glfw.poll_events();
 
         // Clear the screen to black
-        gl::ClearColor(0.3, 0.3, 0.3, 1.0);
-        gl::Clear(gl::COLOR_BUFFER_BIT);
+        unsafe {
+            gl::ClearColor(0.3, 0.3, 0.3, 1.0);
+            gl::Clear(gl::COLOR_BUFFER_BIT);
+        }
 
         // Swap buffers
         window.swap_buffers();
diff --git a/examples/static_triangle.rs b/examples/static_triangle.rs
index cdce09c9..5092b9a6 100644
--- a/examples/static_triangle.rs
+++ b/examples/static_triangle.rs
@@ -53,8 +53,9 @@ fn start(argc: int, argv: *const *const u8) -> int {
 }
 
 fn compile_shader(src: &str, ty: GLenum) -> GLuint {
-    let shader = gl::CreateShader(ty);
+    let shader;
     unsafe {
+        shader = gl::CreateShader(ty);
         // Attempt to compile the shader
         src.with_c_str(|ptr| gl::ShaderSource(shader, 1, &ptr, ptr::null()));
         gl::CompileShader(shader);
@@ -75,27 +76,25 @@ fn compile_shader(src: &str, ty: GLenum) -> GLuint {
     shader
 }
 
-fn link_program(vs: GLuint, fs: GLuint) -> GLuint {
+fn link_program(vs: GLuint, fs: GLuint) -> GLuint { unsafe {
     let program = gl::CreateProgram();
     gl::AttachShader(program, vs);
     gl::AttachShader(program, fs);
     gl::LinkProgram(program);
-    unsafe {
-        // Get the link status
-        let mut status = gl::FALSE as GLint;
-        gl::GetProgramiv(program, gl::LINK_STATUS, &mut status);
-
-        // Fail on error
-        if status != (gl::TRUE as GLint) {
-            let mut len: GLint = 0;
-            gl::GetProgramiv(program, gl::INFO_LOG_LENGTH, &mut len);
-            let mut buf = Vec::from_elem(len as uint - 1, 0u8);     // subtract 1 to skip the trailing null character
-            gl::GetProgramInfoLog(program, len, ptr::null_mut(), buf.as_mut_ptr() as *mut GLchar);
-            panic!("{}", str::from_utf8(buf.as_slice()).expect("ProgramInfoLog not valid utf8"));
-        }
+    // Get the link status
+    let mut status = gl::FALSE as GLint;
+    gl::GetProgramiv(program, gl::LINK_STATUS, &mut status);
+
+    // Fail on error
+    if status != (gl::TRUE as GLint) {
+        let mut len: GLint = 0;
+        gl::GetProgramiv(program, gl::INFO_LOG_LENGTH, &mut len);
+        let mut buf = Vec::from_elem(len as uint - 1, 0u8);     // subtract 1 to skip the trailing null character
+        gl::GetProgramInfoLog(program, len, ptr::null_mut(), buf.as_mut_ptr() as *mut GLchar);
+        panic!("{}", str::from_utf8(buf.as_slice()).expect("ProgramInfoLog not valid utf8"));
     }
     program
-}
+} }
 
 fn main() {
     let glfw = glfw::init(glfw::FAIL_ON_ERRORS).unwrap();
@@ -150,22 +149,24 @@ fn main() {
         // Poll events
         glfw.poll_events();
 
-        // Clear the screen to black
-        gl::ClearColor(0.3, 0.3, 0.3, 1.0);
-        gl::Clear(gl::COLOR_BUFFER_BIT);
+        unsafe {
+            // Clear the screen to black
+            gl::ClearColor(0.3, 0.3, 0.3, 1.0);
+            gl::Clear(gl::COLOR_BUFFER_BIT);
 
-        // Draw a triangle from the 3 vertices
-        gl::DrawArrays(gl::TRIANGLES, 0, 3);
+            // Draw a triangle from the 3 vertices
+            gl::DrawArrays(gl::TRIANGLES, 0, 3);
+        }
 
         // Swap buffers
         window.swap_buffers();
     }
 
-    // Cleanup
-    gl::DeleteProgram(program);
-    gl::DeleteShader(fs);
-    gl::DeleteShader(vs);
     unsafe {
+    // Cleanup
+        gl::DeleteProgram(program);
+        gl::DeleteShader(fs);
+        gl::DeleteShader(vs);
         gl::DeleteBuffers(1, &vbo);
         gl::DeleteVertexArrays(1, &vao);
     }
diff --git a/examples/struct_triangle.rs b/examples/struct_triangle.rs
index 054ebf88..64ad2f46 100644
--- a/examples/struct_triangle.rs
+++ b/examples/struct_triangle.rs
@@ -70,8 +70,9 @@ fn start(argc: int, argv: *const *const u8) -> int {
 }
 
 fn compile_shader(gl: &Gl, src: &str, ty: GLenum) -> GLuint {
-    let shader = gl.CreateShader(ty);
+    let shader;
     unsafe {
+        shader = gl.CreateShader(ty);
         // Attempt to compile the shader
         src.with_c_str(|ptr| gl.ShaderSource(shader, 1, &ptr, ptr::null()));
         gl.CompileShader(shader);
@@ -92,7 +93,7 @@ fn compile_shader(gl: &Gl, src: &str, ty: GLenum) -> GLuint {
     shader
 }
 
-fn link_program(gl: &Gl, vs: GLuint, fs: GLuint) -> GLuint {
+fn link_program(gl: &Gl, vs: GLuint, fs: GLuint) -> GLuint { unsafe {
     let program = gl.CreateProgram();
     gl.AttachShader(program, vs);
     gl.AttachShader(program, fs);
@@ -112,7 +113,7 @@ fn link_program(gl: &Gl, vs: GLuint, fs: GLuint) -> GLuint {
         }
     }
     program
-}
+}}
 
 fn main() {
     let glfw = glfw::init(glfw::FAIL_ON_ERRORS).unwrap();
@@ -167,22 +168,24 @@ fn main() {
         // Poll events
         glfw.poll_events();
 
-        // Clear the screen to black
-        gl.ClearColor(0.3, 0.3, 0.3, 1.0);
-        gl.Clear(gl::COLOR_BUFFER_BIT);
+        unsafe {
+            // Clear the screen to black
+            gl.ClearColor(0.3, 0.3, 0.3, 1.0);
+            gl.Clear(gl::COLOR_BUFFER_BIT);
 
-        // Draw a triangle from the 3 vertices
-        gl.DrawArrays(gl::TRIANGLES, 0, 3);
+            // Draw a triangle from the 3 vertices
+            gl.DrawArrays(gl::TRIANGLES, 0, 3);
+        }
 
         // Swap buffers
         window.swap_buffers();
     }
 
-    // Cleanup
-    gl.DeleteProgram(program);
-    gl.DeleteShader(fs);
-    gl.DeleteShader(vs);
     unsafe {
+        // Cleanup
+        gl.DeleteProgram(program);
+        gl.DeleteShader(fs);
+        gl.DeleteShader(vs);
         gl.DeleteBuffers(1, &vbo);
         gl.DeleteVertexArrays(1, &vao);
     }
diff --git a/src/gl/lib.rs b/src/gl/lib.rs
index 621c2241..eb26fa8e 100644
--- a/src/gl/lib.rs
+++ b/src/gl/lib.rs
@@ -54,7 +54,7 @@
 //! gl::TEXTURE_2D;
 //!
 //! // calling a function
-//! gl::DrawArrays(gl::TRIANGLES, 0, 3);
+//! unsafe { gl::DrawArrays(gl::TRIANGLES, 0, 3) };
 //!
 //! // functions that take pointers are unsafe
 //! # let shader = 0;
diff --git a/src/gl_generator/generators/global_gen.rs b/src/gl_generator/generators/global_gen.rs
index 4bf1b033..2a451d2f 100644
--- a/src/gl_generator/generators/global_gen.rs
+++ b/src/gl_generator/generators/global_gen.rs
@@ -107,23 +107,7 @@ fn write_fns(ecx: &ExtCtxt, registry: &Registry) -> Vec<P<ast::Item>> {
             None => "".to_string()
         };
 
-        ecx.parse_item(if c.is_safe {
-            format!(
-                "#[allow(non_snake_case)] #[allow(unused_variables)] #[allow(dead_code)]
-                #[inline] #[unstable] {doc} pub fn {name}({params}) -> {return_suffix} {{ \
-                    unsafe {{ \
-                        __gl_imports::mem::transmute::<_, extern \"system\" fn({types}) -> {return_suffix}>\
-                            (storage::{name}.f)({idents}) \
-                    }} \
-                }}",
-                name = c.proto.ident,
-                doc = doc,
-                params = super::gen_parameters(ecx, c).into_iter().map(|p| p.to_source()).collect::<Vec<String>>().connect(", "),
-                types = super::gen_parameters(ecx, c).into_iter().map(|p| p.ty.to_source()).collect::<Vec<String>>().connect(", "),
-                return_suffix = super::gen_return_type(ecx, c).to_source(),
-                idents = super::gen_parameters(ecx, c).into_iter().map(|p| p.pat.to_source()).collect::<Vec<String>>().connect(", "),
-            )
-        } else {
+        ecx.parse_item(
             format!(
                 "#[allow(non_snake_case)] #[allow(unused_variables)] #[allow(dead_code)]
                 #[inline] #[unstable] {doc} pub unsafe fn {name}({typed_params}) -> {return_suffix} {{ \
@@ -136,7 +120,7 @@ fn write_fns(ecx: &ExtCtxt, registry: &Registry) -> Vec<P<ast::Item>> {
                 return_suffix = super::gen_return_type(ecx, c).to_source(),
                 idents = super::gen_parameters(ecx, c).into_iter().map(|p| p.pat.to_source()).collect::<Vec<String>>().connect(", "),
             )
-        })
+        )
     }).collect()
 }
 
diff --git a/src/gl_generator/generators/struct_gen.rs b/src/gl_generator/generators/struct_gen.rs
index 50333cc7..f75de543 100644
--- a/src/gl_generator/generators/struct_gen.rs
+++ b/src/gl_generator/generators/struct_gen.rs
@@ -220,34 +220,17 @@ fn write_impl(ecx: &ExtCtxt, registry: &Registry, ns: &Ns) -> P<ast::Item> {
         modules = registry.cmd_iter().map(|c| {
             use syntax::ext::quote::rt::ToSource;
 
-            if c.is_safe {
-                format!(
-                    "#[allow(non_snake_case)] #[allow(unused_variables)] #[allow(dead_code)]
-                    #[inline] #[unstable] pub fn {name}(&self, {params}) -> {return_suffix} {{ \
-                        unsafe {{ \
-                            __gl_imports::mem::transmute::<_, extern \"system\" fn({types}) -> {return_suffix}>\
-                                (self.{name}.f)({idents}) \
-                        }} \
-                    }}",
-                    name = c.proto.ident,
-                    params = super::gen_parameters(ecx, c).into_iter().map(|p| p.to_source()).collect::<Vec<String>>().connect(", "),
-                    types = super::gen_parameters(ecx, c).into_iter().map(|p| p.ty.to_source()).collect::<Vec<String>>().connect(", "),
-                    return_suffix = super::gen_return_type(ecx, c).to_source(),
-                    idents = super::gen_parameters(ecx, c).into_iter().map(|p| p.pat.to_source()).collect::<Vec<String>>().connect(", "),
-                )
-            } else {
-                format!(
-                    "#[allow(non_snake_case)] #[allow(unused_variables)] #[allow(dead_code)]
-                    #[inline] #[unstable] pub unsafe fn {name}(&self, {typed_params}) -> {return_suffix} {{ \
-                        __gl_imports::mem::transmute::<_, extern \"system\" fn({typed_params}) -> {return_suffix}>\
-                            (self.{name}.f)({idents}) \
-                    }}",
-                    name = c.proto.ident,
-                    typed_params = super::gen_parameters(ecx, c).into_iter().map(|p| p.to_source()).collect::<Vec<String>>().connect(", "),
-                    return_suffix = super::gen_return_type(ecx, c).to_source(),
-                    idents = super::gen_parameters(ecx, c).into_iter().map(|p| p.pat.to_source()).collect::<Vec<String>>().connect(", "),
-                )
-            }
+            format!(
+                "#[allow(non_snake_case)] #[allow(unused_variables)] #[allow(dead_code)]
+                #[inline] #[unstable] pub unsafe fn {name}(&self, {typed_params}) -> {return_suffix} {{ \
+                    __gl_imports::mem::transmute::<_, extern \"system\" fn({typed_params}) -> {return_suffix}>\
+                        (self.{name}.f)({idents}) \
+                }}",
+                name = c.proto.ident,
+                typed_params = super::gen_parameters(ecx, c).into_iter().map(|p| p.to_source()).collect::<Vec<String>>().connect(", "),
+                return_suffix = super::gen_return_type(ecx, c).to_source(),
+                idents = super::gen_parameters(ecx, c).into_iter().map(|p| p.pat.to_source()).collect::<Vec<String>>().connect(", "),
+            )
         }).collect::<Vec<String>>().connect("\n")
     ))
 }
diff --git a/src/gl_generator/registry.rs b/src/gl_generator/registry.rs
index 4ffd40a3..c4404b1e 100644
--- a/src/gl_generator/registry.rs
+++ b/src/gl_generator/registry.rs
@@ -225,6 +225,9 @@ pub struct Binding {
 pub struct Cmd {
     pub proto: Binding,
     pub params: Vec<Binding>,
+    /// True if this command doesn't take any pointers.
+    ///
+    /// Unused by the built-in generators.
     pub is_safe: bool,
     pub alias: Option<String>,
     pub vecequiv: Option<String>,


Test patch:
diff --git a/tests/generators_symbols.rs b/tests/generators_symbols.rs
index 1a295adf..4134d3fa 100644
--- a/tests/generators_symbols.rs
+++ b/tests/generators_symbols.rs
@@ -67,7 +67,7 @@ mod egl {
 
 #[test]
 #[ignore]
-fn test_gl() {
+fn test_gl() { unsafe {
     gl::Clear(gl::COLOR_BUFFER_BIT);
     let _: libc::c_uint = gl::CreateProgram();
     gl::CompileShader(5);
@@ -76,32 +76,32 @@ fn test_gl() {
         gl::GetActiveUniformBlockiv(0, 0, gl::UNIFORM_BLOCK_REFERENCED_BY_GEOMETRY_SHADER,
             std::ptr::null_mut());
     }
-}
+}}
 
 #[test]
 #[ignore]
-fn test_gles() {
+fn test_gles() { unsafe {
     gles::Clear(gles::COLOR_BUFFER_BIT);
     let _: libc::c_uint = gles::CreateProgram();
     gles::CompileShader(5);
-}
+}}
 
 #[test]
 #[ignore]
-fn test_glx() {
+fn test_glx() { unsafe {
     let _ = unsafe { glx::GetProcAddress(std::mem::uninitialized()) };
     unsafe { glx::SwapBuffers(std::mem::uninitialized(), std::mem::uninitialized()) };
-}
+}}
 
 #[test]
 #[ignore]
-fn test_wgl() {
+fn test_wgl() { unsafe {
     let _: wgl::types::HGLRC = unsafe { wgl::CreateContext(std::mem::uninitialized()) };
-}
+}}
 
 #[test]
 #[ignore]
-fn test_egl() {
+fn test_egl() { unsafe {
     let _ = [
         egl::SURFACE_TYPE, egl::WINDOW_BIT,
         egl::BLUE_SIZE, 8,
@@ -112,4 +112,4 @@ fn test_egl() {
 
     let _ = egl::GetDisplay(egl::DEFAULT_DISPLAY);
     egl::Terminate(unsafe { std::mem::uninitialized() });
-}
+}}
diff --git a/tests/gl_symbols.rs b/tests/gl_symbols.rs
index fee7def3..84b2bfc9 100644
--- a/tests/gl_symbols.rs
+++ b/tests/gl_symbols.rs
@@ -5,7 +5,7 @@ extern crate libc;
 
 #[test]
 #[ignore]
-fn symbols_exist() {
+fn symbols_exist() { unsafe {
 	gl::Clear(gl::COLOR_BUFFER_BIT);
 	let _: libc::c_uint = gl::CreateProgram();
 	gl::CompileShader(5);
@@ -14,7 +14,7 @@ fn symbols_exist() {
         gl::GetActiveUniformBlockiv(0, 0, gl::UNIFORM_BLOCK_REFERENCED_BY_GEOMETRY_SHADER,
             std::ptr::null_mut());
     }
-}
+} }
 
 #[test]
 fn fallback_works() {