Merge pull request #3886 from grandixximo/issue-3880-fix

Detect and error on nested/invalid subroutine definitions in named subs

Author: andypugh

docs/src/gcode/o-code.adoc

@@ -123,8 +123,37 @@ o100 endsub
 o100 call [100] [2] [325]
 ----
 
-Subroutine bodies may not be nested.
-They may only be called after they are defined.
+Subroutine *definitions* may not be nested.
+Defining a subroutine inside another subroutine is not allowed and will
+produce an error.  For example, the following is *invalid*:
+
+.Invalid Nested Subroutine Definition (produces an error)
+[source,{ngc}]
+----
+o<outer> sub
+  o100 sub       (INVALID: nested subroutine definition)
+    (code here)
+  o100 endsub
+o<outer> endsub
+----
+
+However, *calling* a subroutine from within another subroutine is perfectly
+valid.  It is the `sub`/`endsub` definition that cannot be nested, not the
+`call`:
+
+.Valid Nested Subroutine Call
+[source,{ngc}]
+----
+o<outer> sub
+  (code here)
+  o<helper> call [1] [2]  (OK: calling another sub)
+o<outer> endsub
+----
+
+Other O-code control flow such as `if`/`endif`, `while`/`endwhile`,
+`do`/`while`, and `repeat`/`endrepeat` may be used freely inside subroutines.
+
+Subroutines may only be called after they are defined.
 They may be called from other functions, and may call themselves recursively if it makes sense to do so.
 The maximum subroutine nesting level is 10.
 
@@ -391,6 +420,10 @@ your call and include a sub and endsub in the file. The file must be in the
 directory pointed to by 'PROGRAM_PREFIX' or 'SUBROUTINE_PATH' in the INI file.
 The file name can include *lowercase* letters, numbers, dash, and underscore
 only. A named subroutine file can contain only a single subroutine definition.
+Do not place additional numbered subroutine definitions (e.g. `o100 sub`)
+in the same file as a named subroutine.  Numbered subroutines share a global
+namespace and can silently conflict across files.  If you need helper
+subroutines, place each one in its own file.
 
 .Named File Example
 [source,{ngc}]
@@ -455,6 +488,9 @@ interpreter:
  - a label on `else`, `elseif` or `endif` not pointing to a matching `if`
  - a label on `break` or `continue` which does not point to a matching `while` or `do`
  - a label on `endrepeat` or `endwhile` no referring to a corresponding `while` or `repeat`
+ - a subroutine definition (`sub`) inside another subroutine body (nested definitions)
+ - a numbered subroutine called from within a named subroutine file but not found in the
+   offset table or as a separate file
 
 To make these errors non-fatal  warnings on stderr, set bit 0x20 in
 the `[RS274NGC]FEATURE=` mask ini option.

src/emc/rs274ngc/interp_o_word.cc

@@ -277,7 +277,6 @@ int Interp::execute_call(setup_pointer settings,
 	    logOword("M98 L0 instruction; skipping call");
 	else if (control_back_to(eblock, settings) == INTERP_ERROR) {
 	    settings->call_level--;
-	    ERS(NCE_UNABLE_TO_OPEN_FILE,eblock->o_name);
 	    return INTERP_ERROR;
 	}
 
@@ -605,6 +604,20 @@ int Interp::control_back_to( block_pointer block, // pointer to block
         }
         strncpy(settings->filename, newFileName, sizeof(settings->filename));
     } else {
+	// No file found for this sub name.
+	// Block forward-seek if the current file is not the main program.
+	// Numbered subs after a named endsub pollute the global offset
+	// map and silently conflict across files.  Forward-seek in the
+	// main program's own file (call_level 0) is still allowed.
+	if (settings->call_level > 0) {
+	    context_pointer main_frame = &settings->sub_context[0];
+	    if (main_frame->filename == NULL ||
+		strcmp(settings->filename, main_frame->filename) != 0) {
+		ERS(_("Subroutine 'O%s' not found -- "
+		      "not in offset table and no file '%s' found"),
+		    block->o_name, newFileName);
+	    }
+	}
 	char *dirname = getcwd(NULL, 0);
 	logOword("fopen: |%s| failed CWD:|%s|", newFileName,
 		 dirname);
@@ -792,6 +805,14 @@ int Interp::convert_control_functions(block_pointer block, // pointer to a block
 	// if the level is not zero, this is a call
 	// not the definition
 	if (settings->call_level != 0) {
+	    // Check for nested or extra sub definitions inside a called sub.
+	    // Only the entry sub (matching the call) is allowed; any other
+	    // 'sub' keyword is an error (nested def or numbered sub in file).
+	    context_pointer cf = &settings->sub_context[settings->call_level];
+	    CHKS((cf->subName && strcmp(cf->subName, block->o_name) != 0),
+		 _("Nested subroutine definition: 'O%s sub' found inside "
+		   "called subroutine 'O%s'"),
+		 block->o_name, cf->subName);
 	    logOword("call:%f:%f:%f",
 		     settings->parameters[1],
 		     settings->parameters[2],

src/emc/rs274ngc/interp_read.cc

@@ -1716,6 +1716,18 @@ int Interp::read_o(    /* ARGUMENTS                                     */
     {
 	// Check we're not already defining a main- or sub-program
 	CHKS((_setup.defining_sub == 1), NCE_NESTED_SUBROUTINE_DEFN);
+
+	// Check for sub definition inside a called subroutine.
+	// When call_level > 0 and not seeking (skipping_o == NULL),
+	// hitting a sub that doesn't match the current call is an error.
+	if (_setup.call_level > 0 && _setup.skipping_o == NULL) {
+	    const char *current_sub =
+		_setup.sub_context[_setup.call_level].subName;
+	    CHKS((current_sub && strcmp(current_sub, block->o_name) != 0),
+		 _("Nested subroutine definition: 'O%s sub' found inside "
+		   "called subroutine 'O%s'"),
+		 block->o_name, current_sub);
+	}
     }
   // in terms of execution endsub and return do the same thing
   else if ((block->o_type == O_endsub) || (block->o_type == O_return) ||

tests/interp/nested-sub-error/expected

@@ -0,0 +1,8 @@
+    1 N..... USE_LENGTH_UNITS(CANON_UNITS_MM)
+    2 N..... SET_G5X_OFFSET(1, 0.0000, 0.0000, 0.0000, 0.0000, 0.0000, 0.0000)
+    3 N..... SET_G92_OFFSET(0.0000, 0.0000, 0.0000, 0.0000, 0.0000, 0.0000)
+    4 N..... SET_XY_ROTATION(0.0000)
+    5 N..... SET_FEED_REFERENCE(CANON_XYZ)
+    6 N..... ON_RESET()
+    7 N..... COMMENT("Test: nested sub definition inside named sub should error")
+    8 N..... ON_RESET()

tests/interp/nested-sub-error/subs/nested.ngc

@@ -0,0 +1,7 @@
+o<nested> sub
+o100 sub
+(DEBUG, inner o100: #1 #2 #3)
+o100 endsub
+o100 call [4] [5] [6]
+o<nested> endsub
+M2

tests/interp/nested-sub-error/test.ini

@@ -0,0 +1,2 @@
+[RS274NGC]
+SUBROUTINE_PATH=subs

tests/interp/nested-sub-error/test.ngc

@@ -0,0 +1,3 @@
+(Test: nested sub definition inside named sub should error)
+o<nested> call [7] [8] [9]
+M2

tests/interp/nested-sub-error/test.sh

@@ -0,0 +1,4 @@
+#!/bin/bash -e
+# Nested sub definition inside a called named sub must produce an error.
+# Regression test for LinuxCNC/linuxcnc#3880.
+! rs274 -g -i test.ini test.ngc

tests/interp/nested-sub-in-file-error/expected

@@ -0,0 +1,9 @@
+    1 N..... USE_LENGTH_UNITS(CANON_UNITS_MM)
+    2 N..... SET_G5X_OFFSET(1, 0.0000, 0.0000, 0.0000, 0.0000, 0.0000, 0.0000)
+    3 N..... SET_G92_OFFSET(0.0000, 0.0000, 0.0000, 0.0000, 0.0000, 0.0000)
+    4 N..... SET_XY_ROTATION(0.0000)
+    5 N..... SET_FEED_REFERENCE(CANON_XYZ)
+    6 N..... ON_RESET()
+    7 N..... COMMENT("Test: numbered sub after named endsub in same file should error")
+    8 N..... MESSAGE(" sequential main: 7.000000 8.000000 9.000000")
+    9 N..... ON_RESET()

tests/interp/nested-sub-in-file-error/subs/sequential.ngc

@@ -0,0 +1,9 @@
+o<sequential> sub
+(DEBUG, sequential main: #1 #2 #3)
+o200 call [4] [5] [6]
+o<sequential> endsub
+
+o200 sub
+(DEBUG, o200 helper: #1 #2 #3)
+o200 endsub
+M2