Code cleanup of conddebug.cpp. Make functions that are not externally used static. Changed condition to have a constructor/destructor and allocate via std new/delete. Fixed a small memory leak. For Qt GUI, refactored debugger breakpoint editor window so that it has its own class to allow for more detailed error checking methods to be added.

src/conddebug.cpp

@@ -52,17 +52,17 @@
 #include <cctype>
 
 uint16 debugLastAddress = 0; // used by 'T' and 'R' conditions
-uint8 debugLastOpcode; // used to evaluate 'W' condition
+uint8 debugLastOpcode = 0; // used to evaluate 'W' condition
 
 // Next non-whitespace character in string
-char next;
+static char next = 0;
 
-int ishex(char c)
+static int ishex(char c)
 {
 	return isdigit(c) || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F');
 }
 
-void scan(const char** str)
+static void scan(const char** str)
 {
 	do
 	{
@@ -71,40 +71,37 @@ void scan(const char** str)
 	} while (isspace(next));
 }
 
-// Frees a condition and all of it's sub conditions
-void freeTree(Condition* c)
-{
-	if (c->lhs) freeTree(c->lhs);
-	if (c->rhs) freeTree(c->rhs);
-
-	free(c);
-}
-
 // Generic function to handle all infix operators but the last one in the precedence hierarchy. : '(' E ')'
-Condition* InfixOperator(const char** str, Condition(*nextPart(const char**)), int(*operators)(const char**))
+static Condition* InfixOperator(const char** str, Condition(*nextPart(const char**)), int(*operators)(const char**))
 {
 	Condition* t = nextPart(str);
 	Condition* t1;
 	Condition* mid;
 	int op;
 
+	if (t == nullptr)
+	{
+		return nullptr;
+	}
 	while ((op = operators(str)))
 	{
 		scan(str);
 
 		t1 = nextPart(str);
 
-		if (t1 == 0)
+		if (t1 == nullptr)
 		{
-			if(t)
+			delete t;
-				freeTree(t);
 			return 0;
 		}
 
-		mid = (Condition*)FCEU_dmalloc(sizeof(Condition));
+		mid = new Condition();
-		if (!mid)
+		if (mid == nullptr)
-			return NULL;
+		{
-		memset(mid, 0, sizeof(Condition));
+			delete t;
+			delete t1;
+			return nullptr;
+		}
 
 		mid->lhs = t;
 		mid->rhs = t1;
@@ -117,7 +114,7 @@ Condition* InfixOperator(const char** str, Condition(*nextPart(const char**)), i
 }
 
 // Generic handler for two-character operators
-int TwoCharOperator(const char** str, char c1, char c2, int op)
+static int TwoCharOperator(const char** str, char c1, char c2, int op)
 {
 	if (next == c1 && **str == c2)
 	{
@@ -131,43 +128,43 @@ int TwoCharOperator(const char** str, char c1, char c2, int op)
 }
 
 // Determines if a character is a flag
-int isFlag(char c)
+static int isFlag(char c)
 {
 	return c == 'N' || c == 'I' || c == 'C' || c == 'V' || c == 'Z' || c == 'B' || c == 'U' || c == 'D';
 }
 
 // Determines if a character is a register
-int isRegister(char c)
+static int isRegister(char c)
 {
 	return c == 'A' || c == 'X' || c == 'Y' || c == 'P' || c == 'S';
 }
 
 // Determines if a character is for PC bank
-int isPCBank(char c)
+static int isPCBank(char c)
 {
 	return c == 'K';
 }
 
 // Determines if a character is for Data bank
-int isDataBank(char c)
+static int isDataBank(char c)
 {
 	return c == 'T';
 }
 
 // Determines if a character is for value read
-int isValueRead(char c)
+static int isValueRead(char c)
 {
 	return c == 'R';
 }
 
 // Determines if a character is for value write
-int isValueWrite(char c)
+static int isValueWrite(char c)
 {
 	return c == 'W';
 }
 
 // Reads a hexadecimal number from str
-int getNumber(unsigned int* number, const char** str)
+static int getNumber(unsigned int* number, const char** str)
 {
 //	char buffer[5];
 
@@ -185,10 +182,10 @@ int getNumber(unsigned int* number, const char** str)
 	return 1;
 }
 
-Condition* Connect(const char** str);
+static Condition* Connect(const char** str);
 
 // Handles the following part of the grammar: '(' E ')'
-Condition* Parentheses(const char** str, Condition* c, char openPar, char closePar)
+static Condition* Parentheses(const char** str, Condition* c, char openPar, char closePar)
 {
 	if (next == openPar)
 	{
@@ -216,7 +213,7 @@ Condition* Parentheses(const char** str, Condition* c, char openPar, char closeP
 * Check for primitives
 * Flags, Registers, Numbers, Addresses and parentheses
 */
-Condition* Primitive(const char** str, Condition* c)
+static Condition* Primitive(const char** str, Condition* c)
 {
 	if (isFlag(next)) /* Flags */
 	{
@@ -394,24 +391,22 @@ Condition* Primitive(const char** str, Condition* c)
 }
 
 /* Handle * and / operators */
-Condition* Term(const char** str)
+static Condition* Term(const char** str)
 {
 	Condition* t;
 	Condition* t1;
 	Condition* mid;
 
-	t = (Condition*)FCEU_dmalloc(sizeof(Condition));
+	t = new Condition();
 
-	if (!t)
+	if (t == nullptr)
 	{
 		return NULL;
 	}
 
-	memset(t, 0, sizeof(Condition));
-
 	if (!Primitive(str, t))
 	{
-		freeTree(t);
+		delete t;
 		return 0;
 	}
 
@@ -421,22 +416,25 @@ Condition* Term(const char** str)
 
 		scan(str);
 
-		if (!(t1 = (Condition*)FCEU_dmalloc(sizeof(Condition))))
+		if ((t1 = new Condition()) == nullptr)
-            return NULL;
+		{
-
+			delete t;
-		memset(t1, 0, sizeof(Condition));
+			return nullptr;
+		}
 
 		if (!Primitive(str, t1))
 		{
-			freeTree(t);
+			delete t;
-			freeTree(t1);
+			delete t1;
 			return 0;
 		}
 
-		if (!(mid = (Condition*)FCEU_dmalloc(sizeof(Condition))))
+		if ((mid = new Condition()) == nullptr)
-            return NULL;
+		{
-
+			delete t;
-		memset(mid, 0, sizeof(Condition));
+			delete t1;
+			return nullptr;
+		}
 
 		mid->lhs = t;
 		mid->rhs = t1;
@@ -449,7 +447,7 @@ Condition* Term(const char** str)
 }
 
 /* Check for + and - operators */
-int SumOperators(const char** str)
+static int SumOperators(const char** str)
 {
 	switch (next)
 	{
@@ -460,13 +458,13 @@ int SumOperators(const char** str)
 }
 
 /* Handle + and - operators */
-Condition* Sum(const char** str)
+static Condition* Sum(const char** str)
 {
 	return InfixOperator(str, Term, SumOperators);
 }
 
 /* Check for <=, =>, ==, !=, > and < operators */
-int CompareOperators(const char** str)
+static int CompareOperators(const char** str)
 {
 	int val = TwoCharOperator(str, '=', '=', OP_EQ);
 	if (val) return val;
@@ -490,13 +488,13 @@ int CompareOperators(const char** str)
 }
 
 /* Handle <=, =>, ==, !=, > and < operators */
-Condition* Compare(const char** str)
+static Condition* Compare(const char** str)
 {
 	return InfixOperator(str, Sum, CompareOperators);
 }
 
 /* Check for || or && operators */
-int ConnectOperators(const char** str)
+static int ConnectOperators(const char** str)
 {
 	int val = TwoCharOperator(str, '|', '|', OP_OR);
 	if(val) return val;
@@ -508,7 +506,7 @@ int ConnectOperators(const char** str)
 }
 
 /* Handle || and && operators */
-Condition* Connect(const char** str)
+static Condition* Connect(const char** str)
 {
 	return InfixOperator(str, Compare, ConnectOperators);
 }
@@ -521,6 +519,10 @@ Condition* generateCondition(const char* str)
 	scan(&str);
 	c = Connect(&str);
 
-	if (!c || next != 0) return 0;
+	if (!c || next != 0)
+	{
+		if (c) delete c;
+		return 0;
+	}
 	else return c;
 }

src/conddebug.h

@@ -61,9 +61,28 @@ struct Condition
 
 	unsigned int type2;
 	unsigned int value2;
+
+	Condition(void)
+	{
+		op = 0;
+		lhs = rhs = nullptr;
+		type1 = value1 = 0;
+		type2 = value2 = 0;
+	};
+
+	~Condition(void)
+	{
+		if (lhs)
+		{
+			delete lhs;
+		}
+		if (rhs)
+		{
+			delete rhs;
+		}
+	}
 };
 
-void freeTree(Condition* c);
 Condition* generateCondition(const char* str);
 
 #endif

src/debug.cpp

@@ -139,7 +139,7 @@ int checkCondition(const char* condition, int num)
 		// Remove the old breakpoint condition before adding a new condition.
 		if (watchpoint[num].cond)
 		{
-			freeTree(watchpoint[num].cond);
+			delete watchpoint[num].cond;
 			free(watchpoint[num].condText);
 			watchpoint[num].cond = 0;
 			watchpoint[num].condText = 0;
@@ -153,8 +153,8 @@ int checkCondition(const char* condition, int num)
 		{
 			watchpoint[num].cond = c;
 			watchpoint[num].condText = (char*)malloc(strlen(condition) + 1);
-            if (!watchpoint[num].condText)
+			if (!watchpoint[num].condText)
-                return 0;
+				return 0;
 			strcpy(watchpoint[num].condText, condition);
 		}
 		else
@@ -169,7 +169,7 @@ int checkCondition(const char* condition, int num)
 		// Remove the old breakpoint condition
 		if (watchpoint[num].cond)
 		{
-			freeTree(watchpoint[num].cond);
+			delete watchpoint[num].cond;
 			free(watchpoint[num].condText);
 			watchpoint[num].cond = 0;
 			watchpoint[num].condText = 0;

src/drivers/Qt/ConsoleDebugger.cpp

@@ -1834,6 +1834,391 @@ void ConsoleDebugger::selBmAddrChanged(const QString &txt)
 	//printf("selBmAddrVal = %04X\n", selBmAddrVal );
 }
 //----------------------------------------------------------------------------
+DebuggerBreakpointEditor::DebuggerBreakpointEditor(int editIndex, watchpointinfo *wpIn, QWidget *parent)
+	: QDialog(parent)
+{
+	editIdx = editIndex;
+	wp = wpIn;
+
+	QHBoxLayout *hbox;
+	QVBoxLayout *mainLayout, *vbox;
+	QLabel *lbl;
+	QGridLayout *grid;
+	QFrame *frame;
+	QGroupBox *gbox;
+
+	if ( editIdx >= 0 )
+	{
+		setWindowTitle( tr("Edit Breakpoint") );
+	}
+	else
+	{
+		setWindowTitle( tr("Add Breakpoint") );
+	}
+
+	hbox       = new QHBoxLayout();
+	mainLayout = new QVBoxLayout();
+
+	mainLayout->addLayout( hbox );
+
+	lbl   = new QLabel( tr("Address") );
+	addr1 = new QLineEdit();
+
+	hbox->addWidget( lbl );
+	hbox->addWidget( addr1 );
+
+	lbl   = new QLabel( tr("-") );
+	addr2 = new QLineEdit();
+	hbox->addWidget( lbl );
+	hbox->addWidget( addr2 );
+
+	forbidChkBox = new QCheckBox( tr("Forbid") );
+	hbox->addWidget( forbidChkBox );
+
+	frame = new QFrame();
+	vbox  = new QVBoxLayout();
+	hbox  = new QHBoxLayout();
+	gbox  = new QGroupBox();
+
+	rbp = new QCheckBox( tr("Read") );
+	wbp = new QCheckBox( tr("Write") );
+	xbp = new QCheckBox( tr("Execute") );
+	ebp = new QCheckBox( tr("Enable") );
+
+	gbox->setTitle( tr("Memory") );
+	mainLayout->addWidget( frame );
+	frame->setLayout( vbox );
+	frame->setFrameShape( QFrame::Box );
+	vbox->addLayout( hbox );
+	vbox->addWidget( gbox );
+
+	hbox->addWidget( rbp );
+	hbox->addWidget( wbp );
+	hbox->addWidget( xbp );
+	hbox->addWidget( ebp );
+	
+	hbox         = new QHBoxLayout();
+	cpu_radio    = new QRadioButton( tr("CPU") );
+	ppu_radio    = new QRadioButton( tr("PPU") );
+	oam_radio    = new QRadioButton( tr("OAM") );
+	rom_radio    = new QRadioButton( tr("ROM") );
+	cpu_radio->setChecked(true);
+
+	gbox->setLayout( hbox );
+	hbox->addWidget( cpu_radio );
+	hbox->addWidget( ppu_radio );
+	hbox->addWidget( oam_radio );
+	hbox->addWidget( rom_radio );
+
+	grid  = new QGridLayout();
+
+	mainLayout->addLayout( grid );
+	lbl   = new QLabel( tr("Condition") );
+	cond  = new QLineEdit();
+	condValid = true;
+
+	connect( cond, SIGNAL(textChanged(const QString &)), this, SLOT(conditionTextChanged(const QString &)));
+
+	grid->addWidget(  lbl, 0, 0 );
+	grid->addWidget( cond, 0, 1 );
+
+	lbl   = new QLabel( tr("Name") );
+	name  = new QLineEdit();
+
+	grid->addWidget(  lbl, 1, 0 );
+	grid->addWidget( name, 1, 1 );
+
+	hbox         = new QHBoxLayout();
+	msgLbl       = new QLabel();
+	okButton     = new QPushButton( tr("OK") );
+	cancelButton = new QPushButton( tr("Cancel") );
+
+	mainLayout->addLayout( hbox );
+	hbox->addWidget( msgLbl, 5 );
+	hbox->addWidget( cancelButton, 1 );
+	hbox->addWidget(     okButton, 1 );
+
+	connect(     okButton, SIGNAL(clicked(void)), this, SLOT(accept(void)) );
+	connect( cancelButton, SIGNAL(clicked(void)), this, SLOT(reject(void)) );
+
+	    okButton->setIcon( style()->standardIcon( QStyle::SP_DialogOkButton ) );
+	cancelButton->setIcon( style()->standardIcon( QStyle::SP_DialogCancelButton ) );
+
+	okButton->setDefault(true);
+
+	if ( wp != NULL )
+	{
+		char stmp[256];
+
+		if ( wp->flags & BT_P )
+		{
+			ppu_radio->setChecked(true);
+		}
+		else if ( wp->flags & BT_S )
+		{
+			oam_radio->setChecked(true);
+		}
+		else if ( wp->flags & BT_R )
+		{
+			rom_radio->setChecked(true);
+		}
+
+		sprintf( stmp, "%04X", wp->address );
+
+		addr1->setText( tr(stmp) );
+
+		if ( wp->endaddress > 0 )
+		{
+			sprintf( stmp, "%04X", wp->endaddress );
+
+			addr2->setText( tr(stmp) );
+		}
+
+		if ( wp->flags & WP_R )
+		{
+		   rbp->setChecked(true);
+		}
+		if ( wp->flags & WP_W )
+		{
+		   wbp->setChecked(true);
+		}
+		if ( wp->flags & WP_X )
+		{
+		   xbp->setChecked(true);
+		}
+		if ( wp->flags & WP_F )
+		{
+		   forbidChkBox->setChecked(true);
+		}
+		if ( wp->flags & WP_E )
+		{
+		   ebp->setChecked(true);
+		}
+
+		if ( wp->condText )
+		{
+			cond->setText( tr(wp->condText) );
+		}
+		else
+		{
+			if ( editIdx < 0 )
+			{
+				// If new breakpoint, default enable checkbox to true
+				ebp->setChecked(true);
+
+				// If new breakpoint, suggest condition if in ROM Mapping area of memory.
+				if ( cpu_radio->isChecked() && (wp->address >= 0x8000) )
+				{
+					int romAddr = GetNesFileAddress(wp->address);
+
+					if ( romAddr >= 0 )
+					{
+						wp->address = romAddr;
+						sprintf( stmp, "%X", wp->address );
+						addr1->setText( tr(stmp) );
+						rom_radio->setChecked(true);
+					}
+					else
+					{
+						char str[64];
+						sprintf(str, "K==#%02X", getBank(wp->address));
+						cond->setText( tr(str) );
+					}
+				}
+			}
+		}
+
+		if ( wp->desc )
+		{
+			name->setText( tr(wp->desc) );
+		}
+	}
+	else
+	{
+		// If new breakpoint, default enable checkbox to true
+		ebp->setChecked(true);
+	}
+
+	setLayout( mainLayout );
+
+	connect( this  , SIGNAL(finished(int)), this, SLOT(closeWindow(int)) );
+}
+//----------------------------------------------------------------------------
+DebuggerBreakpointEditor::~DebuggerBreakpointEditor(void)
+{
+
+}
+//----------------------------------------------------------------------------
+void DebuggerBreakpointEditor::closeEvent(QCloseEvent *event)
+{
+	//printf("Close Window Event\n");
+	done(QDialog::Rejected);
+	deleteLater();
+	event->accept();
+}
+//----------------------------------------------------------------------------
+void DebuggerBreakpointEditor::closeWindow(int ret)
+{
+	//printf("Close Window %i\n", ret);
+	if ( ret == QDialog::Accepted )
+	{
+		loadBreakpoint();
+	}
+	deleteLater();
+}
+//----------------------------------------------------------------------------
+void DebuggerBreakpointEditor::checkDataValid(void)
+{
+	bool allEntriesValid = condValid;
+
+	okButton->setEnabled( allEntriesValid );
+
+	if (allEntriesValid)
+	{
+		msgLbl->clear();
+	}
+	else if (!condValid)
+	{
+		msgLbl->setText(tr("Condition Invalid"));
+	}
+	else
+	{
+		msgLbl->clear();
+	}
+}
+//----------------------------------------------------------------------------
+void DebuggerBreakpointEditor::conditionTextChanged(const QString &txt)
+{
+	if ( txt.size() > 0 )
+	{
+		Condition *c = generateCondition( txt.toStdString().c_str() );
+
+		condValid = (c != nullptr);
+
+		if (c)
+		{
+			delete c; c = nullptr;
+		}
+	}
+	else
+	{
+		condValid = true;
+	}
+	checkDataValid();
+}
+//----------------------------------------------------------------------------
+void DebuggerBreakpointEditor::loadBreakpoint(void)
+{
+	int  start_addr = -1, end_addr = -1, type = 0, enable = 1, slot;
+	std::string s;
+
+	FCEU_WRAPPER_LOCK();
+
+	slot = (editIdx < 0) ? numWPs : editIdx;
+
+	if ( cpu_radio->isChecked() )
+	{
+		type |= BT_C;
+	}
+	else if ( ppu_radio->isChecked() ) 
+	{
+		type |= BT_P;
+	}
+	else if ( oam_radio->isChecked() ) 
+	{
+		type |= BT_S;
+	}
+	else if ( rom_radio->isChecked() )
+	{
+		type |= BT_R;
+	}
+
+	s = addr1->text().toStdString();
+
+	if ( s.size() > 0 )
+	{
+		start_addr = offsetStringToInt( type, s.c_str() );
+	}
+
+	s = addr2->text().toStdString();
+
+	if ( s.size() > 0 )
+	{
+		end_addr = offsetStringToInt( type, s.c_str() );
+	}
+
+	if ( rbp->isChecked() )
+	{
+		type |= WP_R;
+	}
+	if ( wbp->isChecked() )
+	{
+		type |= WP_W;
+	}
+	if ( xbp->isChecked() )
+	{
+		type |= WP_X;
+	}
+
+	if ( forbidChkBox->isChecked() )
+	{
+		type |= WP_F;
+	}
+
+	enable = ebp->isChecked();
+
+	if ( (start_addr >= 0) && (numWPs < 64) )
+	{
+		unsigned int retval;
+		std::string nameString, condString;
+
+		nameString = name->text().toStdString();
+		condString = cond->text().toStdString();
+
+		retval = NewBreak( nameString.c_str(), start_addr, end_addr, type, condString.c_str(), slot, enable);
+
+		if ( (retval == 1) || (retval == 2) )
+		{
+			printf("Breakpoint Add Failed\n");
+		}
+		else
+		{
+			if (editIdx < 0)
+			{
+				numWPs++;
+			}
+
+			//bpListUpdate( false );
+		}
+	}
+	FCEU_WRAPPER_UNLOCK();
+}
+//----------------------------------------------------------------------------
+	       	//int editIndex, watchpointinfo *wp, bool forceAccept, 
+//----------------------------------------------------------------------------
+void ConsoleDebugger::openBpEditWindow( int editIdx, watchpointinfo *wp, bool forceAccept )
+{
+	int ret;
+
+	DebuggerBreakpointEditor *dialog = new DebuggerBreakpointEditor( editIdx, wp, this );
+
+	if ( forceAccept )
+	{
+		dialog->loadBreakpoint();
+		dialog->deleteLater();
+		ret = QDialog::Accepted;
+	}
+	else
+	{
+		ret = dialog->exec();
+	}
+
+	if (ret == QDialog::Accepted)
+	{
+		bpListUpdate( false );
+	}
+}
+/*
 void ConsoleDebugger::openBpEditWindow( int editIdx, watchpointinfo *wp, bool forceAccept )
 {
 	int ret;
@@ -2131,6 +2516,7 @@ void ConsoleDebugger::openBpEditWindow( int editIdx, watchpointinfo *wp, bool fo
 		}
 	}
 }
+*/
 //----------------------------------------------------------------------------
 void ConsoleDebugger::openDebugSymbolEditWindow( int addr )
 {
@@ -2439,7 +2825,7 @@ static void DeleteBreak(int sel)
 
 	if (watchpoint[sel].cond)
 	{
-		freeTree(watchpoint[sel].cond);
+		delete watchpoint[sel].cond;
 	}
 	if (watchpoint[sel].condText)
 	{
@@ -2488,7 +2874,7 @@ void debuggerClearAllBreakpoints(void)
 	{
 	   if (watchpoint[i].cond)
 	   {
-	   	freeTree(watchpoint[i].cond);
+	   	delete watchpoint[i].cond;
 	   }
 	   if (watchpoint[i].condText)
 	   {
@@ -2499,7 +2885,7 @@ void debuggerClearAllBreakpoints(void)
 	   	free(watchpoint[i].desc);
 	   }
 
-		watchpoint[i].address = 0;
+	   watchpoint[i].address = 0;
 	   watchpoint[i].endaddress = 0;
 	   watchpoint[i].flags = 0;
 	   watchpoint[i].cond = 0;

src/drivers/Qt/ConsoleDebugger.h

@@ -420,6 +420,49 @@ class DebugBreakOnDialog : public QDialog
 		void resetDeltas(void);
 };
 
+class DebuggerBreakpointEditor : public QDialog
+{
+   Q_OBJECT
+
+	public:
+		DebuggerBreakpointEditor(int editIndex = -1, watchpointinfo *wpIn = nullptr, QWidget *parent = 0);
+		~DebuggerBreakpointEditor(void);
+
+		void loadBreakpoint(void);
+
+	protected:
+		void closeEvent(QCloseEvent *event) override;
+		void checkDataValid(void);
+
+	private:
+		int editIdx;
+		watchpointinfo *wp;
+
+		QLineEdit *addr1;
+		QLineEdit *addr2;
+		QLineEdit *cond;
+		QLineEdit *name;
+		QCheckBox *forbidChkBox;
+		QCheckBox *rbp;
+		QCheckBox *wbp;
+		QCheckBox *xbp;
+		QCheckBox *ebp;
+		QLabel    *msgLbl;
+
+		QPushButton *okButton;
+		QPushButton *cancelButton;
+		QRadioButton *cpu_radio;
+		QRadioButton *ppu_radio;
+		QRadioButton *oam_radio;
+		QRadioButton *rom_radio;
+
+		bool condValid;
+
+	private slots:
+		void closeWindow(int ret);
+		void conditionTextChanged( const QString &text );
+};
+
 class ConsoleDebugger : public QDialog
 {
    Q_OBJECT

src/drivers/win/debugger.cpp

@@ -1235,7 +1235,7 @@ void DeleteBreak(int sel)
 	if(sel<0) return;
 	if(sel>=numWPs) return;
 	if (watchpoint[sel].cond)
-		freeTree(watchpoint[sel].cond);
+		delete watchpoint[sel].cond;
 	if (watchpoint[sel].condText)
 		free(watchpoint[sel].condText);
 	if (watchpoint[sel].desc)