Bug 580873: Remove error checking on dprintf parameters

The parser for dprintf parameters was much too simplistic and
prevents real uses cases from operating.

It is not necessary to remove the , between the arguments to
dprintf, so don't try to split on that anymore. That also
means we can't check for and error on mismatch between format
specifiers and number of arguments.

e.g. "===> XML_EVENT_TEXT(%s)\n", (char *)strtok(Text,"\n")
should be permitted.

The alternative would be to write a more complete parser for
both the printf specification and the arguments. Well out of
scope and unnecessary.

Some inputs may now be accepted by the GUI and then when
inserted with GDB fail. These errors are displayed in the
GUI already anyway.

NewAndNoteworthy/CDT-11.0.md

@@ -10,6 +10,19 @@ This is the New & Noteworthy page for CDT 11.0 which is part of Eclipse 2022-12
 
 Jave 17 is now required to build and run Eclipse CDT. See https://github.com/eclipse-cdt/cdt/issues/80
 
+# Debug
+
+## C/C++ Dynamic Printf Breakpoints
+
+Prior to CDT 11 the Dynamic Printf GUI was somewhat over restrictive rejecting formats and parameters that would have been valid for dprintf in the GDB CLI.
+To achieve this improvement the checks that verified that the number of format specifiers matched the number of parameters has been removed.
+The check that the required the format string to not end with a `)` has been removed as valid format parameters can have a closing paranthesis.
+Instead of doing that check, the GUI now displays a `)` to make it obvious to users that the closing `)` should not be included in the setting.
+
+<p align="center"><img src="images/CDT-11.0-dprintf-gui.png" width="50%"></p>
+
+See [Bug 580873](https://bugs.eclipse.org/bugs/show_bug.cgi?id=580873).
+
 # API Changes, current and planned
 
 Please see [CHANGELOG-API](CHANGELOG-API.md) for details on the breaking API changes in this release as well as future planned API changes.

dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/breakpoints/GDBDynamicPrintfPropertyPage.java

@@ -43,7 +43,9 @@ import org.eclipse.jface.preference.StringFieldEditor;
 import org.eclipse.jface.util.PropertyChangeEvent;
 import org.eclipse.jface.viewers.ISelection;
 import org.eclipse.jface.viewers.IStructuredSelection;
+import org.eclipse.swt.SWT;
 import org.eclipse.swt.widgets.Composite;
+import org.eclipse.swt.widgets.Label;
 import org.eclipse.swt.widgets.Text;
 import org.eclipse.ui.IWorkbenchPropertyPage;
 import org.eclipse.ui.model.IWorkbenchAdapter;
@@ -305,6 +307,8 @@ public class GDBDynamicPrintfPropertyPage extends FieldEditorPreferencePage impl
 	protected void createPrintStringEditor(Composite parent) {
 		fPrintString = new DynamicPrintfStringFieldEditor(ICDynamicPrintf.PRINTF_STRING,
 				Messages.DynamicPrintfPropertyPage_PrintString, parent) {
+			private Label closingParenLabel;
+
 			@Override
 			protected boolean doCheckState() {
 				GDBDynamicPrintfUtils.GDBDynamicPrintfString parsedStr = new GDBDynamicPrintfUtils.GDBDynamicPrintfString(
@@ -317,6 +321,30 @@ public class GDBDynamicPrintfPropertyPage extends FieldEditorPreferencePage impl
 
 				return valid;
 			}
+
+			@Override
+			public int getNumberOfControls() {
+				return super.getNumberOfControls() + 1;
+			}
+
+			@Override
+			protected void adjustForNumColumns(int numColumns) {
+				super.adjustForNumColumns(numColumns - 1);
+			}
+
+			@Override
+			protected void doFillIntoGrid(Composite parent, int numColumns) {
+				super.doFillIntoGrid(parent, numColumns - 1);
+				if (closingParenLabel == null) {
+					closingParenLabel = new Label(parent, SWT.LEFT);
+					closingParenLabel.setFont(parent.getFont());
+					closingParenLabel.setText(")"); //$NON-NLS-1$
+					closingParenLabel.addDisposeListener(event -> closingParenLabel = null);
+				} else {
+					checkParent(closingParenLabel, parent);
+				}
+
+			}
 		};
 		addField(fPrintString);
 	}

dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/breakpoints/GDBDynamicPrintfUtils.java

@@ -15,7 +15,6 @@
 package org.eclipse.cdt.dsf.gdb.breakpoints;
 
 import org.eclipse.cdt.dsf.concurrent.Immutable;
-import org.eclipse.osgi.util.NLS;
 
 /**
  * Utility methods to help deal with Dynamic Printf logic.
@@ -51,17 +50,10 @@ public class GDBDynamicPrintfUtils {
 				return;
 			}
 
-			if (str.charAt(str.length() - 1) == ')') {
-				fErrorMessage = Messages.DynamicPrintf_Printf_not_expecting_a_closing_parenthesis;
-				return;
-			}
-
-			// Now go through the string and look for two things:
-			//   1- count the number of % (but ignore any %%)
-			//   2- find the closing double-quote (but ignore any \")
+			// Now go through the string and find the closing
+			// double-quote (but ignore any \")
 			char[] chars = str.toCharArray();
 			int closingQuoteIndex = 0;
-			int numArgExpected = 0;
 			for (int i = 1; i < chars.length; i++) {
 				switch (chars[i]) {
 				case '\\':
@@ -72,11 +64,6 @@ public class GDBDynamicPrintfUtils {
 					closingQuoteIndex = i;
 					break;
 				case '%':
-					if (chars.length > i + 1 && chars[i + 1] != '%') {
-						// Not a %% so we have found an expected argument.
-						numArgExpected++;
-					}
-
 					// We either found a second %, which we must skip to
 					// avoid parsing it again, or we didn't and we know
 					// our next character must be part of the format.
@@ -101,40 +88,21 @@ public class GDBDynamicPrintfUtils {
 			// We extract the string part of the printf string leaving the arguments
 			fStringSection = str.substring(0, closingQuoteIndex + 1);
 
-			int numArgPresent = 0;
 			if (closingQuoteIndex + 1 >= str.length()) {
 				// No more characters after the string part
 				fArgs = new String[0];
-				numArgPresent = 0;
 			} else {
 				String argString = str.substring(closingQuoteIndex + 1).trim();
+
 				if (argString.charAt(0) != ',') {
 					fErrorMessage = Messages.DynamicPrintf_Missing_comma;
 					return;
 				}
 
-				// Remove the first , to avoid an empty element after the split.
-				// Then split the string but keep any empty results
-				String[] args = argString.substring(1).split(",", -1); //$NON-NLS-1$
-
-				for (String argument : args) {
-					if (argument.trim().isEmpty()) {
-						fErrorMessage = Messages.DynamicPrintf_Empty_arg;
-						return;
-					}
-				}
-
-				fArgs = args;
-				numArgPresent = fArgs.length;
-			}
-
-			if (numArgPresent != numArgExpected) {
-				if (numArgPresent > numArgExpected) {
-					fErrorMessage = NLS.bind(Messages.DynamicPrintf_Extra_arg, numArgPresent - numArgExpected);
-				} else {
-					fErrorMessage = NLS.bind(Messages.DynamicPrintf_Missing_arg, numArgExpected - numArgPresent);
-				}
-				return;
+				// Remove the first , that separates the format
+				// from the arguments
+				String printfArguments = argString.substring(1);
+				fArgs = new String[] { printfArguments };
 			}
 
 			// Everything is ok!

dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/breakpoints/Messages.java

@@ -22,14 +22,7 @@ public class Messages extends NLS {
 	public static String DynamicPrintf_Invalid_string;
 	public static String DynamicPrintf_Printf_must_start_with_quote;
 	public static String DynamicPrintf_Printf_missing_closing_quote;
-	/**
-	 * @since 5.3
-	 */
-	public static String DynamicPrintf_Printf_not_expecting_a_closing_parenthesis;
 	public static String DynamicPrintf_Missing_comma;
-	public static String DynamicPrintf_Empty_arg;
-	public static String DynamicPrintf_Extra_arg;
-	public static String DynamicPrintf_Missing_arg;
 
 	static {
 		// initialize resource bundle

dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/breakpoints/Messages.properties

@@ -15,8 +15,4 @@
 DynamicPrintf_Invalid_string=Invalid printf string
 DynamicPrintf_Printf_must_start_with_quote=Printf string must start with a "
 DynamicPrintf_Printf_missing_closing_quote=Printf string is missing its closing "
-DynamicPrintf_Printf_not_expecting_a_closing_parenthesis=Specify Printf parameters without a closing parenthesis
 DynamicPrintf_Missing_comma=Printf string can only have a , after its closing "
-DynamicPrintf_Empty_arg=Printf string should not have empty arguments
-DynamicPrintf_Extra_arg={0} extra arguments in printf specification
-DynamicPrintf_Missing_arg={0} missing arguments in printf specification

dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/data/launch/src/BreakpointTestApp.cc

@@ -25,6 +25,8 @@ void zeroBlocks(int abc)
 void setBlocks()
 {
 	for (int i = 0; i < ARRAY_SIZE; i++) { // LINE_NUMBER_3
+		char Text[1000];
+		sprintf(Text, "Text %d\nAfter", i);
 		charBlock[i] = (char) i; // LINE_LOOP_1
 		integerBlock[i] = i;
 	}

dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/tests/MIBreakpointsTest.java

@@ -3336,6 +3336,27 @@ public class MIBreakpointsTest extends BaseParametrizedTestCase {
 		for (int i = 0; i < 256; i++) {
 			assertEquals("format " + i, contents[i].trim());
 		}
+	}
 
+	@Test
+	public void insertDprintfBreakpointBug580873() throws Throwable {
+		Map<String, Object> printfBreakpoint = new HashMap<>();
+		printfBreakpoint.put(MIBreakpoints.BREAKPOINT_TYPE, MIBreakpoints.DYNAMICPRINTF);
+		printfBreakpoint.put(MIBreakpoints.FILE_NAME, SOURCE_NAME);
+		printfBreakpoint.put(MIBreakpoints.LINE_NUMBER, LINE_LOOP_1);
+		printfBreakpoint.put(MIBreakpoints.PRINTF_STRING,
+				"\"===> XML_EVENT_TEXT(%s)\\n\", (char *)strtok(Text,\"\\n\")");
+
+		IBreakpointDMContext printfRef = insertBreakpoint(fBreakpointsDmc, printfBreakpoint);
+		waitForBreakpointEvent(1);
+		insertBreakpoint(fBreakpointsDmc, Map.of(BREAKPOINT_TYPE_TAG, BREAKPOINT_TAG, FILE_NAME_TAG, SOURCE_NAME,
+				LINE_NUMBER_TAG, LINE_NUMBER_5));
+		SyncUtil.resumeUntilStopped(5000);
+		IProcess[] processes = getGDBLaunch().getProcesses();
+		IStreamMonitor outputStreamMonitor = processes[1].getStreamsProxy().getOutputStreamMonitor();
+		String[] contents = outputStreamMonitor.getContents().split("\n");
+		for (int i = 0; i < 256; i++) {
+			assertEquals("===> XML_EVENT_TEXT(Text " + i + ")", contents[i].trim());
+		}
 	}
 }