Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  IPC::Locker
  Parallel::Forker
  Voneline
General Info
  Papers

Issue #1535

Double quotes in -f option file

Added by Yves Mathieu 2 months ago. Updated about 1 month ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Invoking/Options
% Done:

0%


Description

SystemVerilog allows string parameters. Using -f option file should support quoted strings.

Please find, attached a suggested patch for the support of double quote in -f file.

allow-double-quotes-in-option-file.patch View (1.46 KB) Yves Mathieu, 10/02/2019 02:49 PM

allow-strings-in-option-file.patch View (8.19 KB) Yves Mathieu, 10/18/2019 04:13 PM

allow-strings-in-option-file.patch View - Third version of the proposal (11 KB) Yves Mathieu, 10/23/2019 07:16 AM

allow-strings-in-option-file.patch View - Version 4 of the proposal (10.9 KB) Yves Mathieu, 10/24/2019 09:34 AM

History

#1 Updated by Wilson Snyder 2 months ago

  • Category set to Invoking/Options
  • Status changed from New to AskedReporter

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. "\""?

#2 Updated by Yves Mathieu 2 months ago

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

#3 Updated by Yves Mathieu 2 months ago

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 "

#4 Updated by Wilson Snyder 2 months ago

  • Status changed from AskedReporter to Assigned
  • Assignee set to Yves Mathieu

Great, awaiting next patch.

#5 Updated by Yves Mathieu about 2 months ago

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.

#6 Updated by Wilson Snyder about 2 months ago

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
+    // 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};
// Strip off arguments and parse into words

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.

#7 Updated by Yves Mathieu about 2 months ago

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.

#8 Updated by Wilson Snyder about 2 months ago

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.

#9 Updated by Yves Mathieu about 2 months ago

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)

#10 Updated by Wilson Snyder about 2 months ago

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?

#11 Updated by Yves Mathieu about 2 months ago

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.

#12 Updated by Wilson Snyder about 2 months ago

  • Status changed from Assigned to Resolved

Great, thanks for the work.

Pushed to git towards eventual 4.022 release.

#13 Updated by Wilson Snyder about 1 month ago

  • Status changed from Resolved to Closed

In 4.022.

Also available in: Atom