Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double quotes in -f option file #1535

Closed
veripoolbot opened this issue Oct 2, 2019 · 13 comments
Closed

Double quotes in -f option file #1535

veripoolbot opened this issue Oct 2, 2019 · 13 comments

Comments

@veripoolbot
Copy link
Contributor

@veripoolbot veripoolbot commented Oct 2, 2019


Author Name: Yves Mathieu
Original Redmine Issue: 1535 from https://www.veripool.org

Original Assignee: Yves Mathieu


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.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 2, 2019


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

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 2, 2019


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/)

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 2, 2019


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 "

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 6, 2019


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


Great, awaiting next patch.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 18, 2019


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.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 18, 2019


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.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 23, 2019


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.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 23, 2019


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.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 24, 2019


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.

  1. No need to call reserve before a push_back.

Ok (modified)

  1. 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)

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 24, 2019


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?

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 24, 2019


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.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 24, 2019


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.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Nov 10, 2019


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


In 4.022.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.