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

Overflow in CovPType with GHDL #43

Closed
eschmidscs opened this issue Jan 14, 2020 · 12 comments
Closed

Overflow in CovPType with GHDL #43

eschmidscs opened this issue Jan 14, 2020 · 12 comments

Comments

@eschmidscs
Copy link

eschmidscs commented Jan 14, 2020

Hi

I've encountered the following error message when using the coverage package with GHDL:
/usr/local/bin/ghdl:error: overflow detected
from: osvvm.coveragepkg.covptype.icoverindex at CoveragePkg.vhd:2072

As far as I understand the code, the overflow is intentionally?
However, I haven't found a way to tell GHDL that this is ok...

The tb below triggers this. It uses vunit, but should be easy to modify to run without.

library ieee;
context ieee.ieee_std_context;

library vunit_lib;
context vunit_lib.vunit_context;

library osvvm;
use osvvm.RandomPkg.all;
use osvvm.CoveragePkg.all;

entity tb_osvvm is
  generic (
    runner_cfg    : string  := runner_cfg_default
    );
end entity tb_osvvm;

architecture tb of tb_osvvm is

  shared variable cov_fill   : CovPType;
begin
  ------------------------------------------------------------------------------------------------------------------------
  -- Test process
  ------------------------------------------------------------------------------------------------------------------------
  p_main : process
    variable rnd      : RandomPType;
  begin
    test_runner_setup(runner, runner_cfg);
    show_all(display_handler);

    while test_suite loop
      if run("random") then
        cov_fill.AddBins(GenBin(0, 3));
        while true loop
          cov_fill.ICover(rnd.RandInt(0, 3));
          cov_fill.WriteBin;
        end loop;
      end if;
    end loop;

    info(to_string(get_checker_stat));
    test_runner_cleanup(runner);
  end process p_main;

end architecture tb;

Best regards,
emanuel

@JimLewis
Copy link
Member

JimLewis commented Jan 14, 2020

Type integer holds 2^31-1 values. You have exceeded that. Hence the GHDL error message.

In a VHDL language sense, your code is erroneous - meaning
it contains error that is not directly detected.

In an OSVVM sense, your test needs to end at some point.
Your condition, while true loop sets the test up to run forever.

OSVVM method IsCovered returns TRUE when each bin has
the number of expected values. By default, it expects each
bin to receive at least one value. You can achieve this by doing
the following. Note I also moved the WriteBin until the test is
done as otherwise in most simulators your printing will determine
how long your test takes.

      if run("random") then
        cov_fill.AddBins(GenBin(0, 3));
        loop
          cov_fill.ICover(rnd.RandInt(0, 3));
          exit when cov_fill.IsCovered ; 
        end loop;
        cov_fill.WriteBin;
      end if;

If you want to run longer, you can set a coverage goal
for each bin using the AtLeast parameter. Below I
set the coverage goal to 1000, but if you wished you
can also do something big like 2^30.

        cov_fill.AddBins(1000, GenBin(0, 3));

If you do 2^31, you will get a similar error message
since 2^31-1 is the largest positive integer value.

In OSVVM, we also set timeouts on the test run length.
Our test can also set bounds on the number of iterations
we allow to run. For example, in most tests, if you loop
more than 2^30 number of times, your test is probably
not going to complete.

I demonstrated above how you code can set reasonable
stopping points. I would never expect a test to try to run
2^31 values for any item, hence, we allow the simulator
to do the checking for this.

If I over simplify your code, the following is effectively what
you are doing. I would expect this test to also finish due
to an overflow.

p_main : process
    variable i : integer;
  begin
    test_runner_setup(runner, runner_cfg);
    show_all(display_handler);

    while test_suite loop
      if run("random") then
        while true loop
          i := i + 1 ; 
          report "i = " & to_string(i) severity note ;
        end loop;
      end if;
    end loop;

    info(to_string(get_checker_stat));
    test_runner_cleanup(runner);
  end process p_main;

@JimLewis
Copy link
Member

I am closing this as it is a user error rather than a tool or OSVVM error. If you disagree, please re-open the issue and explain.

There is no question that OSVVM could detect this, but there is a cost for this sort of checking and it is not going to give you much better of an error message than you already received.

@eschmidscs
Copy link
Author

eschmidscs commented Jan 14, 2020

I forgot one detail: I did check for 2^31-1. The error pops up after about 130k items.
Dump from a test run:

%% WriteBin:
%% Bin:(0)   Count = 32326  AtLeast = 1
%% Bin:(1)   Count = 32691  AtLeast = 1
%% Bin:(2)   Count = 32532  AtLeast = 1
%% Bin:(3)   Count = 33056  AtLeast = 1

%% WriteBin:
%% Bin:(0)   Count = 32326  AtLeast = 1
%% Bin:(1)   Count = 32691  AtLeast = 1
%% Bin:(2)   Count = 32533  AtLeast = 1
%% Bin:(3)   Count = 33056  AtLeast = 1

/usr/local/bin/ghdl:error: overflow detected
  from: osvvm.coveragepkg.covptype.icoverindex at CoveragePkg.vhd:2072
/usr/local/bin/ghdl:error: simulation failed
fail (P=0 S=0 F=1 T=1) zf_radar.tb_osvvm.random (83.8 seconds)

==== Summary ====================================
fail zf_radar.tb_osvvm.random (83.8 seconds)
=================================================
pass 0 of 1
fail 1 of 1
=================================================
Total time was 83.8 seconds
Elapsed time was 83.8 seconds
=================================================
Some failed!

So it has 130606 items in the cover point.

I guess I could make the loop finite to run for 150000 iterations and it would happen.

@eschmidscs
Copy link
Author

According to https://stackoverflow.com/questions/21333654/how-to-re-open-an-issue-in-github i cannot reopen this issue.
Maybe it happens that you look at it anyway...

@JimLewis JimLewis reopened this Jan 14, 2020
@JimLewis
Copy link
Member

Hi Emanual
I modified your code to the following and ran it under Mentor and Aldec tools. It ran without issue. Of course, this takes a little time to run before it even gets to the end.

With that it looks like a GHDL bug.

Jim

library ieee;
context ieee.ieee_std_context;

library osvvm;
use osvvm.RandomPkg.all;
use osvvm.CoveragePkg.all;

entity CovTest is
end entity CovTest;

architecture tb of CovTest is

  shared variable cov_fill   : CovPType;
  constant cTestName : string  := "CovTest" ;
  signal   TestName  : string(cTestName'range) := cTestName ;
begin
  ------------------------------------------------------------------------------------------------------------------------
  -- Test process
  ------------------------------------------------------------------------------------------------------------------------
  p_main : process
    variable rnd      : RandomPType;
  begin
    cov_fill.AddBins(GenBin(0, 3));
    for i in 1 to 4 loop 
      for i in 1 to 2**30 loop
        cov_fill.ICover(rnd.RandInt(0, 3));
        if i mod 2**26 = 0 then 
          cov_fill.WriteBin;
        end if ; 
      end loop;
    end loop;
    cov_fill.WriteBin;
    std.env.stop ; 
  end process p_main;

end architecture tb;

@eschmidscs
Copy link
Author

Hi Jim

Ok, I'll file the same issue for GHDL. Let's see what tgingold knows about.

I saw that Questa runs the TB without issues. I took a short look at the code where the error pops up and had the impression that this indeed can cause an integer to overflow already with not too many items (CovBinPtr(Index).OrderCount looks like some kind of Fibonacci-related series).

So if Questa (and maybe also Riviera?) just takes the overflow and wraps (as a c program would), but GHDL throws an error, this would already explain the behaviour.
Then the question is if GHDL should be told to ignore the overflow or if CoveragePkg may not do this to be compatible with GHDL (if this is a requirement).

Best regards,
emanuel

@Paebbels
Copy link
Member

If the integer really overflows, a simulator must fail when a out of range value is assigned.

But there is a trick in commercial simulators: They wrap around and the new value (false value) is again in range before assignment. Hence no bound check failure. All commercial simulators use strict 32-bit integers. Many of them are not checking the overflow flag in the CPU.

In contrast, GHDL has internally a 64-bit universal_integer and an integer with 32-bit range. It can detect overflows and fail as requested by the standard.

@eschmidscs
Copy link
Author

That wrap around is what a "standard" programming language (like C) would do, right? It just goes negative beyond the other end.

I'll try to get the code dump the value of the mentioned variable. I just have to get a modifiable environment first (sorry, that's caused by our setup right now).

@JimLewis
Copy link
Member

Hi Emanual,
Can you please replace your ICoverIndex with the following one with
the lines associated with OrderCount commented out and test them
on GHDL.

    ------------------------------------------------------------
    -- Local
    procedure ICoverIndex( Index : integer ; CovPoint : integer_vector ) is
    ------------------------------------------------------------
      variable buf : line ;
    begin
      -- Update Count, PercentCov
      CovBinPtr(Index).Count := CovBinPtr(Index).Count + CovBinPtr(Index).action ;
      VendorCovBinInc(VendorCovHandleVar, Index);   -- VendorCov
      CovBinPtr(Index).PercentCov := CalcPercentCov(
        Count => CovBinPtr.all(Index).Count,  
        AtLeast =>  CovBinPtr.all(Index).AtLeast ) ;
--!      -- OrderCount handling - Statistics
--!      OrderCount := OrderCount + 1 ;
--!      CovBinPtr(Index).OrderCount := OrderCount + CovBinPtr(Index).OrderCount ;
      if CovBinPtr(Index).action = COV_ILLEGAL then
        if IllegalMode /= ILLEGAL_OFF then
          if CovPoint = NULL_INTV then
            alert(AlertLogIDVar, GetNamePlus(prefix => "in ", suffix => ", ") & "CoveragePkg.ICoverLast:" & 
                          " Value randomized is in an illegal bin.", IllegalModeLevel) ; 
          else
            write(buf, CovPoint) ; 
            alert(AlertLogIDVar, GetNamePlus(prefix => "in ", suffix => ", ") & "CoveragePkg.ICover:" &  
                          " Value " & buf.all & " is in an illegal bin.", IllegalModeLevel) ; 
            deallocate(buf) ; 
          end if ; 
        else
          IncAlertCount(AlertLogIDVar, ERROR) ; -- silent alert.
        end if ; 
      end if ; 
    end procedure ICoverIndex ;

Order count was part of some statistics I was doing to see how balanced
different intelligent coverage solutions were. It is not needed in any
way for normal operation.

I will comment it out in the next revision if it is the source of the issues.
With VHDL-2019, this sort of thing would probably be inside conditional
compilation anyway.

Best Regards,
Jim

@JimLewis
Copy link
Member

Hi Emanuel
I have tested the change in regression and pushed it to the master branch.

Let me know how it goes.

Best Regards,
Jim

@eschmidscs
Copy link
Author

Thanks a lot, I'll test that as soon as I get time for it....

@eschmidscs
Copy link
Author

ok, works for me. thanks a lot!

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

3 participants