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

@ * is valid and should not be excluded in ivtest #1315

Open
caryr opened this issue Jan 30, 2021 · 14 comments
Open

@ * is valid and should not be excluded in ivtest #1315

caryr opened this issue Jan 30, 2021 · 14 comments

Comments

@caryr
Copy link
Contributor

caryr commented Jan 30, 2021

I have looked in the standard and I find no place that excludes @ * (note the space between the two) from being valid. Sure it uses @* (no space) in the examples, but I can find nothing the requires this to be a single token. Commercial tools support the space between the tokens as well. I would like to remove these from the exclude list for ivtest if I can get confirmation. It looks like this was originally tagged by Naoya Hatta.

@caryr
Copy link
Contributor Author

caryr commented Jan 31, 2021

@dalance this looks like it was your addition.

@dalance
Copy link
Collaborator

dalance commented Jan 31, 2021

In the LRM, event_control is defined as below:

event_control ::=
   @ hierarchical_event_identifier
 | @ ( event_expression )
 | @*
...

This doesn't have space between @ and *.
So I think this is a single token as the same as == which can't be written like = =.

@caryr
Copy link
Contributor Author

caryr commented Jan 31, 2021

The difference is == is specifically called out as a token since it is an operator:

The types of lexical tokens in the language are as follows:
— White space
— Comment
Operator
— Number
— String literal
— Identifier
— Keyword

I can find nowhere in the standard that specifies @* is an operator other than the following:

Boldface-red characters denote reserved keywords, operators, and punctuation marks as a required
part of the syntax.

Which could apply because the characters are shown in the BNF as Boldface-red, but could equally apply to the individual characters. Which I believe is the correct interpretation because of the following:

An @ is declared as an operator.

Processes can wait for a named event to be triggered either via the @ operator or ...

And @* is declared as an implicit event expression not an implicit event operator.

The implicit event_expression, @*,...

If you can find anything other than your interpretation of the BNF to support this is a single token I would appreciate a reference. I really have no opinion other than I want to support this correctly and everything I can find implies @* is actually two different tokens and since commercial tools support it as two tokens I tend to believe that is the correct interpretation.

@dalance
Copy link
Collaborator

dalance commented Jan 31, 2021

In the LRM, there are some sequential symbols not operator:

.*
@@( block_event_expression )

I think these can't be separated by space too.
Additionally commercial tools don't support like below:

always @ 
  * begin
end

always @ /* comment */ * begin
end

I don't know why these tools take care of space specially.
But I think these should be supported too if @ * is valid.

@caryr
Copy link
Contributor Author

caryr commented Jan 31, 2021

Well .* is referred to as a token:

The .* token shall appear at most once in a list of port connections.

I have no comment on @@ since in the 2012 standard it only appears twice and only in the BNF.

Icarus handles @ * exactly as you describe and I agree it is odd that the commercial tools do not handle a comment or newline between the tokens, but do support arbitrary space/tab characters.

@caryr
Copy link
Contributor Author

caryr commented Jan 31, 2021

I just checked and the 2017 standard is the same for @@, just two BNF references. I also checked a number of PDF books I have and they do not mention @@ in any manner. Including the one on coverage.

@dalance
Copy link
Collaborator

dalance commented Jan 31, 2021

The "boldface-red" seems to have no special meaning.

This standard uses a minimal amount of color to enhance readability. The coloring is not essential and does
not affect the accuracy of this standard when viewed in pure black and white.

So I think module, =>, and @* can't be separated to each characters by space similarly.

I think there is possibility that this is a mistake of BNF definition.
But I think @ * is invalid in the current BNF.

@caryr
Copy link
Contributor Author

caryr commented Jan 31, 2021

I agree the BNF does not show a space, but is there anywhere else in the standard that indicates this should be a token/operator? I can find noting that says it should be and a number of places suggesting it is not, which I provided earlier.

It's completely obvious keywords and the arithmetic symbols should be considers tokens. I feel you are treading into the realm of logically fallacies claiming that because a keyword (module) and a mathematical operator (=>) do not allow space then @* should not allow spaces. This is true if @* is a token/operator, but if that was the case why is it not explicitly stated that @* is a token/operator? Further if @* is not a token then having space between the two operators is obviously correct. We can only apply the space is invalid rule once we have determined @* is a token/operator. We cannot use it while determining if @* is a token/operator.

Regarding the odd commercial simulator functionality. I assume this is because they are handling this in the lexor instead of in the parser which would likely give this different functionality. There are lots of reasons someone would have done this and likely does not indicate the space is invalid. Some times people do not think of the subtlety and it may have been easier to change the regex in the lexor instead of the actual parser to support the space.

@dalance
Copy link
Collaborator

dalance commented Jan 31, 2021

In my understanding, BNF don't take care whether the sequence in definition is keyword, operator or token. It is only character sequence.
For example. "DPI-C" is 7 sequential characters, and can't be separated to " DPI - C ".
Similarly module can't be separated, and @* can't be too.
If we should take care of the kind of each sequence, we can't get the strict syntax definition through BNF without symbol kind table.

@dalance
Copy link
Collaborator

dalance commented Jan 31, 2021

After discussion with my friends, I understand the problem is the missing of tokenize definition in LRM.
Now I think like below:

  • @* is valid
  • @ * may be valid, may not be valid
  • @ \n * and @ /* comment */ * should be valid if @ * is valid

I think @ * should be excluded if this is ambiguous definition because tools which don't support @ * can't be judged as incompliant with LRM.

@caryr
Copy link
Contributor Author

caryr commented Jan 31, 2021

"DPI-C" is a string literal which is specified to be a token and since the space character is significant in a string they cannot be added arbitrarily. Space characters can be arbitrarily added between tokens though they are not required between all tokens. I'm still looking for a reference that says @* is a token/operator. It is obvious @ and * by themselves are tokens. If that could be provided this discussion could be concluded.

I do not agree with your ignore/exclude anything with ambiguity in the standard stance, but if that works for your tool then great. What I have found is that users do not care what is actually in the standard. All they really care about is if their design, which was likely written on a commercial tool, can be handled by our tools. If it can't they move on instead of rewriting to be compliant with our tool. It does not matter if the difference is because the commercial tool choose to take a pessimistic implementation choice or even if it is not covered by the standard at all. If you look at the latest ivtest generate script you will find that most of the tests that are excluded have been because they use/demonstrate de facto-standard functionality. I'm all for putting @* in that category if needed, but it seems like it may actually be valid per the standard.

@dalance
Copy link
Collaborator

dalance commented Feb 1, 2021

I think whether each tool should cover de facto-standard functionality depend on the purpose of the tool.
I agree that simulator is expected to support the functionality for usability, but I want linter to check standard strictly because some commercial tools doesn't support ambiguous or de facto-standard behavior.
(If there is obvious incompliant behavior, EDA venders tend to fix it, but de facto-standard is not)

I don't know the stance of this project to ambiguity and de facto-standard. So I'll leave this to maintainers.

@dalance
Copy link
Collaborator

dalance commented Feb 1, 2021

I found the case in Accellera Mantis.
https://accellera.mantishub.io/view.php?id=1286

The case say that the current BNF shows the space between @ and * is not allowed and should be fixed.
So I'll follow the current BNF as for my tool, and fix it after the standard update because my tool aim for compliant with LRM including mistake.

@caryr
Copy link
Contributor Author

caryr commented Feb 1, 2021

Here is my summary of the mantis item. Shalom appears to support them as separate token by the subject @* should have a space ?, but given his Hebrew background maybe he should have swapped @* and should to turn it into a real question so I'm not certain how much to read into the subject wording. The comment Steven makes appears to be only regarding current implementations treating @(*) as four independent tokens and the complexity that creates with (* (the attribute opening token). Note the BNF is written as @ (*) which only shows a single space after the @.

I'll leave it to others to decide if this should be in sv-tests or not. I may reach out to people I know on the standardization committees to see if they have an opinion.

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

No branches or pull requests

2 participants