Unify handling of quoted strings in pass args

kernel/io.cc

@@ -2,6 +2,7 @@
 #include "kernel/log.h"
 #include <iostream>
 #include <string>
+#include <iomanip>
 
 #if !defined(WIN32)
 #include <dirent.h>
@@ -37,15 +38,16 @@ std::string next_token(std::string &text, const char *sep, bool long_strings)
 	if (pos_begin == std::string::npos)
 		pos_begin = text.size();
 
-	if (long_strings && pos_begin != text.size() && text[pos_begin] == '"') {
+	if (long_strings && pos_begin != text.size() && (text[pos_begin] == '"' || text[pos_begin] == '\'')) {
 		std::string sep_string = sep;
 		for (size_t i = pos_begin+1; i < text.size(); i++) {
-			if (text[i] == '"' && (i+1 == text.size() || sep_string.find(text[i+1]) != std::string::npos)) {
+			bool close_quote = (text[i] == text[pos_begin]) && (text[i-1] != '\\' || text[pos_begin] == '\'');
+			if (close_quote && (i+1 == text.size() || sep_string.find(text[i+1]) != std::string::npos)) {
 				std::string token = text.substr(pos_begin, i-pos_begin+1);
 				text = text.substr(i+1);
 				return token;
 			}
-			if (i+1 < text.size() && text[i] == '"' && text[i+1] == ';' && (i+2 == text.size() || sep_string.find(text[i+2]) != std::string::npos)) {
+			if (i+1 < text.size() && close_quote && text[i+1] == ';' && (i+2 == text.size() || sep_string.find(text[i+2]) != std::string::npos)) {
 				std::string token = text.substr(pos_begin, i-pos_begin+1);
 				text = text.substr(i+2);
 				return token + ";";
@@ -592,4 +594,34 @@ void format_emit_void_ptr(std::string &result, std::string_view spec, int *dynam
 	format_emit_stringf(result, spec, dynamic_ints, num_dynamic_ints, arg);
 }
 
+bool needs_quote(const std::string &s) {
+	for (auto c : {' ', '\\', '#', ';', '"', '\''}) {
+		if (s.find(c) != std::string::npos) return true;
+	}
+	return false;
+}
+
+std::string quote(const std::string &s) {
+	std::ostringstream ss;
+	if (s.find('\'') != std::string::npos)
+		ss << std::quoted(s);
+	else
+		ss << '\'' << s << '\'';
+	return ss.str();
+}
+
+std::string unquote(const std::string &s) {
+	if (s.length() >= 2) {
+		if (s.front() == '\'' && s.back() == '\'')
+			return s.substr(1, s.length()-2);
+		else if (s.front() == '"' && s.back() == '"') {
+			std::string result;
+			std::istringstream ss(s);
+			ss >> std::quoted(result);
+			return result;
+		}
+	}
+	return s;
+}
+
 YOSYS_NAMESPACE_END

kernel/io.h

@@ -384,7 +384,7 @@
 		// Collect another int for the dynamic width precision/args
 		// for the next format conversion specifier.
 		if constexpr (std::is_convertible_v<Arg, int>) {
 			dynamic_ints[static_cast<uint8_t>(num_dynamic_ints)] = arg;
 		} else {
 			YOSYS_ABORT("Internal error");
 		}
@@ -470,6 +470,10 @@
 bool create_directory(const std::string& dirname);
 std::string escape_filename_spaces(const std::string& filename);
 
+bool needs_quote(const std::string &s);
+std::string quote(const std::string &s);
+std::string unquote(const std::string &s);
+
 YOSYS_NAMESPACE_END
 
 #endif // YOSYS_IO_H

kernel/register.cc

@@ -222,15 +222,15 @@ void Pass::call(RTLIL::Design *design, std::string command)
 			while (!tok.empty() && tok.back() == ';')
 				tok.resize(tok.size()-1), num_semikolon++;
 			if (!tok.empty())
-				args.push_back(tok);
+				args.push_back(unquote(tok));
 			call(design, args);
 			args.clear();
 			if (num_semikolon == 2)
 				call(design, "clean");
 			if (num_semikolon == 3)
 				call(design, "clean -purge");
 		} else
-			args.push_back(tok);
+			args.push_back(unquote(tok));
 		bool found_nl = false;
 		for (auto c : cmd_buf) {
 			if (c == ' ' || c == '\t')
@@ -256,8 +256,10 @@ void Pass::call(RTLIL::Design *design, std::vector<std::string> args)
 
 	if (echo_mode) {
 		log("%s", create_prompt(design, 0));
-		for (size_t i = 0; i < args.size(); i++)
-			log("%s%s", i ? " " : "", args[i]);
+		for (size_t i = 0; i < args.size(); i++) {
+			auto maybe_quoted = needs_quote(args[i]) ? quote(args[i]) : args[i];
+			log("%s%s", i ? " " : "", maybe_quoted);
+		}
 		log("\n");
 	}
 

passes/cmds/bugpoint.cc

@@ -149,9 +149,6 @@ struct BugpointPass : public Pass {
 		if (grep.empty())
 			return true;
 
-		if (grep.size() > 2 && grep.front() == '"' && grep.back() == '"')
-			grep = grep.substr(1, grep.size() - 2);
-
 		string bugpoint_file = "bugpoint-case";
 		if (suffix.size())
 			bugpoint_file += stringf(".%.8s", suffix);

passes/cmds/logger.cc

@@ -106,7 +106,6 @@ struct LoggerPass : public Pass {
 			}
 			if (args[argidx] == "-warn" && argidx+1 < args.size()) {
 				std::string pattern = args[++argidx];
-				if (pattern.front() == '\"' && pattern.back() == '\"') pattern = pattern.substr(1, pattern.size() - 2);		
 				try {
 					log("Added regex '%s' for warnings to warn list.\n", pattern);
 					log_warn_regexes.push_back(YS_REGEX_COMPILE(pattern));
@@ -118,7 +117,6 @@ struct LoggerPass : public Pass {
 			}
 			if (args[argidx] == "-nowarn" && argidx+1 < args.size()) {
 				std::string pattern = args[++argidx];
-				if (pattern.front() == '\"' && pattern.back() == '\"') pattern = pattern.substr(1, pattern.size() - 2);	
 				try {
 					log("Added regex '%s' for warnings to nowarn list.\n", pattern);
 					log_nowarn_regexes.push_back(YS_REGEX_COMPILE(pattern));
@@ -130,7 +128,6 @@ struct LoggerPass : public Pass {
 			}
 			if (args[argidx] == "-werror" && argidx+1 < args.size()) {
 				std::string pattern = args[++argidx];
-				if (pattern.front() == '\"' && pattern.back() == '\"') pattern = pattern.substr(1, pattern.size() - 2);	
 				try {
 					log("Added regex '%s' for warnings to werror list.\n", pattern);
 					log_werror_regexes.push_back(YS_REGEX_COMPILE(pattern));
@@ -164,7 +161,6 @@ struct LoggerPass : public Pass {
 				if ((type=="error" || type=="prefix-error") && log_expect_error.size()>0)
 					log_cmd_error("Only single error message can be expected !\n");
 				std::string pattern = args[++argidx];
-				if (pattern.front() == '\"' && pattern.back() == '\"') pattern = pattern.substr(1, pattern.size() - 2);
 				int count = atoi(args[++argidx].c_str());
 				if (count<=0)
 					log_cmd_error("Number of expected messages must be higher then 0 !\n");

passes/cmds/setenv.cc

@@ -46,7 +46,6 @@ struct SetenvPass : public Pass {
 
 		std::string name = args[1];
 		std::string value = args[2];
-		if (value.front() == '\"' && value.back() == '\"') value = value.substr(1, value.size() - 2);
 
 #if defined(_WIN32)
 		_putenv_s(name.c_str(), value.c_str());

tests/scripts/.gitignore

@@ -0,0 +1 @@
+/plugin.so

tests/scripts/file name.v

@@ -0,0 +1,2 @@
+module top();
+endmodule

tests/scripts/file name.ys

@@ -0,0 +1 @@
+log Hello!

tests/scripts/plugin.cc

@@ -0,0 +1,57 @@
+#include "kernel/yosys.h"
+
+USING_YOSYS_NAMESPACE
+PRIVATE_NAMESPACE_BEGIN
+
+struct TestArgsPass : public Pass {
+    TestArgsPass() : Pass("test_args", "dummy pass to test arg parsing") {
+        internal();
+    }
+    void execute(std::vector<std::string> args, RTLIL::Design*) override {
+        int argidx;
+        for (argidx = 0; argidx < GetSize(args); argidx++)
+        {
+            log("%s\n", args[argidx]);
+        }
+    }
+} TestArgsPass;
+
+struct TestArgsFrontend : public Frontend {
+    TestArgsFrontend() : Frontend("test_args", "dummy frontend to test arg parsing") {
+        internal();
+    }
+    void execute(std::istream *&f, std::string filename, std::vector<std::string> args, RTLIL::Design *) override {
+        int argidx;
+        log("pass: %s\n", args[0]);
+        for (argidx = 1; argidx < GetSize(args); argidx++) {
+            if (args[argidx] == "-arg" && argidx+1 < GetSize(args)) {
+                log("arg: %s\n", args[++argidx]);
+                continue;
+            }
+            break;
+        }
+		extra_args(f, filename, args, argidx);
+        log("filename: %s\n", filename);
+    }
+} TestArgsFrontend;
+
+struct TestArgsBackend : public Backend {
+    TestArgsBackend() : Backend("test_args", "dummy backend to test arg parsing") {
+        internal();
+    }
+    void execute(std::ostream *&f, std::string filename,  std::vector<std::string> args, RTLIL::Design *) override {
+        int argidx;
+        log("pass: %s\n", args[0]);
+        for (argidx = 1; argidx < GetSize(args); argidx++) {
+            if (args[argidx] == "-arg" && argidx+1 < GetSize(args)) {
+                log("arg: %s\n", args[++argidx]);
+                continue;
+            }
+            break;
+        }
+		extra_args(f, filename, args, argidx);
+        log("filename: %s\n", filename);
+    }
+} TestArgsBackend;
+
+PRIVATE_NAMESPACE_END

tests/scripts/run-test.sh

@@ -0,0 +1,4 @@
+#!/usr/bin/env bash
+set -eu
+source ../gen-tests-makefile.sh
+generate_mk --bash

tests/scripts/space_in_name.sh

@@ -0,0 +1,47 @@
+#!/usr/bin/env bash
+set -eu
+
+yosys="$PWD/../../yosys"
+
+# these ones are fine because bash handles it
+$yosys "file name.ys"
+$yosys file\ name.ys
+
+$yosys "file name.v" -o "file name.out" -b verilog
+$yosys file\ name.v -o file\ name.out -b verilog
+
+# these already have special handling in Yosys thanks to `extra_args`
+$yosys -p 'read_verilog "file name.v"'
+$yosys -p 'write_verilog "file name.out"'
+
+# this one works if passes get their arguments unquoted
+$yosys -p 'script "file name.ys"'
+
+# these get split by space and treated as two separate filenames
+# $yosys -p script\ "file name.ys"
+# $yosys -p script\ file\ name.ys
+# $yosys -p read_verilog\ "file name.v"
+# $yosys -p read_verilog\ file\ name.v
+# $yosys -p write_verilog\ file\ name.out
+# $yosys -p write_verilog\ "file name.out"
+
+# what does test_args say
+rm -f plugin.so
+CXXFLAGS=$(../../yosys-config --cxxflags)
+DATDIR=$(../../yosys-config --datdir)
+DATDIR=${DATDIR//\//\\\/}
+CXXFLAGS=${CXXFLAGS//$DATDIR/..\/..\/share}
+../../yosys-config --exec --cxx ${CXXFLAGS} --ldflags -shared -o plugin.so plugin.cc
+yosys_plugin="$yosys -m ./plugin.so"
+
+$yosys_plugin -p test_args\ "quoted spaces"
+$yosys_plugin -p test_args\ escaped\ spaces
+$yosys_plugin -p test_args\ \"escaped\ quotes\"
+$yosys_plugin -p 'test_args "inner quotes"'
+$yosys_plugin -p 'test_args "inner \"escaped quotes\""'
+
+$yosys_plugin -p 'read_test_args "file name.v" "file name.ys"'
+$yosys_plugin -p 'write_test_args "file name.out"'
+
+# and as a script
+$yosys_plugin space_in_name.ys

tests/scripts/space_in_name.ys

@@ -0,0 +1,104 @@
+logger -expect-no-warnings
+
+# quoted strings are a single argument to passes
+logger -expect log "quoted space" 1
+logger -expect log "quoted" 2
+logger -expect log "space" 2
+logger -expect log '\\"escaped' 1
+logger -expect log 'quotes\\"' 1
+
+test_args unquoted space
+test_args "quoted space"
+test_args \"escaped quotes\"
+
+logger -check-expected
+
+# empty strings can be arguments
+logger -expect log "a   b  c" 2
+
+log a " " b "" c
+log a ' ' b '' c
+
+logger -check-expected
+
+# quotes can be arguments
+logger -expect log '^"' 2
+logger -expect log "^'" 1
+test_args "'" '"' "\""
+logger -check-expected
+
+# whitespace (or a lack thereof) shouldn't break things
+logger -expect log 'a. .b' 2
+log a" "b
+log a' 'b
+
+logger -expect log '.a.b c' 2
+log "a"b c
+log 'a'b c
+
+logger -check-expected
+
+# numeric literals don't need quotes (unless they include a space)
+logger -expect log "1'd2" 1
+logger -expect log "3'b 011" 1
+
+test_args 1'd2
+test_args "3'b 011"
+
+logger -check-expected
+
+# sidenote that apparently logger regexp ends with '\n$', but \n in the pattern is rejected
+logger -expect log "^('|\").$" 2
+log "
+log '
+
+logger -check-expected
+
+# bonus test
+test_args -opt "some value here" -b "some other \"escaped value\""
+
+# special characters can appear in strings
+logger -expect log "#no comment" 2
+logger -expect log ";" 1
+logger -warn "a comment"
+
+log "this is #no comment"
+log this is #a comment
+log "semicolon; "; log "#no comment"
+
+logger -check-expected
+
+# special characters are quoted in echo
+logger -expect log "'#'" 1
+logger -expect log "';'" 1
+logger -expect log "' '" 1
+logger -expect log "\"'\"" 1
+logger -expect log "'\"'" 1
+logger -expect log '.\\.' 1
+
+echo on
+log '#'
+log ';'
+log ' '
+log "'"
+log '"'
+log '\'
+echo off
+
+logger -check-expected
+
+# should this be a backslash or an escaped space?
+log escaped\ space?
+
+# frontend/backend args also work as expected
+logger -expect log "arg: inner \"escaped quotes\"" 3
+logger -expect log 'filename: file name\..{1,3}' 3
+logger -expect log "filename: <stdout>" 1
+logger -expect log "arg: no_quotes" 2
+
+read_test_args -arg "inner \"escaped quotes\"" "file name.v" "file name.ys"
+write_test_args -arg "inner \"escaped quotes\"" "file name.out"
+read_test_args -arg no_quotes plugin.cc
+write_test_args -arg no_quotes
+
+logger -check-expected