Open GL API Error Reporting

bogglez · Post by **bogglez** » Thu Jan 15, 2015 5:57 am

Found some more errors in libgl. I compiled with a more modern gcc on Linux, just to get some diagnostics:

gcc -Wall -Wextra -I$KOS_BASE/utils/dc-chain/newlib-2.0.0/newlib/libc/include -I/opt/toolchains/kos-ports/include -I/opt/toolchains/KallistiOS/include -I/opt/toolchains/KallistiOS/kernel/arch/dreamcast/include -I/opt/toolchains/KallistiOS/addons/include -D_arch_dreamcast -D_arch_sub_pristine -I include *.c 2> out.txt

The cull func one is a bug. The PVR init code is ok by coincidence, because the memory seems to always be zeroed on init.

Spoiler!

Here is a patch to fix C++-style function prototypes without arguments (void foo(void); is a prototype, while void foo(); is a definition)

Spoiler!

This is a patch to fix abuse of return foo(), where foo() returns a void:

Spoiler!

Code: Select all

diff --git a/gl-api.c b/gl-api.c
index b9fae8c..599426a 100755
--- a/gl-api.c
+++ b/gl-api.c
@@ -282,11 +282,11 @@ void APIENTRY(*glVertex3f)(GLfloat, GLfloat, GLfloat);
 void APIENTRY(*glVertex3fv)(const GLfloat *);
 
 void APIENTRY glVertex2f(GLfloat x, GLfloat y) {
-    return _glKosVertex3ft(x, y, 0.0f);
+    _glKosVertex3ft(x, y, 0.0f);
 }
 
 void APIENTRY glVertex2fv(const GLfloat *xy) {
-    return _glKosVertex3ft(xy[0], xy[1], 0.0f);
+    glKosVertex3ft(xy[0], xy[1], 0.0f);
 }
 
 void APIENTRY glRectf(GLfloat x1, GLfloat y1, GLfloat x2, GLfloat y2) {
@@ -312,11 +312,11 @@ void APIENTRY glRectfv(const GLfloat *v1, const GLfloat *v2) {
 }
 
 void APIENTRY glRecti(GLint x1, GLint y1, GLint x2, GLint y2) {
-    return glRectf((GLfloat)x1, (GLfloat)y1, (GLfloat)x2, (GLfloat)y2);
+    glRectf((GLfloat)x1, (GLfloat)y1, (GLfloat)x2, (GLfloat)y2);
 }
 
 void APIENTRY glRectiv(const GLint *v1, const GLint *v2) {
-    return glRectfv((const GLfloat *)v1, (const GLfloat *)v2);
+    glRectfv((const GLfloat *)v1, (const GLfloat *)v2);
 }
 
 void APIENTRY glKosVertex2f(GLfloat x, GLfloat y) {
@@ -923,4 +923,4 @@ GLubyte _glKosDepthMask() {
 
 GLuint _glKosVertexColor() {
     return GL_KOS_VERTEX_COLOR;
-}
\ No newline at end of file
+}
diff --git a/gl-arrays.c b/gl-arrays.c
index efdc366..1e0d4e9 100755
--- a/gl-arrays.c
+++ b/gl-arrays.c
@@ -502,8 +502,10 @@ static inline void _glKosElementMultiTexCoord2fU8C(GLuint count) {
 }
 
 static inline void _glKosElementMultiTexCoord2fU16(GLuint count) {
-    if(_glKosEnabledNearZClip())
-        return _glKosElementMultiTexCoord2fU16C(count);
+    if(_glKosEnabledNearZClip()) {
+        _glKosElementMultiTexCoord2fU16C(count);
+        return;
+    }
 
     GLuint i, index;
     GLfloat *t = GL_KOS_TEXCOORD1_POINTER;
@@ -519,8 +521,10 @@ static inline void _glKosElementMultiTexCoord2fU16(GLuint count) {
 }
 
 static inline void _glKosElementMultiTexCoord2fU8(GLuint count) {
-    if(_glKosEnabledNearZClip())
-        return _glKosElementMultiTexCoord2fU8C(count);
+    if(_glKosEnabledNearZClip()) {
+        _glKosElementMultiTexCoord2fU8C(count);
+        return;
+    }
 
     GLuint i, index;
     GLfloat *t = GL_KOS_TEXCOORD1_POINTER;
@@ -1109,8 +1113,10 @@ GLAPI void APIENTRY glDrawArrays(GLenum mode, GLint first, GLsizei count) {
     /* Compile the PVR polygon context with the currently enabled flags */
     _glKosArraysApplyHeader();
 
-    if(GL_KOS_VERTEX_SIZE == 2)
-        return _glKosDrawArrays2D(mode, first, count);
+    if(GL_KOS_VERTEX_SIZE == 2) {
+        _glKosDrawArrays2D(mode, first, count);
+        return;
+    }
 
     /* Destination of Output Vertex Array */
     pvr_vertex_t *dst = _glKosArraysDest();
@@ -1191,4 +1197,4 @@ void APIENTRY glClientActiveTextureARB(GLenum texture) {
     }
 
     GL_KOS_CLIENT_ACTIVE_TEXTURE = texture & 0xF;
-}
\ No newline at end of file
+}
diff --git a/gl-cap.c b/gl-cap.c
index 3857518..0e04063 100755
--- a/gl-cap.c
+++ b/gl-cap.c
@@ -31,7 +31,10 @@ static GLbitfield GL_KOS_ENABLE_CAP = 0;
 //== External API Functions ==//
 
 void APIENTRY glEnable(GLenum cap) {
-    if(cap >= GL_LIGHT0 && cap <= GL_LIGHT15) return _glKosEnableLight(cap);
+    if(cap >= GL_LIGHT0 && cap <= GL_LIGHT15) {
+        _glKosEnableLight(cap);
+        return;
+    }
 
     switch(cap) {
         case GL_TEXTURE_2D:
@@ -70,7 +73,10 @@ void APIENTRY glEnable(GLenum cap) {
 }
 
 void APIENTRY glDisable(GLenum cap) {
-    if(cap >= GL_LIGHT0 && cap <= GL_LIGHT15) return _glKosDisableLight(cap);
+    if(cap >= GL_LIGHT0 && cap <= GL_LIGHT15) {
+        _glKosDisableLight(cap);
+        return;
+    }
 
     switch(cap) {
         case GL_TEXTURE_2D:
@@ -249,4 +255,4 @@ GLubyte _glKosEnabledTexture2D() {
 
 GLubyte _glKosEnabledBlend() {
     return (GL_KOS_ENABLE_CAP & GL_KOS_ENABLE_BLENDING) >> 8;
-}
\ No newline at end of file
+}
diff --git a/gl-fog.c b/gl-fog.c
index 82dcd4b..0330cb9 100755
--- a/gl-fog.c
+++ b/gl-fog.c
@@ -21,20 +21,17 @@ void APIENTRY glFogi(GLenum pname, GLint param) {
         case GL_FOG_MODE:
             switch(param) {
                 case GL_LINEAR:
-                    return pvr_fog_table_linear(FOG_START, FOG_END);
+                    pvr_fog_table_linear(FOG_START, FOG_END);
+                    break;
 
                 case GL_EXP:
-                    return pvr_fog_table_exp(FOG_DENSITY);
+                    pvr_fog_table_exp(FOG_DENSITY);
+                    break;
 
                 case GL_EXP2:
-                    return pvr_fog_table_exp2(FOG_DENSITY);
-
-                default:
-                    return;
+                    pvr_fog_table_exp2(FOG_DENSITY);
+                    break;
             }
-
-        default:
-            return;
     }
 }
 
@@ -44,59 +41,53 @@ void APIENTRY glFogf(GLenum pname, GLfloat param) {
             FOG_START = param;
 
             if(FOG_MODE == GL_LINEAR)
-                return pvr_fog_table_linear(FOG_START, FOG_END);
-
-            return;
+                pvr_fog_table_linear(FOG_START, FOG_END);
+            break;
 
         case GL_FOG_END:
             FOG_END = param;
 
             if(FOG_MODE == GL_LINEAR)
-                return pvr_fog_table_linear(FOG_START, FOG_END);
-
-            return;
+                pvr_fog_table_linear(FOG_START, FOG_END);
+            break;
 
         case GL_FOG_DENSITY:
             FOG_DENSITY = param;
 
             if(FOG_MODE == GL_EXP)
-                return pvr_fog_table_exp(FOG_DENSITY);
+                pvr_fog_table_exp(FOG_DENSITY);
 
             if(FOG_MODE == GL_EXP2)
-                return pvr_fog_table_exp2(FOG_DENSITY);
-
-            return;
-
-        default:
-            return;
+                pvr_fog_table_exp2(FOG_DENSITY);
+            break;
     }
 }
 
 void APIENTRY glFogfv(GLenum pname, const GLfloat *params) {
     switch(pname) {
         case GL_FOG_MODE:
-            return glFogi(pname, (GLint) * params);
+            glFogi(pname, (GLint) * params);
+            break;
 
         case GL_FOG_DENSITY:
             FOG_DENSITY = *params;
 
             if(FOG_MODE == GL_EXP)
-                return pvr_fog_table_exp(FOG_DENSITY);
-
-            if(FOG_MODE == GL_EXP2)
-                return pvr_fog_table_exp2(FOG_DENSITY);
-
-            return;
+                pvr_fog_table_exp(FOG_DENSITY);
+            else if(FOG_MODE == GL_EXP2)
+                pvr_fog_table_exp2(FOG_DENSITY);
+            break;
 
         case GL_FOG_START:
         case GL_FOG_END:
-            return glFogf(pname, *params);
+            glFogf(pname, *params);
+            break;
 
         case GL_FOG_COLOR:
-            return pvr_fog_table_color(params[3], params[0], params[1], params[2]);
+            pvr_fog_table_color(params[3], params[0], params[1], params[2]);
+            break;
 
         case GL_FOG_INDEX:
-        default:
-            return;
+            break;
     }
 }
diff --git a/gl-texture.c b/gl-texture.c
index 93e44e7..01fc95c 100755
--- a/gl-texture.c
+++ b/gl-texture.c
@@ -92,8 +92,11 @@ GLvoid *_glKosTextureData(GLuint index) {
 }
 
 void _glKosCompileHdrTx() {
-    return GL_KOS_TEXTURE_UNIT[GL_TEXTURE0_ARB & 0xF] ?
-           _glKosCompileHdrT(GL_KOS_TEXTURE_UNIT[GL_TEXTURE0_ARB & 0xF]) : _glKosCompileHdr();
+    if(GL_KOS_TEXTURE_UNIT[GL_TEXTURE0_ARB & 0xF]) {
+        _glKosCompileHdrT(GL_KOS_TEXTURE_UNIT[GL_TEXTURE0_ARB & 0xF]);
+    else {
+        glKosCompileHdr();
+    }
 }
 
 GL_TEXTURE_OBJECT *_glKosBoundMultiTexID() {
@@ -357,4 +360,4 @@ void APIENTRY glActiveTextureARB(GLenum texture) {
     }
 
     GL_KOS_ACTIVE_TEXTURE = texture & 0xF;
-}
\ No newline at end of file
+}

BTW GLsizei is defined as a signed 32-bit int in the spec, although the parameters are not supposed to be negative. But functions that check their arguments explicitly check for GLsize w; if(w < 0) { error }. This could make client code underflow.

PH3NOM · Post by **PH3NOM** » Sat Jan 17, 2015 7:55 pm

Thanks again for interest.

To be fair, the PVR init code is due to the fact that BlueCrab has changed the PVR init parameters after that build was submitted (autosort_disabled has not always been an init parameter

).
But, in fact, that code has already been changed in my local repository before you mentioned it here

The GLsizei stuff, although contradictory; makes sense. IMO a size type can not be negative, yet I wondered why we check for less than zero in the error generation. One of those things you ask why the design allows a size type to have a negative value

But my compiler did not generate such warnings, I guess it would be good to go ahead and make the changes there.

This is a patch to fix abuse of return foo(), where foo() returns a void:

Are you sure that is a problem? I know that java does not allow the return of a void function call from a void function, but I do not have any information that says it is prohibited in C?

Here is a patch to fix C++-style function prototypes without arguments (void foo(void); is a prototype, while void foo(); is a definition)

Maybe I am dense here, IMO (void foo(); is a declaration, while void foo(){;} is a definition).

Post by **BlueCrab** » Sat Jan 17, 2015 8:51 pm

Returning something that is void from a void function is perfectly valid in C, however it is a bit wonky and thus unclear. I'd personally recommend against it simply because it is unclear. As I said though, it is perfectly valid and legal in C to do so.

The practice of putting (void) instead of () in function prototypes is a vestige of traditional C. Technically, not putting (void) as the parameter list does not specify a parameter list for the function, and thus passing any number of parameters is technically valid. Placing (void) as the parameter list makes it clear to the compiler that the function in question accepts no parameters. That said, this is a vestige of traditional C, and I'd guess that pretty much all compilers treat int func(); and int func(void); the same for most intents. That said, make sure that things match at the place where things are prototyped and at the place where the function itself is actually defined -- so if you add void as the parameter list in the header files as suggested, please do so in the files where those functions are actually implemented as well.

With regard to the pvr_init parameters, that's why the new item in the parameters is at the end and the default (which keeps the old functionality) is zero. That way (regardless of excessive warning levels), things still work as they should as long as all the code involved is recompiled (since any non-automatic scoped variables are always initialized to zero by the compiler and/or runtime, and C defines that structures with automatic scope should initialize any values not specified in an initializer to 0 as well).

bogglez · Post by **bogglez** » Sun Jan 18, 2015 5:30 am

BlueCrab, as far as I know returning void is illegal in both old and modern C standards.
In old ones. "void f(); (accepts a constant but unknown number of arguments)"
In new ones it is entirely deprecated.
Quick summary: https://en.wikipedia.org/wiki/Void_type

That said, I find it very confusing to comprehend foreign code written in such a way.

Post by **BlueCrab** » Sun Jan 18, 2015 8:50 am

I think that in C++ it is actually valid to return an expression with void type from a function returning void. Thus, most C compilers that also do C++ will accept it as valid as well.

That said, you do appear to be correct that it is technically invalid in C:

ISO/IEC 9899:TC3 Draft 6.8.6.4.1 wrote:A return statement with an expression shall not appear in a function whose return type is void. A return statement without an expression shall only appear in a function whose return type is void.

I do agree with you that it is weird and wonky to do so.

As for the other part (about (void) vs () for functions with no arguments), you've basically said the same thing I did.

bogglez · Post by **bogglez** » Sun Jan 18, 2015 2:47 pm

BlueCrab wrote: As for the other part (about (void) vs () for functions with no arguments), you've basically said the same thing I did.

Heh, sorry, I was in a rush.

bogglez · Post by **bogglez** » Mon Aug 31, 2015 10:16 pm

I realized that the library is called libgl.a instead of libGL.a. It's a bit annoying when switching between PC and DC builds (requires special casing in Makefile) and may be confusing to people who are used to passing -lGL.

PH3NOM · Post by **PH3NOM** » Tue Sep 01, 2015 7:49 pm

Heh, on Windows this is a non-issue

But on Linux, yes, the FS is case sensitive. Hmmz, should be an easy enough fix

bogglez · Post by **bogglez** » Sun Jan 03, 2016 12:43 pm

The following test program displays 2 problems.
https://bitbucket.org/bogglez/libgl15/s ... ew-default
Please comment out glGetError since it's not defined in kos-libGL

1. glVertexPointer must be called every frame, which is non-standard. The program will work if the call is moved after glClear.

2. Nothing shows up in kos-libGL when face culling is enabled. Can you verify that the order is correct? It seems to me that kos-libGL doesn't obey the standard order of CCW?
https://www.opengl.org/sdk/docs/man2/xh ... ntFace.xml

Post by **BlueCrab** » Sun Jan 03, 2016 9:29 pm

bogglez wrote:The following test program displays 2 problems.
https://bitbucket.org/bogglez/libgl15/s ... ew-default
Please comment out glGetError since it's not defined in kos-libGL

It is now. See https://sourceforge.net/p/cadcdev/libgl ... 19097c5c4/ .

bogglez · Post by **bogglez** » Mon Jan 04, 2016 1:31 pm

Patch for GLbyte->GLubyte in glGetString and some missing defines I use.
BTW the texture upload functions are non-standard, especially for compressed textures. Should look up the specs for sized texture formats. Right now the size is supplied in the wrong argument basically.

Code: Select all

diff --git a/gl-cap.c b/gl-cap.c
index af4ac60..fc8e704 100644
--- a/gl-cap.c
+++ b/gl-cap.c
@@ -228,22 +228,22 @@ void APIENTRY glGetFloatv(GLenum pname, GLfloat *params) {
     }
 }
 
-const GLbyte *glGetString(GLenum name) {
+const GLubyte *glGetString(GLenum name) {
     switch(name) {
         case GL_VENDOR:
-            return "KallistiOS";
+            return (const GLubyte*)"KallistiOS";
 
         case GL_RENDERER:
-            return "PowerVR2 CLX2 100mHz";
+            return (const GLubyte*)"PowerVR2 CLX2 100mHz";
 
         case GL_VERSION:
-            return "KGL 1.x";
+            return (const GLubyte*)"KGL 1.x";
 
         case GL_EXTENSIONS:
-            return "GL_ARB_framebuffer_object, GL_ARB_multitexture";
+            return (const GLubyte*)"GL_ARB_framebuffer_object, GL_ARB_multitexture";
     }
 
-    return "GL_KOS_ERROR: ENUM Unsupported\n";
+    return (const GLubyte*)"GL_KOS_ERROR: ENUM Unsupported\n";
 }
 
 //===============================================================================//
diff --git a/include/gl.h b/include/gl.h
index 9808254..dd80aa8 100644
--- a/include/gl.h
+++ b/include/gl.h
@@ -85,6 +85,8 @@ __BEGIN_DECLS
 #define GL_DEPTH_WRITEMASK    0x0B72
 #define GL_DEPTH_COMPONENT    0x1902
 
+#define GL_DITHER             0x0BD0
+
 /* Blending: Simply Need to Map GL constants to PVR constants */
 #define GL_BLEND_DST            0x0BE0
 #define GL_BLEND_SRC            0x0BE1
@@ -169,6 +171,7 @@ __BEGIN_DECLS
 #define GL_TEXTURE30                      0x84DE
 #define GL_TEXTURE31                      0x84DF
 #define GL_ACTIVE_TEXTURE                 0x84E0
+#define GL_MULTISAMPLE                    0x809D
 #define GL_CLIENT_ACTIVE_TEXTURE          0x84E1
 
 #define GL_CURRENT_BIT                          0x00000001
@@ -302,6 +305,8 @@ __BEGIN_DECLS
 #define GL_MAX_PROJECTION_STACK_DEPTH     0x0D38
 #define GL_MAX_TEXTURE_STACK_DEPTH        0x0D39
 #define GL_MAX_VIEWPORT_DIMS              0x0D3A
+#define GL_BGR                            0x80E0
+#define GL_BGRA                           0x80E1
 #define GL_MAX_ELEMENTS_VERTICES          0x80E8
 #define GL_MAX_ELEMENTS_INDICES           0x80E9
 #define GL_MAX_TEXTURE_UNITS              0x84E2
@@ -645,7 +650,7 @@ GLAPI GLuint APIENTRY glKosMipMapTexSize(GLuint width, GLuint height);
 GLAPI void APIENTRY glGetIntegerv(GLenum pname, GLint *params);
 GLAPI void APIENTRY glGetFloatv(GLenum pname, GLfloat *params);
 GLAPI GLboolean APIENTRY glIsEnabled(GLenum cap);
-GLAPI const GLbyte* APIENTRY glGetString(GLenum name);
+GLAPI const GLubyte* APIENTRY glGetString(GLenum name);
 
 /* Multi-Texture Extensions - Currently not supported in immediate mode */
 GLAPI void APIENTRY glActiveTextureARB(GLenum texture);

Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting

Re: Open GL API Error Reporting