From f238f0ba5ca520cbf9a6a96b49bb00623993ab64 Mon Sep 17 00:00:00 2001 From: Christoph Oelckers Date: Sun, 13 Nov 2016 12:02:41 +0100 Subject: [PATCH] - try to preserve a bit more information about incorrect use of user variables to print more meaningful error messages. This is not complete yet and will need integration with the previous commit. --- src/dobjtype.cpp | 1 + src/dobjtype.h | 1 + src/p_actionfunctions.cpp | 10 ++++ src/p_pspr.cpp | 9 ++++ src/sc_man.cpp | 11 +++-- src/sc_man.h | 4 +- src/scripting/codegeneration/codegen.cpp | 2 +- src/scripting/codegeneration/codegen.h | 1 + src/scripting/decorate/thingdef_parse.cpp | 1 + src/scripting/thingdef.cpp | 58 +++++++++++++++++++++++ src/scripting/vm/vm.h | 1 + 11 files changed, 94 insertions(+), 5 deletions(-) diff --git a/src/dobjtype.cpp b/src/dobjtype.cpp index c2b1008df9..f3fda2b147 100644 --- a/src/dobjtype.cpp +++ b/src/dobjtype.cpp @@ -2791,6 +2791,7 @@ PClass::PClass() Defaults = nullptr; bRuntimeClass = false; bExported = false; + bDecorateClass = false; ConstructNative = nullptr; mDescriptiveName = "Class"; diff --git a/src/dobjtype.h b/src/dobjtype.h index be15193a4d..e1bfd07968 100644 --- a/src/dobjtype.h +++ b/src/dobjtype.h @@ -775,6 +775,7 @@ public: BYTE *Defaults; bool bRuntimeClass; // class was defined at run-time, not compile-time bool bExported; // This type has been declared in a script + bool bDecorateClass; // may be subject to some idiosyncracies due to DECORATE backwards compatibility TArray Virtuals; // virtual function table void (*ConstructNative)(void *); diff --git a/src/p_actionfunctions.cpp b/src/p_actionfunctions.cpp index a99ba9ab61..74d828d550 100644 --- a/src/p_actionfunctions.cpp +++ b/src/p_actionfunctions.cpp @@ -78,6 +78,7 @@ #include "p_spec.h" #include "templates.h" #include "vm.h" +#include "v_text.h" #include "thingdef.h" #include "math/cmath.h" @@ -132,6 +133,15 @@ bool ACustomInventory::CallStateChain (AActor *actor, FState *state) if (state->ActionFunc != NULL) { + if (state->ActionFunc->Unsafe) + { + // If an unsafe function (i.e. one that accesses user variables) is being detected, print a warning once and remove the bogus function. We may not call it because that would inevitably crash. + auto owner = FState::StaticFindStateOwner(state); + Printf(TEXTCOLOR_RED "Unsafe state call in state %s.%d to %s which accesses user variables. The action function has been removed from this state\n", + owner->TypeName.GetChars(), state - owner->OwnedStates, static_cast(state->ActionFunc)->PrintableName.GetChars()); + state->ActionFunc = nullptr; + } + VMFrameStack stack; PPrototype *proto = state->ActionFunc->Proto; VMReturn *wantret; diff --git a/src/p_pspr.cpp b/src/p_pspr.cpp index d279a180cc..56562ab219 100644 --- a/src/p_pspr.cpp +++ b/src/p_pspr.cpp @@ -29,6 +29,7 @@ #include "g_level.h" #include "d_player.h" #include "serializer.h" +#include "v_text.h" // MACROS ------------------------------------------------------------------ @@ -337,6 +338,14 @@ void DPSprite::SetState(FState *newstate, bool pending) { FState *nextstate; FStateParamInfo stp = { newstate, STATE_Psprite, ID }; + if (newstate->ActionFunc != nullptr && newstate->ActionFunc->Unsafe) + { + // If an unsafe function (i.e. one that accesses user variables) is being detected, print a warning once and remove the bogus function. We may not call it because that would inevitably crash. + auto owner = FState::StaticFindStateOwner(newstate); + Printf(TEXTCOLOR_RED "Unsafe state call in state %s.%d to %s which accesses user variables. The action function has been removed from this state\n", + owner->TypeName.GetChars(), newstate - owner->OwnedStates, static_cast(newstate->ActionFunc)->PrintableName.GetChars()); + newstate->ActionFunc = nullptr; + } if (newstate->CallAction(Owner->mo, Caller, &stp, &nextstate)) { // It's possible this call resulted in this very layer being replaced. diff --git a/src/sc_man.cpp b/src/sc_man.cpp index f48d6b4612..254caed625 100644 --- a/src/sc_man.cpp +++ b/src/sc_man.cpp @@ -1046,7 +1046,10 @@ void FScriptPosition::Message (int severity, const char *message, ...) const { FString composed; - if ((severity == MSG_DEBUG || severity == MSG_DEBUGLOG) && developer < DMSG_NOTIFY) return; + if (severity == MSG_DEBUGLOG && developer < DMSG_NOTIFY) return; + if (severity == MSG_DEBUGERROR && developer < DMSG_ERROR) return; + if (severity == MSG_DEBUGWARN && developer < DMSG_WARNING) return; + if (severity == MSG_DEBUGMSG && developer < DMSG_NOTIFY) return; if (severity == MSG_OPTERROR) { severity = StrictErrors || strictdecorate ? MSG_ERROR : MSG_WARNING; @@ -1073,9 +1076,11 @@ void FScriptPosition::Message (int severity, const char *message, ...) const return; case MSG_WARNING: + case MSG_DEBUGWARN: + case MSG_DEBUGERROR: // This is intentionally not being printed as an 'error', the difference to MSG_DEBUGWARN is only the severity level at which it gets triggered. WarnCounter++; type = "warning"; - color = TEXTCOLOR_YELLOW; + color = TEXTCOLOR_ORANGE; break; case MSG_ERROR: @@ -1085,7 +1090,7 @@ void FScriptPosition::Message (int severity, const char *message, ...) const break; case MSG_MESSAGE: - case MSG_DEBUG: + case MSG_DEBUGMSG: type = "message"; color = TEXTCOLOR_GREEN; break; diff --git a/src/sc_man.h b/src/sc_man.h index 5eeacd8924..fde6efbed3 100644 --- a/src/sc_man.h +++ b/src/sc_man.h @@ -125,7 +125,9 @@ enum MSG_FATAL, MSG_ERROR, MSG_OPTERROR, - MSG_DEBUG, + MSG_DEBUGERROR, + MSG_DEBUGWARN, + MSG_DEBUGMSG, MSG_LOG, MSG_DEBUGLOG, MSG_MESSAGE diff --git a/src/scripting/codegeneration/codegen.cpp b/src/scripting/codegeneration/codegen.cpp index f59a2753df..2ce1b42651 100644 --- a/src/scripting/codegeneration/codegen.cpp +++ b/src/scripting/codegeneration/codegen.cpp @@ -8073,7 +8073,7 @@ FxExpression *FxClassTypeCast::Resolve(FCompileContext &ctx) delete this; return nullptr; } - ScriptPosition.Message(MSG_DEBUG, "resolving '%s' as class name", clsname.GetChars()); + ScriptPosition.Message(MSG_DEBUGLOG, "resolving '%s' as class name", clsname.GetChars()); } } FxExpression *x = new FxConstant(cls, to, ScriptPosition); diff --git a/src/scripting/codegeneration/codegen.h b/src/scripting/codegeneration/codegen.h index 0f937b0cdc..b9679ad31d 100644 --- a/src/scripting/codegeneration/codegen.h +++ b/src/scripting/codegeneration/codegen.h @@ -79,6 +79,7 @@ struct FCompileContext int StateIndex; // index in actor's state table for anonymous functions, otherwise -1 (not used by DECORATE which pre-resolves state indices) int StateCount; // amount of states an anoymous function is being used on (must be 1 for state indices to be allowed.) int Lump; + bool Unsafe = false; TDeletingArray FunctionArgs; FCompileContext(PFunction *func, PPrototype *ret, bool fromdecorate, int stateindex, int statecount, int lump); diff --git a/src/scripting/decorate/thingdef_parse.cpp b/src/scripting/decorate/thingdef_parse.cpp index baa662047c..e292e18445 100644 --- a/src/scripting/decorate/thingdef_parse.cpp +++ b/src/scripting/decorate/thingdef_parse.cpp @@ -909,6 +909,7 @@ PClassActor *CreateNewActor(const FScriptPosition &sc, FName typeName, FName par } } ti = DecoDerivedClass(sc, parent, typeName); + ti->bDecorateClass = true; // we only set this for 'modern' DECORATE. The original stuff is so limited that it cannot do anything that may require flagging. ti->Replacee = ti->Replacement = NULL; ti->DoomEdNum = -1; diff --git a/src/scripting/thingdef.cpp b/src/scripting/thingdef.cpp index 8c42e8987d..d163ffb38d 100644 --- a/src/scripting/thingdef.cpp +++ b/src/scripting/thingdef.cpp @@ -206,6 +206,55 @@ void CreateDamageFunction(PClassActor *info, AActor *defaults, FxExpression *id, } } +//========================================================================== +// +// CheckForUnsafeStates +// +// Performs a quick analysis to find potentially bad states. +// This is not perfect because it cannot track jumps by function. +// For such cases a runtime check in the relevant places is also present. +// +//========================================================================== +static int CheckForUnsafeStates(PClassActor *obj) +{ + static ENamedName weaponstates[] = { NAME_Ready, NAME_Deselect, NAME_Select, NAME_Fire, NAME_AltFire, NAME_Hold, NAME_AltHold, NAME_Flash, NAME_AltFlash, NAME_None }; + static ENamedName pickupstates[] = { NAME_Pickup, NAME_Drop, NAME_Use, NAME_None }; + TMap checked; + ENamedName *test; + int errors = 0; + + if (obj->IsDescendantOf(RUNTIME_CLASS(AWeapon))) + { + if (obj->Size == RUNTIME_CLASS(AWeapon)->Size) return 0; // This class cannot have user variables. + test = weaponstates; + } + else if (obj->IsDescendantOf(RUNTIME_CLASS(ACustomInventory))) + { + if (obj->Size == RUNTIME_CLASS(ACustomInventory)->Size) return 0; // This class cannot have user variables. + test = pickupstates; + } + else return 0; // something else derived from AStateProvider. We do not know what this may be. + + for (; *test != NAME_None; test++) + { + FState *state = obj->FindState(*test); + while (state != nullptr && checked.CheckKey(state) == nullptr) // have we checked this state already. If yes, we can stop checking the current chain. + { + checked[state] = true; + if (state->ActionFunc && state->ActionFunc->Unsafe) + { + // If an unsafe function (i.e. one that accesses user variables) is being detected, print a warning once and remove the bogus function. We may not call it because that would inevitably crash. + auto owner = FState::StaticFindStateOwner(state); + Printf(TEXTCOLOR_RED "Unsafe state call in state %s.%d to %s, reached by %s.%s which accesses user variables.\n", + owner->TypeName.GetChars(), state - owner->OwnedStates, static_cast(state->ActionFunc)->PrintableName.GetChars(), obj->TypeName.GetChars(), FName(*test).GetChars()); + errors++; + } + state = state->NextState; + } + } + return errors; +} + //========================================================================== // // LoadActors @@ -253,6 +302,15 @@ void LoadActors () errorcount++; continue; } + + if (ti->bDecorateClass && ti->IsDescendantOf(RUNTIME_CLASS(AStateProvider))) + { + // either a DECORATE based weapon or CustomInventory. + // These are subject to relaxed rules for user variables in states. + // Although there is a runtime check for bogus states, let's do a quick analysis if any of the known entry points + // hits an unsafe state. If we can find something here it can be handled wuth a compile error rather than a runtime error. + errorcount += CheckForUnsafeStates(ti); + } } if (errorcount > 0) { diff --git a/src/scripting/vm/vm.h b/src/scripting/vm/vm.h index 6c7878a9a0..019d6c3a6b 100644 --- a/src/scripting/vm/vm.h +++ b/src/scripting/vm/vm.h @@ -656,6 +656,7 @@ class VMFunction : public DObject public: bool Native; bool Final = false; // cannot be overridden + bool Unsafe = false; // Contains references to class fields that are unsafe for psp and item state calls. BYTE ImplicitArgs = 0; // either 0 for static, 1 for method or 3 for action int VirtualIndex = -1; FName Name;