Lua compile issue

[gilboa]

Hello,

I'm trying to build spring for the up-coming Fedora 15 release and I'm facing a Lua compilation issue.

/builddir/build/BUILD/spring_0.82.7.1/rts/Lua/LuaMaterial.cpp: In member function 'void LuaMaterial::Execute(const LuaMaterial&) const':
/builddir/build/BUILD/spring_0.82.7.1/rts/Lua/LuaMaterial.cpp:382:91: error: taking address of temporary [-fpermissive]

Looking at the code, I can only assume that GCC 4.6 really doesn't like this code part:

Code: Select all

if (shadowParamsLoc >= 0) {
		glUniform4fv(shadowParamsLoc, 1, const_cast<float*>(&(shadowHandler->GetShadowParams().x)));
	}

(Most likely a data locality issue with GetShadowParams).

I could simply -fpermissive and drop the error altogether, but I rather find some other (nicer?) solution (E.g. copy the value to a local variable and push it from there).

- Gilboa

[gilboa]

FWIW I'm trying to see if GCC will be willing to accept this:

Code: Select all

--- rts/Lua/LuaMaterial.cpp.old	2011-01-08 02:45:23.000000000 +0200
+++ rts/Lua/LuaMaterial.cpp	2011-03-10 07:05:05.265917526 +0200
@@ -337,6 +337,8 @@
 
 void LuaMaterial::Execute(const LuaMaterial& prev) const
 {
+	float fTemp;
+
 	if (prev.postList != 0) {
 		glCallList(prev.postList);
 	}
@@ -379,7 +381,8 @@
 	}
 
 	if (shadowParamsLoc >= 0) {
-		glUniform4fv(shadowParamsLoc, 1, const_cast<float*>(&(shadowHandler->GetShadowParams().x)));
+		fTemp = shadowHandler->GetShadowParams().x;
+		glUniform4fv(shadowParamsLoc, 1, &fTemp);
 	}
 
 	const int maxTex = std::max(texCount, prev.texCount);

[Tobi]

That solution will not work. The "4fv" part at the end of glUniform4fv means it expects to get a pointer to 4 floats, so by copying only the one float to a temporary you solve the compiler error, but introduce a bug. (The other three floats being garbage.)

You'll need to copy the whole float4 to a temporary and then pass in the address of the x component of that temporary. That should probably get rid of the error and not introduce a bug

[hoijui]

minor things:
don't follow that C rule of declaring local vars at function start, but declare them where they are used. maybe call it something like shadowParams instead of fTemp. add a comment why the temp var is needed, like // GCC 4.6+ requires .... without a comment, someone with GCC 4.5- will undo it in some weeks or months.

[gilboa]

That solution will not work. The "4fv" part at the end of glUniform4fv means it expects to get a pointer to 4 floats, so by copying only the one float to a temporary you solve the compiler error, but introduce a bug. (The other three floats being garbage.)

You'll need to copy the whole float4 to a temporary and then pass in the address of the x component of that temporary. That should probably get rid of the error and not introduce a bug

Thanks.
I wondered if the count = 1 meant 1 float or one float block.
Side question: How many times is function being called; I'd hate to add useless-compiler-warning-killing memcpy to function that's being called 1,000,000 times a second.

- Gilboa

[gilboa]

hoijui,

Nice catch :). I'm indeed a C programmer.
I wonder if declaring parameter within the adjacent code block won't trigger the same compile warning.

One more thing, were do I post the patch so it can be picked by, well, you guys?

- Gilboa

[hoijui]

the preferred way to submit a patch is, to fork spring on guithub, commit your patch there, and tell us to merge it into master, either by a forum post, a PM, or by using githubs pull request.
to be able to fork spring, you need to have an account on github. then go here:
https://github.com/spring/spring
and use the [Fork] button (3rd line of links/buttons).
then commit your patch (to git@github.com:gilboa/spring.git).
if you are new to git/github, theres lots of good info here:
http://help.github.com/
or just ask for help about specific stuff in the spring dev channel in the lobby. everyone loves to give git help.

if that is too much work for you/you do not plan to make other patches... as this is a relatively simple thing, just tell us to do it/send a patch file (based on a recent master commit) in here.

[gilboa]

Hi,

Sorry for the late reply.
It seems that gcc breaks spring in a number of places. (missing includes, local variables, etc).
I'll create a mega patch and continue from there.

- Gilboa

[hoijui]

ok.
remember to make multiple commits, if stuff you fix is relatively independent/it makes sense.
good luck! and thanks!

[gilboa]

Hello,

Please ACK these patches.
(I tried making them as non-intrusive as I could)

GCC 4.6 include fix.

Code: Select all

--- AI/Skirmish/E323AI/AAStar.h.old	2011-03-10 05:31:35.558283029 +0200
+++ AI/Skirmish/E323AI/AAStar.h	2011-03-10 05:31:42.333474485 +0200
@@ -1,6 +1,6 @@
 #ifndef E323ASTAR_H
 #define E323ASTAR_H
-
+#include <cstdio>
 #include <queue>
 #include <vector>
 #include <list>
--- rts/Rendering/GLContext.cpp.old	2011-03-16 15:36:46.001675256 +0200
+++ rts/Rendering/GLContext.cpp	2011-03-16 15:36:56.443566867 +0200
@@ -7,7 +7,7 @@
 #endif
 
 #include "GLContext.h"
-
+#include <cstdio>
 #include <list>
 
 
--- ./rts/System/MemPool.h.old	2011-03-28 17:27:09.515598615 +0200
+++ ./rts/System/MemPool.h	2011-03-28 17:33:48.916600171 +0200
@@ -3,6 +3,7 @@
 #ifndef _MEM_POOL_H_
 #define _MEM_POOL_H_
 
+#include <cstdio>
 #include <new>
 
 const size_t MAX_MEM_SIZE=200;

GCC 4.6 temporary parameter fix.

Code: Select all

--- rts/Map/SMF/BFGroundDrawer.cpp.old	2011-03-28 12:51:56.331599362 +0200
+++ rts/Map/SMF/BFGroundDrawer.cpp	2011-03-28 12:54:43.209600038 +0200
@@ -1342,7 +1342,23 @@
 			smfShaderGLSL->Enable();
 			smfShaderGLSL->SetUniform3fv(10, &camera->pos[0]);
 			smfShaderGLSL->SetUniformMatrix4fv(12, false, &shadowHandler->shadowMatrix.m[0]);
-			smfShaderGLSL->SetUniform4fv(13, const_cast<float*>(&(shadowHandler->GetShadowParams().x)));
+
+			{
+				/* 
+				 * GCC 4.6 barfs on having a temporary copy of float4
+				 *   passed as pointer to glUniform4fv.
+				 * Pending a cleaner solution that will include
+				 *   changes to shadowHandler and float4,
+				 *   the only thing remaining is to manually copy
+				 *   the members to temporary stack array and pass
+				 *   it down to glUniform4fv.
+				 */
+				float4 shadowFloat4 =
+						shadowHandler->GetShadowParams();
+				float shadowFloatArr[4];
+	
+				smfShaderGLSL->SetUniform4fv(13, shadowFloat4.as_float_arr(shadowFloatArr));
+			}
 
 			glActiveTexture(GL_TEXTURE5); glBindTexture(GL_TEXTURE_2D, map->GetNormalsTexture());
 			glActiveTexture(GL_TEXTURE6); glBindTexture(GL_TEXTURE_2D, map->GetSpecularTexture());
--- rts/Rendering/Env/GrassDrawer.cpp.old	2011-03-28 14:29:32.521597516 +0200
+++ rts/Rendering/Env/GrassDrawer.cpp	2011-03-28 14:33:18.881600104 +0200
@@ -485,7 +485,24 @@
 
 		if (globalRendering->haveGLSL) {
 			grassShader->SetUniformMatrix4fv(6, false, &shadowHandler->shadowMatrix.m[0]);
-			grassShader->SetUniform4fv(7, const_cast<float*>(&(shadowHandler->GetShadowParams().x)));
+
+			{
+				/* 
+				 * GCC 4.6 barfs on having a temporary copy of float4
+				 *   passed as pointer to glUniform4fv.
+				 * Pending a cleaner solution that will include
+				 *   changes to shadowHandler and float4,
+				 *   the only thing remaining is to manually copy
+				 *   the members to temporary stack array and pass
+				 *   it down to glUniform4fv.
+				 */
+				float4 shadowFloat4 =
+						shadowHandler->GetShadowParams();
+				float shadowFloatArr[4];
+
+				grassShader->SetUniform4fv(7, shadowFloat4.as_float_arr(shadowFloatArr));
+			}
+
 			grassShader->SetUniform1f(8, gs->frameNum);
 			grassShader->SetUniform3fv(9, const_cast<float*>(&(windSpeed.x)));
 		} else {
@@ -585,7 +607,24 @@
 
 		if (globalRendering->haveGLSL) {
 			grassShader->SetUniformMatrix4fv(6, false, &shadowHandler->shadowMatrix.m[0]);
-			grassShader->SetUniform4fv(7, const_cast<float*>(&(shadowHandler->GetShadowParams().x)));
+
+			{
+				/* 
+				 * GCC 4.6 barfs on having a temporary copy of float4
+				 *   passed as pointer to glUniform4fv.
+				 * Pending a cleaner solution that will include
+				 *   changes to shadowHandler and float4,
+				 *   the only thing remaining is to manually copy
+				 *   the members to temporary stack array and pass
+				 *   it down to glUniform4fv.
+				 */
+				float4 shadowFloat4 =
+						shadowHandler->GetShadowParams();
+				float shadowFloatArr[4];
+
+				grassShader->SetUniform4fv(7, shadowFloat4.as_float_arr(shadowFloatArr));
+			}
+
 			grassShader->SetUniform1f(8, gs->frameNum);
 			grassShader->SetUniform3fv(9, const_cast<float*>(&(windSpeed.x)));
 		}
--- rts/Rendering/Env/AdvTreeDrawer.cpp.old	2011-03-28 14:29:47.511601193 +0200
+++ rts/Rendering/Env/AdvTreeDrawer.cpp	2011-03-28 14:35:20.958600022 +0200
@@ -458,7 +458,22 @@
 
 		if (globalRendering->haveGLSL) {
 			treeShader->SetUniformMatrix4fv(7, false, &shadowHandler->shadowMatrix.m[0]);
-			treeShader->SetUniform4fv(8, const_cast<float*>(&(shadowHandler->GetShadowParams().x)));
+			{
+				/* 
+				 * GCC 4.6 barfs on having a temporary copy of float4
+				 *   passed as pointer to glUniform4fv.
+				 * Pending a cleaner solution that will include
+				 *   changes to shadowHandler and float4,
+				 *   the only thing remaining is to manually copy
+				 *   the members to temporary stack array and pass
+				 *   it down to glUniform4fv.
+				 */
+				float4 shadowFloat4 =
+						shadowHandler->GetShadowParams();
+				float shadowFloatArr[4];
+
+				treeShader->SetUniform4fv(8, shadowFloat4.as_float_arr(shadowFloatArr));
+			}
 		} else {
 			treeShader->SetUniformTarget(GL_FRAGMENT_PROGRAM_ARB);
 			treeShader->SetUniform4f(10, L.groundAmbientColor.x, L.groundAmbientColor.y, L.groundAmbientColor.z, 1.0f);
@@ -501,7 +521,23 @@
 
 			if (globalRendering->haveGLSL) {
 				treeShader->SetUniformMatrix4fv(7, false, &shadowHandler->shadowMatrix.m[0]);
-				treeShader->SetUniform4fv(8, const_cast<float*>(&(shadowHandler->GetShadowParams().x)));
+
+				{
+					/* 
+					 * GCC 4.6 barfs on having a temporary copy of float4
+					 *   passed as pointer to glUniform4fv.
+					 * Pending a cleaner solution that will include
+					 *   changes to shadowHandler and float4,
+					 *   the only thing remaining is to manually copy
+					 *   the members to temporary stack array and pass
+					 *   it down to glUniform4fv.
+					 */
+					float4 shadowFloat4 =
+							shadowHandler->GetShadowParams();
+					float shadowFloatArr[4];
+
+					treeShader->SetUniform4fv(8, shadowFloat4.as_float_arr(shadowFloatArr));
+				}
 			}
 
 			glActiveTexture(GL_TEXTURE1);
--- rts/Lua/LuaMaterial.cpp.old	2011-03-16 11:34:10.123708670 +0200
+++ rts/Lua/LuaMaterial.cpp	2011-03-16 14:44:23.736824894 +0200
@@ -379,7 +379,20 @@
 	}
 
 	if (shadowParamsLoc >= 0) {
-		glUniform4fv(shadowParamsLoc, 1, const_cast<float*>(&(shadowHandler->GetShadowParams().x)));
+		/* 
+		 * GCC 4.6 barfs on having a temporary copy of float4
+		 *   passed as pointer to glUniform4fv.
+		 * Pending a cleaner solution that will include
+		 *   changes to shadowHandler and float4,
+		 *   the only thing remaining is to manually copy
+		 *   the members to temporary stack array and pass
+		 *   it down to glUniform4fv.
+		 */
+		float4 shadowFloat4 =
+				shadowHandler->GetShadowParams();
+		float shadowFloatArr[4];
+
+		glUniform4fv(shadowParamsLoc, 1, shadowFloat4.as_float_arr(shadowFloatArr));
 	}
 
 	const int maxTex = std::max(texCount, prev.texCount);
--- rts/Rendering/UnitDrawer.cpp.old	2011-03-28 14:11:57.129599851 +0200
+++ rts/Rendering/UnitDrawer.cpp	2011-03-28 14:14:01.937599970 +0200
@@ -1195,7 +1195,23 @@
 			modelShaders[MODEL_SHADER_S3O_ACTIVE]->SetUniformMatrix4dv(7, false, const_cast<double*>(camera->GetViewMat()));
 			modelShaders[MODEL_SHADER_S3O_ACTIVE]->SetUniformMatrix4dv(8, false, const_cast<double*>(camera->GetViewMatInv()));
 			modelShaders[MODEL_SHADER_S3O_ACTIVE]->SetUniformMatrix4fv(13, false, &shadowHandler->shadowMatrix.m[0]);
-			modelShaders[MODEL_SHADER_S3O_ACTIVE]->SetUniform4fv(14, const_cast<float*>(&(shadowHandler->GetShadowParams().x)));
+
+			{
+				/* 
+				 * GCC 4.6 barfs on having a temporary copy of float4
+				 *   passed as pointer to glUniform4fv.
+				 * Pending a cleaner solution that will include
+				 *   changes to shadowHandler and float4,
+				 *   the only thing remaining is to manually copy
+				 *   the members to temporary stack array and pass
+				 *   it down to glUniform4fv.
+				 */
+				float4 shadowFloat4 =
+						shadowHandler->GetShadowParams();
+				float shadowFloatArr[4];
+
+				modelShaders[MODEL_SHADER_S3O_ACTIVE]->SetUniform4fv(14, shadowFloat4.as_float_arr(shadowFloatArr));
+			}
 		} else {
 			modelShaders[MODEL_SHADER_S3O_ACTIVE]->SetUniformTarget(GL_VERTEX_PROGRAM_ARB);
 			modelShaders[MODEL_SHADER_S3O_ACTIVE]->SetUniform4f(10, mapInfo->light.sunDir.x, mapInfo->light.sunDir.y ,mapInfo->light.sunDir.z, 0.0f);
--- rts/System/float4.h.old	2011-03-31 06:13:26.908050146 +0200
+++ rts/System/float4.h	2011-03-31 06:16:38.697117765 +0200
@@ -54,6 +54,13 @@
 		return !(*this == f);
 	}
 
+	inline float *as_float_arr(float *floatArr) {
+		floatArr[0] = x;
+		floatArr[1] = y;
+		floatArr[2] = z;
+		floatArr[3] = w;
+		return floatArr;
+	}
 
 	/// Allows implicit conversion to float* (for passing to gl functions)
 	operator const float* () const { return &x; }

[gilboa]

I play tested it for a couple of minutes (@4am) and it seems to be working OK.

Never the less, as I'm not a GL expert (far from it), please OK the changes before I push them to F15.

- Gilboa

[Kloot]

Those changes are still much more invasive than necessary. Fix the problem at its source (as has been done in master) instead:

Code: Select all

index 6618b17..0cb6254 100644
--- a/rts/Rendering/ShadowHandler.cpp
+++ b/rts/Rendering/ShadowHandler.cpp
@@ -318,6 +318,11 @@ void CShadowHandler::CreateShadows(void)
        xmid = 1.0f - (sqrt(fabs(x2)) / (sqrt(fabs(x2)) + sqrt(fabs(x1))));
        ymid = 1.0f - (sqrt(fabs(y2)) / (sqrt(fabs(y2)) + sqrt(fabs(y1))));

+       shadowParams.x = xmid;
+       shadowParams.y = ymid;
+       shadowParams.z = p17;
+       shadowParams.w = p18;
+
        shadowMatrix[ 0] =   cross1.x / maxLengthX;
        shadowMatrix[ 4] =   cross1.y / maxLengthX;
        shadowMatrix[ 8] =   cross1.z / maxLengthX;
diff --git a/rts/Rendering/ShadowHandler.h b/rts/Rendering/ShadowHandler.h
index e587859..6f22ba8 100644
--- a/rts/Rendering/ShadowHandler.h
+++ b/rts/Rendering/ShadowHandler.h
@@ -38,7 +38,7 @@ public:
        CMatrix44f shadowMatrix;
        void CalcMinMaxView(void);

-       const float4 GetShadowParams() const { return float4(xmid, ymid, p17, p18); }
+       const float4& GetShadowParams() const { return shadowParams; }

        enum ShadowGenProgram {
                SHADOWGEN_PROGRAM_MODEL      = 0,
@@ -76,6 +76,7 @@ protected:
        //! to write the (FBO) depth-buffer texture
        std::vector<Shader::IProgramObject*> shadowGenProgs;

+       float4 shadowParams;
        float x1, x2, y1, y2;
        float xmid, ymid;
        float p17, p18;

[gilboa]

Those changes are still much more invasive than necessary. Fix the problem at its source (as has been done in master) instead:

Won't work.
As far as I could test, GCC 4.6 (at least when using the default Fedora 15 configuration) barfs on two things:
1. Automatic conversion of float4 to float *.
2. Having a non-local variable (read: not in stack and/or heap) being passed as pointer to a functions.

As far as I could test, I -must- have a local copy of float[4] in the current function's stack, or I trigger a "-fno-permissive" GCC error.

- Gilboa

[gilboa]

Strike the above:
I've re-based the fix above against 0.82.7.1 (latest stable) and thus far it seem to build just fine.

In short, it seems that only fix required by GCC 4.6 is the missing includes listed above.

I'll push it as update for F15-Beta ASAP.

Thanks!
- Gilboa

[Macbetht]

this should be a pretty straightforward issue -- I'm trying to compile Lua (or rather lua-vec, which is a minor variant) on a CentOS Linux install, and I get the following error:

[jt@flyboy src]#make linux
make all MYCFLAGS=-DLUA_USE_LINUX MYLIBS="-Wl,-E -ldl -lreadline -lhistory -lncurses"
make[1]: Entering directory `/jt/flyboy/fly/lua/lua-vec/src'
gcc -o lua lua.o liblua.a -lm -Wl,-E -ldl -lreadline -lhistory -lncurses
/usr/bin/ld: cannot find -lreadline
collect2: ld returned 1 exit status
make[1]: *** [lua] Error 1

That would suggest the readline lib is not installed. But...

[jt@flyboy src]#ls /usr/lib/libreadline*
/usr/lib/libreadline.so.5 /usr/lib/libreadline.so.5.1

Interestingly, if I rearrange the order of readline/history/ncurses, whichever is first triggers the same error, so I suspect that this is some sort of a folder-specification problem, not a missing library problem.

Any ideas?

yum install readline-devel.x86_64 readline-devel.i386 ncurses-devel.i386 ncurses-devel.x86_64

seems to have done the trick! The odd thing is I have compiled this before without these libs... but enough time pondering life's mysteries...