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

frontend: json: parse negative values #2567

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

kgugala
Copy link
Contributor

@kgugala kgugala commented Jan 27, 2021

When scc breaks logic loops with $__ABC9_SCC_BREAKER module it adds the following entry to resulting json:

  "attributes": {
    "blackbox": "00000000000000000000000000000001",
    "cells_not_processed": "00000000000000000000000000000001",
    "dynports": "00000000000000000000000000000001",
    "src": "/(...)/share/yosys/abc9_model.v:9.1-11.10"
  },  
  "cells": {}, 
  "netnames": {
    "I": {
      "attributes": {
        "src": "/(...)/share/yosys/abc9_model.v:9.47-9.48"
      },  
      "bits": [
        2,  
        3   
      ],  
      "hide_name": 0,
      "offset": -1, 
      "upto": 1
    }, 

reading the json back to yosys results in:

ERROR: Unexpected character in JSON file: '-'

in the "offset": -1, line

This PR fixes that

Signed-off-by: Karol Gugala <kgugala@antmicro.com>
Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens if the input is something like 123-456?

@kgugala
Copy link
Contributor Author

kgugala commented Jan 27, 2021

So what happens if the input is something like 123-456?

It will break here https://github.com/YosysHQ/yosys/blob/master/frontends/json/jsonparse.cc#L91

@whitequark
Copy link
Member

Actually, is this even the right approach? Is there a (* force_downto *) missing somewhere, like in #2058? cc @mwkmwkmwk

@Ravenslofty
Copy link
Collaborator

I think we should ping @eddiehung in here too, since this is from the eddie/abc9_scc_fixes2 branch, and it would be a good idea to roll it up into his next round of ABC9 fixes.

@mwkmwkmwk
Copy link
Member

Actually, is this even the right approach? Is there a (* force_downto *) missing somewhere, like in #2058? cc @mwkmwkmwk

Right or not, negative offsets are valid in Verilog either way, so this patch should be merged regardless.

@mwkmwkmwk
Copy link
Member

As in, currently converting the following to JSON and trying to read it back just fails:

module top(input [3:-3] I);
endmodule

data_number = ch - '0';
if (ch == '-') {
data_number = 0;
negative = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace error.

@mwkmwkmwk mwkmwkmwk merged commit cc7d18d into YosysHQ:master Feb 22, 2021
@eddiehung
Copy link
Collaborator

I'm not sure the $__ABC9_SCC_BREAKER module nor abc9 is relevant here. For one, I think other modules can trigger negative indices, plus, that cell should never leak into the outside world as it's an internal cell used between sub-passes.

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.

None yet

5 participants