Indentation rules fake report due to missing environment label

[staerz]

Environment VSG 1.11.1 @ fedora 30

Describe the bug A missing label for a generate statement results in misleading reports of indentation rule violations.

To Reproduce Steps to reproduce the behavior:

1. Run VSG on the following code

library IEEE;
use IEEE.std_logic_1164.all;

entity fakeindent is
  generic (
    config  : integer := 1000
  port (
    clk            : in    std_logic;
    rst            : in    std_logic;
    sig_in         : in    std_logic := '1';
    sig_out        : out   std_logic := '0'
  );
end entity fakeindent;

architecture behavorial of fakeindent is

begin

  if config > 1 generate
    signal counter      : signed(config-2 downto 0);
  begin
    sig_out <= counter(config-2)
  else generate
    sig_out <= sig_in;
  end generate

end behavorial;

The following rules are affected:

• architecture_007 | 112 | Invalid indentation.
• architecture_008 | 148 | Invalid indentation.
• concurrent_001 | 146 | Invalid indentation.
• constant_001 | 103 | Invalid indentation.
• generate_001 | 180 | Invalid indentation.
• generate_007 | 182 | Invalid indentation.
• if_001 | 164 | Invalid indentation.
• if_012 | 120 | Invalid indentation.
• if_013 | 125 | Invalid indentation.
• if_014 | 128 | Invalid indentation.
• if_014 | 170 | Invalid indentation.
• process_001 | 158 | Invalid indentation.
• process_003 | 115 | Invalid indentation.
• process_006 | 144 | Invalid indentation.
• sequential_001 | 118 | Invalid indentation.
• signal_001 | 107 | Invalid indentation.

(not necessarily all of them in the example above)

Expected behavior I wouldn't expect these rule violations but instead a violation of the generate label missing.

Additional context To me it seems as if the rules for generate wouldn't be including the if EXPRESSION generate case.

[staerz]

Note: The fake warnings actually also show up when the signals would be declared in a process environment ...

[jeremiah-c-leary]

It looks like the LRM says the generate label is required: It also appears generate statements have evolved from the source I was using.

I could update VSG to handle this, but I am curious why a tool would ignore the LRM. Unless I am missing something.

[staerz]

Hi @jeremiah-c-leary,

just out of curiosity, where did you look up the LRM?

I can only say that Quartus and Questa both didn't complain about the missing label! I only realised it when asking myself why VSG complains that it is actually missing :rofl:

[jeremiah-c-leary]

I found the LRM at this location: http://www.eda-twiki.org/twiki/pub/P1076/PrivateDocuments/1076-2018-5C.pdf

I can only say that Quartus and Questa both didn't complain about the missing label!

so this is an interesting situation. I am not 100% sure how to proceed. Part of me says to stick with the LRM. Another part wants to match Intel/Mentor Graphics.

Opinion?

[staerz]

So then I'd say follow the IEEE rules.

I just looked up the 2008 version of the LRM and there also a label is required for each kind.

But - and this is what I meant with the Note in my second comment - VSG actually has a problem with the declarative part! That is, replace the if config > 1 generate by proc_label : process and try defining some variables etc. - VSG complains (at least it did on my side ...). In some cases, sure, one can move the declarative part one level up, but that's what is intended. Esp. sometimes you even cannot.

[jeremiah-c-leary]

I have not debugged this yet, but I am positive VSG thinks it is an if statement and then gets confused.

The statement is clearly a generate statement, whether it has a label or not. So I propose the following:

1) Update VSG to handle a generate without a label 2) Add a rule that says generates should have labels

[staerz]

That sounds very good to me!

[jeremiah-c-leary]

Hey @staerz ,

So with my new parser, if the generate label is not there then it will fail.

Error: Unknown token in architecture_body(20:3)
       Expecting end, found if

I wanted to revisit this statement I made earlier:

The statement is clearly a generate statement, whether it has a label or not. So I propose the following:

1. Update VSG to handle a generate without a label
2. Add a rule that says generates should have labels

I am not sure I still want to do this. At the moment my parser is pure, in that it follows the LRM. Should I put an exception in because Quartus and Questa allow it?

What are your thoughts?

Regards,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

sorry for the delayed reply, I have passed another very busy week with a firmware release just yesterday...

Then I'd say to go with the LRM!

What would though help is if you could improve the error message. It now really depends what can cause this error, but maybe one could make it a suggestive error message like "Expecting end or label, found if" - that would require that both, "end", or "label" would be the only 2 keywords expected here. Not sure if that's the case. Can your parser check what is behind the if? If it does and can find a "then" or "generate", then the error message could be further improved.

That's my 2 cents on that subject.

[jeremiah-c-leary]

Hey @staerz ,

I have passed another very busy week with a firmware release just yesterday

I hope everything went well.

Can your parser check what is behind the if? If it does and can find a "then" or "generate", then the error message could be further improved.

I will look into improving the error message. A couple of times I had a hard time figuring out what it was telling me. In the past, I complained about error messages being vague....now I know why.

[staerz]

It went well. Our project is still in a very early stage, so not much can go wrong, except that things are not delivered in time, but that's the usual business.

Yes, precise error messages are key. You don't want to know ...

[jeremiah-c-leary]

Hey @staerz ,

I put in a better error reporting message to catch missing labels on the if, for and case generate statements.

When you get a chance, could you check it out and let me know what you think?

Thanks,

--Jeremy

[jeremiah-c-leary]

Hey @staerz,

Wondering is this issue could be closed.

Thanks,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

alright, now, if I omit the label (in a if ... generate statement) is:

Error: Unexpected token detected while parsing if_generate_statement @ Line 127, Column 3
       Expecting : generate_label
       Found     : if

That is fairly self-explanatory and hence alright.

Note a minor detail that I found in the doc: I'd change the : in the generate_002 to colon. Same applies in for_loop_004.

[jeremiah-c-leary]

Hey @staerz ,

I fixed those two documentation issues and then searched the rest of the documentation and fixed the other occurances.

[staerz]

Hi @jeremiah-c-leary,

alright, great!

I think you can hence close this issue.

Happy New Year!

[jeremiah-c-leary]

Happy New Year!