Skip to content

Remove quotes around Z3 arguments#588

Merged
fpoli merged 2 commits into
masterfrom
remove-quotes
Mar 25, 2022
Merged

Remove quotes around Z3 arguments#588
fpoli merged 2 commits into
masterfrom
remove-quotes

Conversation

@fpoli

@fpoli fpoli commented Jan 19, 2022

Copy link
Copy Markdown
Member

The quotation marks that Silicon requires around Z3 arguments are annoying but also unnecessary. Usually, the issue is that scripts don't correctly forward arguments with spaces or quotes, but if so it's the script that should be fixed.

This PR removes the requirement for quotes and fixes the silicon.sh script. I also attempted to fix the silicon.bat script (%* should preserve spaces and quotes and %CMD% was probably dropping all quotes), but I don't have a Windows OS to test it.

For reference, I tested the following ways to run Silicon and they all work fine:

  • From the sbt shell: run --z3ConfigArgs "model=true model_validate=true" prog.vpr
  • From bash: sbt "run --z3ConfigArgs \"model=true model_validate=true\" prog.vpr"
  • From bash: sbt 'run --z3ConfigArgs "model=true model_validate=true" prog.vpr'
  • From bash: ./silicon.sh --z3ConfigArgs "model=true model_validate=true" prog.vpr
  • From bash: ./silicon.sh --z3ConfigArgs 'model=true model_validate=true' prog.vpr

This PR is an alternative to #568.

This is a (small) breaking change for clients of Silicon that use --z3Args or --z3ConfigArgs.

@fpoli

fpoli commented Jan 19, 2022

Copy link
Copy Markdown
Member Author

By the way, https://www.shellcheck.net/ is great for checking bash scripts.

@mschwerhoff

Copy link
Copy Markdown
Contributor

Thank you! I'll try to find some time soon to test it on Windows, but not before next week.

Did you also test argument forwarding via sbt test, e.g. testOnly -- -n arithmetic.vpr -Dsilicon:z3ConfigArgs="model=true model_validate=true"?

@fpoli

fpoli commented Jan 20, 2022

Copy link
Copy Markdown
Member Author

Did you also test argument forwarding via sbt test, e.g. testOnly -- -n arithmetic.vpr -Dsilicon:z3ConfigArgs="model=true model_validate=true"?

Is that the exact command? It doesn't seem to work for me, not even on master.

On master, the following works in the sbt shell:

sbt:Silicon> testOnly -- -n arithmetic.vpr "-Dsilicon:z3ConfigArgs=\\"model=true model_validate=true\\""

With this PR, the previous doesn't work and one of the following commands must be used.

sbt:Silicon> testOnly -- -n arithmetic.vpr "-Dsilicon:z3ConfigArgs=model=true model_validate=true"
sbt:Silicon> testOnly -- -n arithmetic.vpr \"-Dsilicon:z3ConfigArgs=model=true model_validate=true\"

It turns out that this is the expected behavior of the scalatest runner. First, sbt splits the command line in a list of arguments. This seems to be done in a naive way, so -Dsilicon:z3ConfigArgs="model=true model_validate=true" is (incorrectly, IMO) parsed as two arguments and makes scalatest complain, but "-Dsilicon:z3ConfigArgs=model=true model_validate=true" is parserd as one argument. I couldn't pinpoint the exact sbt code that implements this step. Then, if an argument starts with -D scalatest splits that into a key and a value using the first = in the argument (source). So, our spaces luckily end up in the value, and Silicon correctly preserves the spaces of the value (source).

@mschwerhoff

Copy link
Copy Markdown
Contributor

Did you also test argument forwarding via sbt test, e.g. testOnly -- -n arithmetic.vpr -Dsilicon:z3ConfigArgs="model=true model_validate=true"?

Is that the exact command? It doesn't seem to work for me, not even on master.

I found that snippet in Silicon's wiki, I'm not surprised that the exact syntax no longer works.

On master, the following works in the sbt shell:

sbt:Silicon> testOnly -- -n arithmetic.vpr "-Dsilicon:z3ConfigArgs=\\"model=true model_validate=true\\""

With this PR, the previous sbt command incorrectly includes the quote at the beginning of the first key and at the end of the last value :-/ However, the following works fine (but it looks a bit odd):

sbt:Silicon> testOnly -- -n arithmetic.vpr "-Dsilicon:z3ConfigArgs=model=true model_validate=true"

I can live with that. It's probably a rather esoteric use-case anyway.

I'm now thinking that testOnly doesn't correctly parse arguments with quotes and spaces. Do you know if it's our code or sbt's code?

I guess everybody is involved in this syntax party: testOnly -- ... is due to sbt, the -D... bit is due to ScalaTest, the stuff after -D is then handled by us (Silver or Silicon).

@fpoli fpoli self-assigned this Jan 21, 2022
@fpoli

fpoli commented Jan 25, 2022

Copy link
Copy Markdown
Member Author

I just checked the command prompt on Windows 10 and pushed a fix:

  • silicon.bat --z3ConfigArgs="model=true model_validate=true" prog.vpr works fine.
  • silicon.bat --z3ConfigArgs "model=true model_validate=true" prog.vpr works fine.
  • silicon.bat "--z3ConfigArgs=model=true model_validate=true" prog.vpr works (surprisingly) fine.
  • silicon.bat --z3ConfigArgs 'model=true model_validate=true' prog.vpr and other attempts don't work.

@fpoli

fpoli commented Feb 7, 2022

Copy link
Copy Markdown
Member Author

The testOnly -- -n arithmetic.vpr -Dsilicon:z3ConfigArgs="model=true model_validate=true" that doesn't work in the Sbt shell is a bug of Sbt. It should be fixed in the future Sbt v1.6.3.

@fpoli

fpoli commented Mar 24, 2022

Copy link
Copy Markdown
Member Author

Any objection to merging this?

@mschwerhoff

Copy link
Copy Markdown
Contributor

Any objection to merging this?

Not from my side.

@fpoli

fpoli commented Mar 25, 2022

Copy link
Copy Markdown
Member Author

Great; I'll merge. I find really odd that users have to write things like --z3Args """""trace=true proof=true""""" :)

@fpoli fpoli merged commit 43ea415 into master Mar 25, 2022
@fpoli fpoli deleted the remove-quotes branch March 25, 2022 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants