From f9bb20fe2bf1d9154a3244579ea84400912473b4 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 2 Aug 2013 15:41:45 -0400 Subject: [PATCH] variable_watch: Don't share memory for callbacks The command itself is owned by the cmMakefile class, but the cmVariableWatch which holds a pointer to the cmVariableWatchCommand via the client_data for the callback outlives the cmMakefile class in the Qt GUI. This means that when the cmMakefile is destroyed, the variable watch is still in effect, but with a stale pointer. To fix this, each callback is now a separate entity completely and doesn't rely on the command which spawned it at all. An example CMakeLists.txt which demonstrates the issue (only displayed in cmake-gui, so no tests can be written for it): set(var 0) variable_watch(var) --- Source/cmVariableWatchCommand.cxx | 177 +++++++++++++++++------------- Source/cmVariableWatchCommand.h | 17 +-- 2 files changed, 107 insertions(+), 87 deletions(-) diff --git a/Source/cmVariableWatchCommand.cxx b/Source/cmVariableWatchCommand.cxx index 2b81d79e2..3b75ade44 100644 --- a/Source/cmVariableWatchCommand.cxx +++ b/Source/cmVariableWatchCommand.cxx @@ -13,20 +13,104 @@ #include "cmVariableWatch.h" +//---------------------------------------------------------------------------- +struct cmVariableWatchCallbackData +{ + bool InCallback; + std::string Command; +}; + //---------------------------------------------------------------------------- static void cmVariableWatchCommandVariableAccessed( const std::string& variable, int access_type, void* client_data, const char* newValue, const cmMakefile* mf) { - cmVariableWatchCommand* command - = static_cast(client_data); - command->VariableAccessed(variable, access_type, newValue, mf); + cmVariableWatchCallbackData* data + = static_cast(client_data); + + if ( data->InCallback ) + { + return; + } + data->InCallback = true; + + cmListFileFunction newLFF; + cmListFileArgument arg; + bool processed = false; + const char* accessString = cmVariableWatch::GetAccessAsString(access_type); + const char* currentListFile = mf->GetDefinition("CMAKE_CURRENT_LIST_FILE"); + + /// Ultra bad!! + cmMakefile* makefile = const_cast(mf); + + std::string stack = makefile->GetProperty("LISTFILE_STACK"); + if ( !data->Command.empty() ) + { + newLFF.Arguments.clear(); + newLFF.Arguments.push_back( + cmListFileArgument(variable, true, "unknown", 9999)); + newLFF.Arguments.push_back( + cmListFileArgument(accessString, true, "unknown", 9999)); + newLFF.Arguments.push_back( + cmListFileArgument(newValue?newValue:"", true, "unknown", 9999)); + newLFF.Arguments.push_back( + cmListFileArgument(currentListFile, true, "unknown", 9999)); + newLFF.Arguments.push_back( + cmListFileArgument(stack, true, "unknown", 9999)); + newLFF.Name = data->Command; + newLFF.FilePath = "Some weird path"; + newLFF.Line = 9999; + cmExecutionStatus status; + if(!makefile->ExecuteCommand(newLFF,status)) + { + arg.FilePath = "Unknown"; + arg.Line = 0; + cmOStringStream error; + error << "Error in cmake code at\n" + << arg.FilePath << ":" << arg.Line << ":\n" + << "A command failed during the invocation of callback \"" + << data->Command << "\"."; + cmSystemTools::Error(error.str().c_str()); + data->InCallback = false; + return; + } + processed = true; + } + if ( !processed ) + { + cmOStringStream msg; + msg << "Variable \"" << variable.c_str() << "\" was accessed using " + << accessString << " with value \"" << newValue << "\"."; + makefile->IssueMessage(cmake::LOG, msg.str()); + } + + data->InCallback = false; +} + +//---------------------------------------------------------------------------- +static void deleteVariableWatchCallbackData(void* client_data) +{ + cmVariableWatchCallbackData* data + = static_cast(client_data); + delete data; } //---------------------------------------------------------------------------- cmVariableWatchCommand::cmVariableWatchCommand() { - this->InCallback = false; +} + +//---------------------------------------------------------------------------- +cmVariableWatchCommand::~cmVariableWatchCommand() +{ + std::set::const_iterator it; + for ( it = this->WatchedVariables.begin(); + it != this->WatchedVariables.end(); + ++it ) + { + this->Makefile->GetCMakeInstance()->GetVariableWatch()->RemoveWatch( + *it, cmVariableWatchCommandVariableAccessed); + } } //---------------------------------------------------------------------------- @@ -39,10 +123,10 @@ bool cmVariableWatchCommand return false; } std::string variable = args[0]; + std::string command; if ( args.size() > 1 ) { - std::string command = args[1]; - this->Handlers[variable].Commands.push_back(args[1]); + command = args[1]; } if ( variable == "CMAKE_CURRENT_LIST_FILE" ) { @@ -52,74 +136,19 @@ bool cmVariableWatchCommand return false; } - this->Makefile->GetCMakeInstance()->GetVariableWatch()->AddWatch( - variable, cmVariableWatchCommandVariableAccessed, this); + cmVariableWatchCallbackData* data = new cmVariableWatchCallbackData; + + data->InCallback = false; + data->Command = command; + + this->WatchedVariables.insert(variable); + if ( !this->Makefile->GetCMakeInstance()->GetVariableWatch()->AddWatch( + variable, cmVariableWatchCommandVariableAccessed, + data, deleteVariableWatchCallbackData) ) + { + deleteVariableWatchCallbackData(data); + return false; + } return true; } - -//---------------------------------------------------------------------------- -void cmVariableWatchCommand::VariableAccessed(const std::string& variable, - int access_type, const char* newValue, const cmMakefile* mf) -{ - if ( this->InCallback ) - { - return; - } - this->InCallback = true; - - cmListFileFunction newLFF; - cmVariableWatchCommandHandler *handler = &this->Handlers[variable]; - cmVariableWatchCommandHandler::VectorOfCommands::iterator it; - cmListFileArgument arg; - bool processed = false; - const char* accessString = cmVariableWatch::GetAccessAsString(access_type); - const char* currentListFile = mf->GetDefinition("CMAKE_CURRENT_LIST_FILE"); - - /// Ultra bad!! - cmMakefile* makefile = const_cast(mf); - - std::string stack = makefile->GetProperty("LISTFILE_STACK"); - for ( it = handler->Commands.begin(); it != handler->Commands.end(); - ++ it ) - { - std::string command = *it; - newLFF.Arguments.clear(); - newLFF.Arguments.push_back( - cmListFileArgument(variable, true, "unknown", 9999)); - newLFF.Arguments.push_back( - cmListFileArgument(accessString, true, "unknown", 9999)); - newLFF.Arguments.push_back( - cmListFileArgument(newValue?newValue:"", true, "unknown", 9999)); - newLFF.Arguments.push_back( - cmListFileArgument(currentListFile, true, "unknown", 9999)); - newLFF.Arguments.push_back( - cmListFileArgument(stack, true, "unknown", 9999)); - newLFF.Name = command; - newLFF.FilePath = "Some weird path"; - newLFF.Line = 9999; - cmExecutionStatus status; - if(!makefile->ExecuteCommand(newLFF,status)) - { - arg.FilePath = "Unknown"; - arg.Line = 0; - cmOStringStream error; - error << "Error in cmake code at\n" - << arg.FilePath << ":" << arg.Line << ":\n" - << "A command failed during the invocation of callback \"" - << command << "\"."; - cmSystemTools::Error(error.str().c_str()); - this->InCallback = false; - return; - } - processed = true; - } - if ( !processed ) - { - cmOStringStream msg; - msg << "Variable \"" << variable.c_str() << "\" was accessed using " - << accessString << " with value \"" << newValue << "\"."; - makefile->IssueMessage(cmake::LOG, msg.str()); - } - this->InCallback = false; -} diff --git a/Source/cmVariableWatchCommand.h b/Source/cmVariableWatchCommand.h index 3abc08894..545535c40 100644 --- a/Source/cmVariableWatchCommand.h +++ b/Source/cmVariableWatchCommand.h @@ -14,13 +14,6 @@ #include "cmCommand.h" -class cmVariableWatchCommandHandler -{ -public: - typedef std::vector VectorOfCommands; - VectorOfCommands Commands; -}; - /** \class cmVariableWatchCommand * \brief Watch when the variable changes and invoke command * @@ -39,6 +32,9 @@ public: //! Default constructor cmVariableWatchCommand(); + //! Destructor. + ~cmVariableWatchCommand(); + /** * This is called when the command is first encountered in * the CMakeLists.txt file. @@ -83,13 +79,8 @@ public: cmTypeMacro(cmVariableWatchCommand, cmCommand); - void VariableAccessed(const std::string& variable, int access_type, - const char* newValue, const cmMakefile* mf); - protected: - std::map Handlers; - - bool InCallback; + std::set WatchedVariables; };