gcc/Linux update #95

z33ky · 2021-01-27T09:34:59Z

Fixes to build for Linux.

Does this PR close any issues?

No.

PR Checklist

My PR follows all guidelines in the CONTRIBUTING.md file
My PR targets a develop branch OR targets another branch with a specific goal in mind

z33ky · 2021-01-27T09:39:26Z

sp/src/game/client/c_baselesson.cpp

@@ -1039,40 +1040,40 @@ Vector CIconLesson::GetIconTargetPosition( C_BaseEntity *pIconTarget )

 #define LESSON_VARIABLE_INIT_SYMBOL( _varEnum, _varName, _varType ) g_n##_varEnum##Symbol = KeyValuesSystem()->GetSymbolForString( #_varEnum );

-#define LESSON_SCRIPT_STRING_ADD_TO_MAP( _varEnum, _varName, _varType ) g_NameToTypeMap.Insert( #_varEnum, LESSON_VARIABLE_##_varEnum## );


A note to the changes here: The ## operator is for creating a singular token. In this instance, for example, pasting LESSON_VARIABLE_ with _varEnum with ); will never create a valid singular token. Here three tokens are to be created: LESSON_VARIABLE_##_varEnum ) ;.
gcc is more strict than MSVC here in that it rejects the operation if pasting fails.

z33ky · 2021-01-27T09:41:21Z

sp/src/public/vscript/ivscript.h

-#ifdef MAPBASE_VSCRIPT
-template <typename T> T *HScriptToClass( HSCRIPT hObj )
-{
-	return (hObj) ? (T*)g_pScriptVM->GetInstanceValue( hObj, GetScriptDesc( (T*)NULL ) ) : NULL;


IScriptVM is not yet defined here. MSVC probably accepts this as long as the template is not expanded yet.
Moving it below the definition of IScriptVM seemed to cause no harm and fixes the gcc build.

z33ky · 2021-01-27T09:43:07Z

sp/src/public/vscript/ivscript.h

@@ -645,8 +631,21 @@ struct ScriptEnumDesc_t
 // 
 //-----------------------------------------------------------------------------

+// forwards T (and T&) if T is neither enum or an unsigned integer


gcc complained that it can't discern how to create a ScriptVariant_t from enums and unsigned integers.
This awkward template ensures it can deal with it properly via an implicit cast to int due to the overload below.

You'll still need std::forward<T> on this if your expecting to forward it.

Indeed, and fix the return-type from T to T&&.

z33ky · 2021-01-27T09:46:35Z

sp/src/vscript/vscript_squirrel.cpp

@@ -827,7 +829,7 @@ void PushVariant(HSQUIRRELVM vm, const ScriptVariant_t& value)
 		sq_createinstance(vm, -1);
 		SQUserPointer p;
 		sq_getinstanceup(vm, -1, &p, 0);
-		new(p) Vector(value);
+		new(p) Vector(static_cast<const Vector&>(value));


I find this one interesting.
Vector() has an overload for vec_t (i.e. float) and ScriptVariant_t can be cast to both Vector& and float. gcc regards this as an unresolvable ambiguity. I guess MSVC doesn't, and I'm guessing a cast to Vector& is what's desired here.

Ya, this cast is desirable, unfortunately MSVC doesn't conform with the standard for these conversions and allows implicit conversions here. (Compiling with conforming options on MSVC will normally pick these up)

z33ky · 2021-01-27T09:53:26Z

sp/src/game/client/c_baselesson.cpp

@@ -666,7 +666,8 @@ void CIconLesson::UpdateInactive()
 			CUtlBuffer msg_data;
 			msg_data.PutChar( 1 );
 			msg_data.PutString( m_szHudHint.String() );
-			usermessages->DispatchUserMessage( usermessages->LookupUserMessage( "KeyHintText" ), bf_read( msg_data.Base(), msg_data.TellPut() ) );


The issue here is that DispatchUserMessage() expects a (non-const) bf_read&. rvalues (unnamed temporaries) are normally not allowed to be used as non-const references (const would be fine). MSVC seems to allow it anyways. By using an explicit variable, it is no longer an rvalue and thus can be passed as non-const reference.

While looking into this I read the implementation of DispatchUserMessage(). For every hook it copies the bf_read, which seems unnecessary to me, considering there's a Reset() function on it. So if you wanted to you can probably optimize it to avoid making the copies and instead reset it before invoking each hook.

reductor · 2021-01-27T10:47:04Z

sp/src/vscript/vscript_squirrel.cpp

-SQUserPointer TYPETAG_VECTOR = "VectorTypeTag";
+// we cast away constness here, but this looks to be an API deficit of squrrel,
+// since the typetag shouldn't be modified
+static SQUserPointer TYPETAG_VECTOR = const_cast<char*>("VectorTypeTag");


Instead of doing this should be able to just make it a char array

Yes, that works.
I didn't know that it's fine to initialize a char* from a string literal (while apparently for void* it isn't).

reductor · 2021-01-27T11:48:46Z

sp/src/devtools/makefile_base_posix.mak

-$(MAKEFILE_LINK): $(shell which $(CC)) $(THISFILE)
-	if [ "$(shell printf "$(shell $(CC) -dumpversion)\n8" | sort -Vr | head -1)" = 8 ]; then \
-		$(COMPILE.cpp) -o gcc9+support.o gcc9+support.c ;\
+$(MAKEFILE_LINK): $(shell which $(CXX)) $(THISFILE)


The dependency on CXX seems a bit unusual here, it should probably depend on the gcc9+support.cpp file here also.

The thought behind this is that this check will be rerun when the system(/compiler) is updated. So if previously it was on gcc 7 it will then switch to the gcc8+ Makefile. I can add a comment for that.
It should also depend on the gcc9+support.cpp file, yeah.

Blixibon · 2021-01-27T17:23:13Z

@z33ky Would you say that this PR is ready to merge?

There were a few big changes committed to develop today, so I would recommend making sure these fixes still apply.

z33ky · 2021-01-27T18:16:29Z

I rebased locally and there are no conflicts or new warnings, so yeah, all good :)

Blixibon · 2021-01-28T13:31:20Z

The code in this PR currently cannot be compiled with Visual Studio 2013 on Windows due to the compiler not supporting keywords like constexpr.

Visual Studio 2013 is the default compiler for Source SDK 2013 and Mapbase does not currently change that. I am aware that upgrading the SDK to a newer tool-set is possible, but I'm relatively hesitant to subject users to a new requirement, especially right now when we're about to release an update.

Could the involved code perhaps be #if'd according to _MSC_VER and/or the actual GCC/Linux pre-processors?

z33ky · 2021-01-28T16:09:20Z

There are only two occurrences of constexpr, both for ToConstantVariant() in sp/src/public/vscript/ivscript.h . They can both be removed without any practical difference. If you could check that this is the only piece missing for MSVS 2013 support I'd only change that.
Otherwise #ifdefing is also possible.

z33ky · 2021-02-17T12:06:40Z

bump

Blixibon · 2021-03-10T19:59:00Z

I'm terribly sorry for how long it's taken me to get to this. I went through a major natural disaster in the middle of February and it took me some time to get my life back in order.

Aside from what I reported, these two #includes in ivscript.h are the only other lines of code causing compile errors for me in VS2013:

#include <type_traits>
#include <utility>

3>c:\program files (x86)\microsoft visual studio 12.0\vc\include\crtdbg.h(264): error C2220: warning treated as error - no 'object' file generated (vscript_bindings_math.cpp)
3>c:\program files (x86)\microsoft visual studio 12.0\vc\include\crtdbg.h(264): warning C4005: '_malloc_dbg' : macro redefinition (vscript_bindings_math.cpp)
3>          e:\mapbase\mapbase-hl2-master\sp\src\public\tier0\memdbgon.h(176) : see previous definition of '_malloc_dbg'

However, be warned that #105 will cause a few merge conflicts with these changes and it may be merged with develop before this one. They're mostly just from code that was moved or deleted, but I can resolve them myself if needed.

z33ky · 2021-03-18T00:26:04Z

I'm terribly sorry for how long it's taken me to get to this. I went through a major natural disaster in the middle of February and it took me some time to get my life back in order.

No worries, take it easy then.

Aside from what I reported, these two #includes in ivscript.h are the only other lines of code causing compile errors for me in VS2013:
[...]

Ah, that's just an incorrect include ordering I think. The two files I added must be included before memdbgon.h, not after.

For the error you mentioned previously, did you try if it works if you remove the constexpr? As I said, it shouldn't make any practical difference there anyways.

However, be warned that #105 will cause a few merge conflicts with these changes and it may be merged with develop before this one. They're mostly just from code that was moved or deleted, but I can resolve them myself if needed.

I think I can get that done on the weekend.

z33ky · 2021-03-19T16:20:17Z

Ah, that's just an incorrect include ordering I think. The two files I added must be included before memdbgon.h, not after.

For the error you mentioned previously, did you try if it works if you remove the constexpr? As I said, it shouldn't make any practical difference there anyways.

I've included these changes in an attempt to fix MSVC compilation while rebasing my changes onto the latest develop.

The button mask is created by shifting a bit according to the MouseCode, which is just a renamed ButtonCode_t. Mouse buttons start at 107, which is way out of range for our ints. To fix this, introduce MouseButtonBit(), which checks if a button code corresponds to a mouse button at all and returns a usable bitmask for the corresponding mouse button code. This is then used for the button mask.

z33ky · 2021-03-20T18:29:19Z

I tested the VScript implementation using this rather trivial script:

function GetPlayerOrigin()
{
	local player = Entities.FindByClassname(null, "player")

	if (player != null)
	{
		printl(format("found player %s", player.GetPlayerName()))
		local pos = player.GetOrigin()
		printl(format("Player origin: %.2f %.2f %.2f", pos.x, pos.y, pos.z))
	}
	else
	{
		printl("unable to find player")
	}
}

GetPlayerOrigin()

I invoked it via

] ent_fire !self RunScriptFile test.nut
found player z33ky
Player origin: 771.79 -1022.78 64.03
] getpos
setpos 771.786377 -1022.781067 128.031250;setang 16.460728 -83.867760 0.000000

The Z value looks suspicious. Any idea? At least it doesn't crash anymore :P

If you have a better (more complex) testcase I can try let me know and I'll test.

Btw the links on https://github.com/mapbase-source/source-sdk-2013/wiki/VScript-in-Mapbase#tutorials are broken due to erroneous semicolons.

z33ky · 2021-03-20T18:33:20Z

The Z value looks suspicious. Any idea?

getpos returns the view-position by default... so it's fine.

] ent_fire !self RunScriptFile test.nut
found player z33ky
Player origin: 1244.79 -803.04 125.38
] getpos 2
setpos_exact 1244.790771 -803.041016 125.381271;setang_exact 57.767498 94.879036 0.000000

Blixibon · 2021-04-07T16:30:28Z

I finally got around to testing this and it seems to compile and function as intended. Assuming you have no objections, I will merge this later today

z33ky · 2021-04-07T16:41:45Z

Cool :) Yeah, it's ready from my side.

gcc/Linux update

z33ky commented Jan 27, 2021

View reviewed changes

reductor reviewed Jan 27, 2021

View reviewed changes

z33ky force-pushed the gcc-linux branch from 62a167c to e83eb88 Compare January 27, 2021 13:22

z33ky force-pushed the gcc-linux branch from e83eb88 to 8eaeecf Compare January 27, 2021 22:32

z33ky force-pushed the gcc-linux branch from 8eaeecf to d626eae Compare March 19, 2021 16:19

z33ky added 3 commits March 19, 2021 17:41

Fix gcc build errors & warnings

49836ab

Fix gcc9+support.o compilation

a5fb07d

Specify gcc9+support.cpp dependency for gcc8+ Makefiles

718186f

z33ky force-pushed the gcc-linux branch from d626eae to 718186f Compare March 19, 2021 16:41

z33ky added 3 commits March 20, 2021 13:58

Fix stdshaders compilation on Linux

761f065

Fix/Implement VScript FFI for GCC's virtual member functions

28e87ce

z33ky mentioned this pull request Mar 20, 2021

mapbase-source sync & small fixes FIREFIGHT-RELOADED/FIREFIGHT-RELOADED-src-sdk-2013#21

Closed

Blixibon merged commit d431158 into mapbase-source:develop Apr 7, 2021

VDeltaGabriel pushed a commit to Project-P2009/p2009_sdk that referenced this pull request Apr 27, 2023

Merge pull request mapbase-source#95 from z33ky/gcc-linux

bb2ec04

gcc/Linux update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcc/Linux update #95

gcc/Linux update #95

z33ky commented Jan 27, 2021

z33ky Jan 27, 2021

z33ky Jan 27, 2021

z33ky Jan 27, 2021

reductor Jan 27, 2021

z33ky Jan 27, 2021

z33ky Jan 27, 2021

reductor Jan 27, 2021

z33ky Jan 27, 2021

reductor Jan 27, 2021

z33ky Jan 27, 2021

reductor Jan 27, 2021

z33ky Jan 27, 2021

Blixibon commented Jan 27, 2021

z33ky commented Jan 27, 2021

Blixibon commented Jan 28, 2021

z33ky commented Jan 28, 2021

z33ky commented Feb 17, 2021

Blixibon commented Mar 10, 2021

z33ky commented Mar 18, 2021

z33ky commented Mar 19, 2021

z33ky commented Mar 20, 2021

z33ky commented Mar 20, 2021

Blixibon commented Apr 7, 2021

z33ky commented Apr 7, 2021

		@@ -1039,40 +1040,40 @@ Vector CIconLesson::GetIconTargetPosition( C_BaseEntity *pIconTarget )

		#define LESSON_VARIABLE_INIT_SYMBOL( _varEnum, _varName, _varType ) g_n##_varEnum##Symbol = KeyValuesSystem()->GetSymbolForString( #_varEnum );

		#define LESSON_SCRIPT_STRING_ADD_TO_MAP( _varEnum, _varName, _varType ) g_NameToTypeMap.Insert( #_varEnum, LESSON_VARIABLE_##_varEnum## );

gcc/Linux update #95

gcc/Linux update #95

Conversation

z33ky commented Jan 27, 2021

Does this PR close any issues?

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blixibon commented Jan 27, 2021

z33ky commented Jan 27, 2021

Blixibon commented Jan 28, 2021

z33ky commented Jan 28, 2021

z33ky commented Feb 17, 2021

Blixibon commented Mar 10, 2021

z33ky commented Mar 18, 2021

z33ky commented Mar 19, 2021

z33ky commented Mar 20, 2021

z33ky commented Mar 20, 2021

Blixibon commented Apr 7, 2021

z33ky commented Apr 7, 2021