Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-02T15:07:42Z

Thanks for your issue & looking at how to fix this.

As the old comment noted, most other simulators don't support this. Which big-3 simulator respects quotes? I don't think Verilator should be on its own in how they are handled.

Also if your doing this to support e.g. spaces in directory names, note that will break "make".

As to the patch itself, assuming there's another simulator that supports this:

1. Can you please post a comment saying you acknowledge this and future contributions are made under the Developer Certificate of Origin (https://developercertificate.org/). (Needed just once; for other options see docs/CONTRIBUTING.adoc.)

2. Think we'll need a warning as this isn't portable.

3. Please provide a test.

4. Do quoted quotes need handling, i.e. do other simulators allow e.g. """?

Original Redmine Comment
Author Name: Yves Mathieu
Original Date: 2019-10-02T16:01:31Z

I acknowledge this and future contributions are made under the Developer Certificate of Origin (https://developercertificate.org/)

Original Redmine Comment
Author Name: Yves Mathieu
Original Date: 2019-10-02T16:15:31Z

Hi Wilson,

As for big-3 simulators, i have tested the following cases:

+define+CHAIN1=\"ABCD\"
+define+CHAIN2=\"AB\ C\"
+define+CHAIN3=\"AB\\\"C\"

CHAIN1 is a simple string,

CHAIN2 contains a space,

CHAIN3 contains an escaped double quote.

Simulator 1 supports the 3 chains.

Simulator 2 only supports simple strings (CHAIN1)

Simulator 3 supports, CHAIN1 and CHAIN2, but doesn't support escaped double quote.

Verilator could support a least simple chains without spaces, nor escaped quotes while being compatible whith other simulators.

So I need to modify my patch to look for " instead of "

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-06T14:56:21Z

Great, awaiting next patch.

Original Redmine Comment
Author Name: Yves Mathieu
Original Date: 2019-10-18T16:22:29Z

Please find here an updated proposal based on further investigations on the big-3 simulators.

First of all the "string parameter" may be transmitted to the simulator using two methods:

• A standard "+define+" option
• A simulator specific way of overriding a parameter (no standard option)

Furthermore we have to distinguish 2 cases for "string parameter" transmission:

• Through the command line (the shell will pre-process the string before transmission to the
  compiler)
• Through a file (the shell doesn't preprocess the string)

As regards "quoting" we have also to take into account integer definitions using annotations like : 32'h

As regards the "+define+" solution on the command line the following table summarize
some obtained cases for the big-3 tools and the current verilator version:

| On the command line       | simulator 1 | simulator 2 | simulator 3 | verilator   |
|---------------------------|-------------|-------------|-------------|-------------|
| +define+C0='"AB CD"'      | AB CD       | AB CD       | AB CD       | AB CD       |
| +define+C1=\"AB\ CD\"     | AB CD       | AB CD       | AB CD       | AB CD       |
| +define+C2="\"AB CD\""    | AB CD       | AB CD       | AB CD       | AB CD       |
| +define+C3="\"AB\ CD\""   | AB CD       | AB CD       | AB CD       | AB CD       |
| +define+C4=32'h600D600D   | UNSUPPORTED | UNSUPPORTED | UNSUPPORTED | UNSUPPORTED | (1)
| +define+C5=32\'h600D600D  | 32'h600D600D| 32'h600D600D| 32'h600D600D| 32'h600D600D| 
| +define+C6="32'h600D600D" | 32'h600D600D| 32'h600D600D| 32'h600D600D| 32'h600D600D| 
(1) Only one simple "'" on the commande line is an opened string...

Has regards the same "+define+" solution through the "-f" option files, the results are
as follows:

| In the options file       | simulator 1 | simulator 2 | simulator 3 | verilator   |
|----------------------- ---|-------------|-------------|-------------|-------------|
| +define+C0='"AB CD"'      | AB CD       | UNSUPPORTED | AB CD       | UNSUPPORTED |
| +define+C1=\"AB\ CD\"     | AB CD       | UNSUPPORTED | AB CD       | UNSUPPORTED |
| +define+C2="\"AB CD\""    | AB CD       | AB CD       | UNSUPPORTED | UNSUPPORTED |
| +define+C3="\"AB\ CD\""   | AB CD       | AB CD       | UNSUPPORTED | UNSUPPORTED |
| +define+C4=32'h600D600D   | UNSUPPORTED | 32'h600D600D| 32'h600D600D| 32'h600D600D| (2)
| +define+C5=32\'h600D600D  | 32'h600D600D| UNSUPPORTED | UNSUPPORTED | UNSUPPORTED |
| +define+C6="32'h600D600D" | 32'h600D600D| 32'h600D600D| 32'h600D600D| UNSUPPORTED |
(2) Only simulator 1 has a behavior equivalent to the command line version

Has regards the direct overriding of a parameters, the exact syntax differs from one
simulator to the other.
Simulator 3 seems to support only a specific parameter file
(no direct support on command line)

| On the command line | simulator 1 | simulator 2 | simulator 3 | verilator   |
|---------------------|-------------|-------------|-------------|-------------|
| -gC0='"AB CD"'      | AB CD       | AB CD       | UNSUPPORTED | AB CD       |
| -gC1=\"AB\ CD\"     | AB CD       | UNSUPPORTED | UNSUPPORTED | AB CD       |
| -gC2="\"AB CD\""    | AB CD       | AB CD       | UNSUPPORTED | AB CD       |
| -gC3="\"AB\ CD\""   | AB\ CD      | AB\\ CD     | UNSUPPORTED | AB\ CD      |
| -gC4=32'h600D600D   | UNSUPPORTED | UNSUPPORTED | UNSUPPORTED | UNSUPPORTED |  (1)
| -gC5=32\'h600D600D  | 32'h600D600D| UNSUPPORTED | UNSUPPORTED | 32'h600D600D|
| -gC6="32'h600D600D" | 32'h600D600D| 32'h600D600D| UNSUPPORTED | 32'h600D600D|
(1) Only one simple "'" on the commande line is an opened string...

Parameters overriding may be done either through the "-f" option file either
from a simulator specific parameter file.

| Option/Param file   | simulator 1 | simulator 2 | simulator 3 | verilator   |
|---------------------|-------------|-------------|-------------|-------------|
| -gC0='"AB CD"'      | AB CD       | UNSUPPORTED | AB CD       | UNSUPPORTED |
| -gC1=\"AB\ CD\"     | AB CD       | UNSUPPORTED | UNSUPPORTED | UNSUPPORTED |
| -gC2="\"AB CD\""    | AB CD       | AB CD       | AB CD       | UNSUPPORTED |
| -gC3="\"AB\ CD\""   | AB CD       | AB\\ CD     | AB CD       | UNSUPPORTED |
| -gC4=32'h600D600D   | UNSUPPORTED | 32'h600D600D| 32'h600D600D| 32'h600D600D| 
| -gC5=32\'h600D600D  | 32'h600D600D| UNSUPPORTED | UNSUPPORTED | 32'h600D600D|
| -gC6="32'h600D600D" | 32'h600D600D| 32'h600D600D| UNSUPPORTED | UNSUPPORTED |

As a conclusion, verilator behavior is very close to "simulator 1" behavior for string parameters defined in the command line.

I propose to choose the same behavior for the "-f" option file, while keeping already
supported number forms (32'h...).

Please find here a patch including the V3Options.cpp source file as well as updated non regression tests t_flag_parameters and t_flag_define.

I also changed the error message, to info messages.

I didn't notice any new error in other non regression tests.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-18T22:30:12Z

Thanks for the great analysis. I don't see that there's anything at all
special about numbers, they are just normal string substitution. (With '
needing quoting due to the shell.)

+++ b/src/V3Options.cpp


     // Strip off arguments and parse into words
+    // parse file using a state machine, taking into account quoted strings and escaped chars
+    enum state {search_option, in_option, escaped_char, in_quoted_string, in_double_quoted_string};

Use Upper for enum names, ALL_CAPS for enum values, with common prefix. e.g.

enum State {ST_SEARCH_OP, ST_IN_OPT, ...


+    state st = search_option ;

Fix style: Remove spaces before semicolons. Two spaces before //
comment. Single space after if/switch/for.

+               if(curr_char == '=') {  // get an argument

I don't see why this code needs to parse the =. The extracted argument (including the equal)
should be passed through to the option parser, that will extract the =.

+    if (arg.length() != 0) {  // add last word

Faster to use:

if (!arg.empty() ...


+  fl->v3warn(EC_INFO,
+          "Simple quoted strings in -f files may cause unexpected behavior");

This would need to be a new warning code (in V3Error.h). Alternatively if
you want, we can just remove the message.

+++ b/test_regress/t/t_flag_define.v

Tests looks good. In t_flag_parameter.v I suggest you also paste (in comments) the
result of your experiments, with the verilator column fixed to represent the new behavior.
That way if the future we wonder why it does what it does we'll have the reference.

Original Redmine Comment
Author Name: Yves Mathieu
Original Date: 2019-10-23T07:18:57Z

New proposal updated taking into account your last post.

• fixed style, upercase, ...
• removed parsing of '='
• comments inside the non regressions tests.

As regards the parsing of '=' it was an attempt to distinguish the two following cases:

'"bla bla"'
32'h1234ABCD

• In the first case <'> should be filtered out before sending the arg to the parser.
• In the second case <'> should be sent to the parser.
• I solved the problem by detecting a <'"> sequence.

I updated also the non regression tests (t_flag_defines.v) in order check the different forms of base specified literal constants.

I propose to suppress any "warning" message, as this code supports all cases of well formed strings. It's up to the parser to detect any incorrect option.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-23T12:26:24Z

Looking good.

1. You don't check for escaped chars inside single quotes. I might have missed it but didn't see that in your experiments - can you see if that's correct?

2. No need to call reserve before a push_back.

3. Missing a break/fallthru in ST_SEARCH_OPTION when if is true. Actually I suspect you don't need this state at all, as IN_OPTION will work to remove leading spaces.

Original Redmine Comment
Author Name: Yves Mathieu
Original Date: 2019-10-24T10:56:53Z

Wilson Snyder wrote:

Looking good.

1. You don't check for escaped chars inside single quotes. I might have missed it but didn't see that in your experiments - can you see if that's correct?

Yes that's correct: shells (sh, zsh, bash, csh...) don't check escaped chars inside single quotes, all inside chars are transmitted. The proposed code has the same behavior. Furthermore, as I tried to be compatible with the current command line behavior of Verilator, strings like in the following example are not supported.

-gC7='ABCD'

As a result simple quotes are only used to protect double quote strings, and to avoid escaping chars for readability.

I added an example in the tables of the non regression tests.

2. No need to call reserve before a push_back.

Ok (modified)

3. Missing a break/fallthru in ST_SEARCH_OPTION when if is true. Actually I suspect you don't need this state at all, as IN_OPTION will work to remove leading spaces.

Yes, this state is not usefull (modified)

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-24T11:14:05Z

I think good to go (with a few trivial space issues I'll fix). May I use the email you registered under for the git commit or do you prefer something else?

Original Redmine Comment
Author Name: Yves Mathieu
Original Date: 2019-10-24T11:17:38Z

Wilson Snyder wrote:

I think good to go (with a few trivial space issues I'll fix). May I use the email you registered under for the git commit or do you prefer something else?

That's ok for the registered mail.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-24T11:33:53Z

Great, thanks for the work.

Pushed to git towards eventual 4.022 release.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-10T19:28:16Z

In 4.022.