Compiler warning with newer GCC on linux
Peter Mayo <[email protected]>
| Newsgroups | gmane.comp.video.mesa3d.user |
|---|---|
| Message-ID | <[email protected]> |
Hi users/developers for Mesa, In compiling some of the mesa project in the chromium situation I ran across a compile warning which looks valid and easily fixable. I would like to pass along a solution in case it has value to you, either as part of the next release, or as a downstream fix for other users. The problem shows up as a warning from GCC 4.3 and probably up. MesaLib/src/egl/main/eglconfig.c: In function _eglCompareConfigs: MesaLib/src/egl/main/eglconfig.h:98: error: array subscript is below array bounds This comes about because of the nexted inline functions _eglIndexConfig and _eglGetConfigKey which although in the case of the call site in CompareConfigs are called with valid attributes, the call is nested deeply enough that it appears GCC excercises it's option on not inlining the functions, and compiled without specific values of arguments in optimized mode (assert turns empty) they could access element -1 of the Storage array - hence the warning. It seems to me that the smallest kind of fix to this is to move the assert down (since each call to eglIndexConfig is followed by an assert(idx>0) ) and return 0 instead of -1; providing equivalent protection in the debug case and less random behavior in the non-debug case (-01.patch) Alternately, if it does not affect performance too much, adding "if (idx < 0) return 0;" between the assert and the return in _eglGetConfigKey would achieve deterministic performance and no potential illegal accesses. A third alternative, which is more intrusive/larger but should not affect performance is to write a macro for the inner portion of the compare loop, and replace the local array with code that simply lists the attribute values in place. (see -02.patch) Alternately, you could disable the warning for the illegal access which would be a bit of a shame, since with the warning the compiler would warn you at compile time if you passed an illegal atribute. About the best time to get such a warning. For what it's worth, your mileage may vary, etc, Peter. _______________________________________________ mesa-users mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-users
fix-2011-01-03-01.patch
(application/octet-stream, 985 B)
Index: MesaLib/src/egl/main/eglconfig.h
===================================================================
--- MesaLib/src/egl/main/eglconfig.h (repository)
+++ MesaLib/src/egl/main/eglconfig.h (working copy)
@@ -38,7 +38,7 @@
/**
* Given a key, return an index into the storage of the config.
- * Return -1 if the key is invalid.
+ * The key must be valid, or an assertion may be thrown.
*/
static INLINE EGLint
_eglIndexConfig(const _EGLConfig *conf, EGLint key)
@@ -54,7 +54,8 @@
return _EGL_CONFIG_FIRST_EXTRA_ATTRIB;
#endif
default:
- return -1;
+ assert(0);
+ return 0;
}
}
@@ -82,7 +83,6 @@
_eglSetConfigKey(_EGLConfig *conf, EGLint key, EGLint val)
{
EGLint idx = _eglIndexConfig(conf, key);
- assert(idx >= 0);
conf->Storage[idx] = val;
}
@@ -94,7 +94,6 @@
_eglGetConfigKey(const _EGLConfig *conf, EGLint key)
{
EGLint idx = _eglIndexConfig(conf, key);
- assert(idx >= 0);
return conf->Storage[idx];
}
fix-2011-01-03-02.patch
(application/octet-stream, 2.3 KB)
Index: MesaLib/src/egl/main/eglconfig.c
===================================================================
--- MesaLib/src/egl/main/eglconfig.c (repository)
+++ MesaLib/src/egl/main/eglconfig.c (working copy)
@@ -605,17 +605,18 @@
_eglCompareConfigs(const _EGLConfig *conf1, const _EGLConfig *conf2,
const _EGLConfig *criteria, EGLBoolean compare_id)
{
- const EGLint compare_attribs[] = {
- EGL_BUFFER_SIZE,
- EGL_SAMPLE_BUFFERS,
- EGL_SAMPLES,
- EGL_DEPTH_SIZE,
- EGL_STENCIL_SIZE,
- EGL_ALPHA_MASK_SIZE,
- };
+/*
+ * This is the common business of comparing an attribute of a config.
+ * We do it a lot here; make it easier to read.
+ */
+#define _INTERNAL_COMPARE(attrib) \
+ val1 = GET_CONFIG_ATTRIB(conf1, attrib); \
+ val2 = GET_CONFIG_ATTRIB(conf2, attrib); \
+ if (val1 != val2) \
+ return (val1-val2);
+
EGLint val1, val2;
EGLBoolean rgb_buffer;
- EGLint i;
if (conf1 == conf2)
return 0;
@@ -623,17 +636,12 @@
/* the enum values have the desired ordering */
assert(EGL_NONE < EGL_SLOW_CONFIG);
assert(EGL_SLOW_CONFIG < EGL_NON_CONFORMANT_CONFIG);
- val1 = GET_CONFIG_ATTRIB(conf1, EGL_CONFIG_CAVEAT);
- val2 = GET_CONFIG_ATTRIB(conf2, EGL_CONFIG_CAVEAT);
- if (val1 != val2)
- return (val1 - val2);
+ _INTERNAL_COMPARE(EGL_CONFIG_CAVEAT)
/* the enum values have the desired ordering */
assert(EGL_RGB_BUFFER < EGL_LUMINANCE_BUFFER);
- val1 = GET_CONFIG_ATTRIB(conf1, EGL_COLOR_BUFFER_TYPE);
- val2 = GET_CONFIG_ATTRIB(conf2, EGL_COLOR_BUFFER_TYPE);
- if (val1 != val2)
- return (val1 - val2);
+ _INTERNAL_COMPARE(EGL_COLOR_BUFFER_TYPE)
+
rgb_buffer = (val1 == EGL_RGB_BUFFER);
if (criteria) {
@@ -672,12 +680,12 @@
if (val1 != val2)
return (val2 - val1);
+ _INTERNAL_COMPARE(EGL_BUFFER_SIZE)
+ _INTERNAL_COMPARE(EGL_SAMPLE_BUFFERS)
+ _INTERNAL_COMPARE(EGL_SAMPLES)
+ _INTERNAL_COMPARE(EGL_DEPTH_SIZE)
+ _INTERNAL_COMPARE(EGL_STENCIL_SIZE)
+ _INTERNAL_COMPARE(EGL_ALPHA_MASK_SIZE)
- for (i = 0; i < ARRAY_SIZE(compare_attribs); i++) {
- val1 = GET_CONFIG_ATTRIB(conf1, compare_attribs[i]);
- val2 = GET_CONFIG_ATTRIB(conf2, compare_attribs[i]);
- if (val1 != val2)
- return (val1 - val2);
- }
/* EGL_NATIVE_VISUAL_TYPE cannot be compared here */