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

Invalid xml output generated when code contains functions with string arguments #1444

Closed
veripoolbot opened this issue May 16, 2019 · 12 comments
Labels
area: wrong runtime result Issue involves an incorrect runtine result from Verilated model resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor


Author Name: Kanad Kanhere
Original Redmine Issue: 1444 from https://www.veripool.org

Original Assignee: Kanad Kanhere


For the attached code (topmod.sv), verilator (4.014) is generating invalid xml output (attached Vtopmod.xml).

The run command: ```verilator_bin -sv topmod.sv --xml-only --Mdir verilator_output --top-module topmod


On line 52 of file Vtopmod.xml, the xml statement reads
```<const fl="d20" name="\"invalid input during reset\"" dtype_id="2"/>

XML format needs '"' be used for quotes and not '"'

I took a shot at root-causing the issue and my investigation leads me to believe that the culprit is the putsQuoted() function in V3EmitXml.cpp which has a call ```putsNoTracking(V3Number::quoteNameControls(str));



@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-16T16:37:31Z


Your diagnosis sounds reasonable, perhaps you could provide a patch? Probably V3File's OutFormatter is the right spot to quote, or perhaps V3EmitXml.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Kanad Kanhere
Original Date: 2019-05-16T16:42:29Z


I can work on a fix. What I am not sure is whether

  • Add language argument to V3Number::quoteNameControls function
  • Create a new function in V3OutputFormatter - but this sounds like logic duplication

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-16T18:46:44Z


Thanks for taking this on & asking.

V3Number::quoteNameControls really is in the wrong place, I would suggest first make a patch that moves it to be a static function in V3OutFormatter::quoteNameControls that takes a language argument. (It looks like it's called from places where there isn't a File object to taking the language as parameter is easiest vs. making it a member function and having to construct lots of temp classes.) All tests should pass as-is.

Then second patch would change this function for XML - please also update the e.g. test_regress/t/t_xml_tag.v test to exercise this case.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Kanad Kanhere
Original Date: 2019-05-16T22:56:16Z


First patch to relocate quoteNameControls function from V3Number to V3OutFormatter is attached.

Please let me know if it needs correction/modification.

Regards

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-17T01:41:40Z


0001 looks good, I just changed some spacing to bring up to newer standard (no tabs). Pushed to git in 4.015.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Kanad Kanhere
Original Date: 2019-05-17T18:54:32Z


Patch for the fix is attached. Includes test update.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-17T21:37:36Z


Please add a \005 to the test and I suspect you'll see we need to also ampersand-number quote non-ascii similar to the C version.

We probably could use the &number style for special characters too, but people might find that odd. Either way.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Kanad Kanhere
Original Date: 2019-05-17T21:45:48Z


Do you mean U+0005 control character? My understanding is that it is not allowed in XML 1.0 [https://en.wikipedia.org/wiki/Valid_characters_in_XML#XML_1.0]
Or do you mean non-printable characters should be spelt out in XML.

Basically can you give a transfer function from ascii-code to xml string and I can certainly update the function for that.

Regards

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-17T22:27:03Z


From https://www.liquid-technologies.com/xml/escapingdata.aspx

says you can character escape unicode format in decimal (better) or hex, so � (ampersand pound 5 semi)

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Kanad Kanhere
Original Date: 2019-05-29T16:05:01Z


Apologies for the delay. I have attached the latest patch for the fix.
The change now handles all ascii values (except for the null character). I have also updated the test to check this.

NOTE: the 3.2.1 lxml python library didn't like ascii values 1-8,11,12,14-31. It complained about invalid character (e.g. "invalid xmlChar 2, ...")

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-29T22:45:20Z


Great, thanks for the effort on this.

Pushed to git towards 4.016.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-16T13:59:02Z


In 4.016.

@veripoolbot veripoolbot added area: wrong runtime result Issue involves an incorrect runtine result from Verilated model resolution: fixed Closed; fixed labels Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: wrong runtime result Issue involves an incorrect runtine result from Verilated model resolution: fixed Closed; fixed
Projects
None yet
Development

No branches or pull requests

1 participant