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

DPI import with invalid return type compiles (to wrong output) #2207

Open
gezalore opened this issue Mar 26, 2020 · 4 comments
Open

DPI import with invalid return type compiles (to wrong output) #2207

gezalore opened this issue Mar 26, 2020 · 4 comments

Comments

@gezalore
Copy link
Contributor

@gezalore gezalore commented Mar 26, 2020

The specific issue I found is that importing a DPI function with bit [63:0] as return type compiles:

$ cat test_regress/t/t_dpi_invalid_result_type.v

// DESCRIPTION: Verilator: Verilog Test module
//
// Copyright 2009 by Wilson Snyder. This program is free software; you can
// redistribute it and/or modify it under the terms of either the GNU
// Lesser General Public License Version 3 or the Perl Artistic License
// Version 2.0.

module t;

   // Bit vector return type is invalid according to 1800-2017 35.5.5
   import "DPI-C" function bit [63:0] f();

endmodule

$ cat test_regress/t/t_dpi_invalid_result_type.pl

#!/usr/bin/perl
if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
# DESCRIPTION: Verilator: Verilog Test driver/expect definition
#
# Copyright 2003 by Wilson Snyder. This program is free software; you can
# redistribute it and/or modify it under the terms of either the GNU
# Lesser General Public License Version 3 or the Perl Artistic License
# Version 2.0.

scenarios(simulator => 1);

compile(
    verilator_flags2 => ["-Wall -Wno-DECLFILENAME"],
    fails => 1
    );

ok(1);
1;

This compiles into the following, which is clearly broken as svBitVecVal is typedefed to uint32_t by the standard (H.10.1.2):

$ cat test_regress/obj_vlt/t_dpi_invalid_result_type/Vt_dpi_invalid_result_type__Dpi.h

// Verilated -*- C++ -*-
// DESCRIPTION: Verilator output: Prototypes for DPI import and export functions.
//
// Verilator includes this file in all generated .cpp files that use DPI functions.
// Manually include this file where DPI .c import functions are declared to ensure
// the C functions match the expectations of the DPI imports.

#include "svdpi.h"

#ifdef __cplusplus
extern "C" {
#endif


    // DPI IMPORTS
    // DPI import at t/t_dpi_invalid_result_type.v:11
    extern svBitVecVal f();

#ifdef __cplusplus
}
#endif

Verilator rightly complains and fails if you use logic [63:0] instead of bit [63:0]

More generically, reading the standard at 35.5.5, imported (and exported) function return types are restricted to the following types:

— void, byte, shortint, int, longint, real, shortreal, chandle, and string
— Scalar values of type bit and logic

By "Scalar values of type bit and logic" I believe they mean a single bit or a single logic, i.e.:

import "DPI-C" function bit f();
import "DPI-C" function logic g();

Contrast with 35.5.6 just below which describes types of formal arguments, which explicitly mentions packed arrays of bit and logic.

Table H.1 describes the mappings between SV and C types, and mentions that types for bit and logic are defined in svdpi.h, which H.10.1.1 shows as svBit and svLogic (both are just typedefed to unsigned char)

I appreciate it is useful to be compatible with the EDA vendors' simulators (I haven't checked at all what they do), but to me, reading the standard it sounds like the only valid return types are the following, and only the following + unsigned variants where applicable:

import "DPI-C" function void a();
import "DPI-C" function byte b(); // also unsigned
import "DPI-C" function shortint c(); // also unsigned
import "DPI-C" function int d(); // also unsigned
import "DPI-C" function longint e(); // also unsigned
import "DPI-C" function real f();
import "DPI-C" function shortreal g();
import "DPI-C" function chandle h();
import "DPI-C" function string i();
import "DPI-C" function bit j();
import "DPI-C" function logic k();

Some of the more generic issues then are:

  • Scalar logic is not currently accepted
  • bit[n:0] for n < 32 is accepted and compiles to an svBitVecVal return type, which works but is not standards compliant
  • `bit[n:0] for 32 <= n < 64 is accepted and compiles to an svBitVecVal return type, which does not work

I am happy to attempt to fix the original issue (i.e.: error for packed bit array return type if width > 32), though I wonder what your opinion is on what Verilator should really be doing.

@gezalore gezalore added the new label Mar 26, 2020
@gezalore

This comment has been minimized.

Copy link
Contributor Author

@gezalore gezalore commented Mar 26, 2020

Here is another one which is questionable:

import "DPI-C" function void f(bit [0:0] i);

This is passed by value (as if you declared bit i), rather than reference, which the standard would require for packed array (based on H.7.7, H.8.4 and H.8.7 in particular). Verilator outputs:

extern void f(unsigned char i);
@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Mar 26, 2020

Yes, we would welcome any patches that clean this up, thanks for looking at this.

The intent is to support all of the types allowed by IEEE, and have exactly compatible "C" declaration types matching the other simulators, and ideally error out on anything not legal.

"Scalar values of type bit and logic" includes in my reading e.g. bit [63:0], and also works on other simulators.

Yes, please check the test cases you propose to change against the commercial offerings. If at least one allows something I'd say Verilator should allow it. If they disagree on how to declare something we'll need to debate what is the correct choice.

@wsnyder wsnyder added type: feature-IEEE and removed new labels Mar 26, 2020
@gezalore

This comment has been minimized.

Copy link
Contributor Author

@gezalore gezalore commented Mar 28, 2020

Ok, the number are in... using the following versions of the big 3:

$ irun -version
TOOL: irun 15.20-s075
$ vlog -version
QuestaSim-64 vlog 2020.1 Compiler 2020.01 Jan 28 2020
$ vcs -ID | grep "Compiler version"
Compiler version = VCS Q-2020.03

and Verilator (master):

$ verilator --version
Verilator 4.031 devel rev v4.030-23-g510be535

All 3 accepts the following (except for shortreal, which NC does not support), and produces the given prototypes (bar some typedefs to the equivalents). There are the ones explicitly mentioned in 35.5.5:

  `IMPORT void              ret_void              (); // void ret_void();
  `IMPORT byte              ret_byte              (); // char ret_byte();
  `IMPORT byte unsigned     ret_byte_unsigned     (); // unsigned char ret_byte_unsigned();
  `IMPORT shortint          ret_shortint          (); // short ret_shortint();
  `IMPORT shortint unsigned ret_shortint_unsigned (); // unsigned short ret_shortint_unsigned();
  `IMPORT int               ret_int               (); // int ret_int();
  `IMPORT int unsigned      ret_int_unsigned      (); // unsigned int ret_int_unsigned();
  `IMPORT longint           ret_longint           (); // long long ret_longint();
  `IMPORT longint unsigned  ret_longint_unsigned  (); // unsigned long long ret_longint_unsigned();
  `IMPORT real              ret_real              (); // double ret_real();
  `IMPORT shortreal         ret_shortreal         (); // float ret_shortreal();
  `IMPORT chandle           ret_chandle           (); // void* ret_chandle();
  `IMPORT string            ret_string            (); // const char* ret_string();
  `IMPORT bit               ret_bit               (); // svBit ret_bit();
  `IMPORT logic             ret_logic             (); // svLogic ret_logic();

Verilator produces the same except for shortreal, and logic which it errors on as unsupported.

For Questa, the above is all it accepts.

NC furthermore accepts packed arrays of 2-state bit not wider than 32. These are declared as svBitVecVal ret_bits_n(). It does not accept any packed struct of any width, wide or not.

VCS accepts the same as above, including the packed 2-state arrays up to 32 wide accepted by NC (with the same C declarations), and it also accepts 2-state packed structures of width not more than 32, which are also using the svBitVecVal return type.

None of the simulators accept any form of 4-state output other than scalar logic, not even logic [0:0] in particular.

I have attached for reference the actual test files that I have tried. They contain the outputs as well:
dpi_ret.tar.gz

Verilator is correct on the unambiguous scalar types as defined in 35.5.5, but it's a bit broken on other things. I propose the following changes:

  • Support scalar logic as it's in the standard (just treat it the same as scalar bit...), declare it as svLogic
  • Declare bit as svBit to be closer to the standard (currently it's unsigned char, which is what svBit is typedefed to).
  • Support any 2-state packed data-type of width between 1 and 32 inclusive, and declare as svBitVecVal, for compatibility with the big 3. Currently width 1 packed arrays are declare as unsigned char, this will change to svBitVecVal. Widths of between 33 and 64 are currently accepted but are broken (as the root bug in this thread), these will now error.
  • 4-state packed structures of width up to 64 are currently accepted (but not 4-state packed arrays of logic), (declared as unsigned char for width 1 and svBitVecVal for wider). All of these will now error as this just looks like an oversight..

This will bring us in-line with big 3, bar the shortreal, but that is a different issue for Verilator altogether.

Unless you have any objections, I will have a stab at these changes to start with. If you think some of the above is particularly hard to catch due to internals, I would appreciate a warning. If all goes well I will go through a similar exercise for function arguments..

@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Mar 28, 2020

Thanks for the research & agree with your plan.

If you could please make sure all of the test cases make it (back?) into test_regress tests we'll be able to make sure they stay working correctly. Also it probably would be good in the tests's .pl file to use files_identical to check that the generated header file (with all of the data types) exactly matches a golden file which you check once manually. e.g.

files_identical("$Self->{obj_dir}/Vt_dpi_import__Dpi.h", $Self->{golden_filename});

There are also a few tests that use DPI for reasons other then testing DPI, if they break it should be obvious how to change the types appropriately to fix them up.

gezalore added a commit to gezalore/verilator that referenced this issue Apr 5, 2020
Verilator now generates DPI function signatures that adhere to the
standard (IEEE 1800-2017) for all primitive return types (35.5.5),
and primitive and other packed argument types (35.5.6). The generated
function signatures have been checked against the big 3 commercial
simulators, and Verilator now supports a superset of all of their
functionalities (except for the 'shortreal' type).

Fixes issue verilator#2207 and more.
gezalore added a commit to gezalore/verilator that referenced this issue Apr 5, 2020
Verilator now generates DPI function signatures that adhere to the
standard (IEEE 1800-2017) for all primitive return types (35.5.5),
and primitive and other packed argument types (35.5.6). The generated
function signatures have been checked against the big 3 commercial
simulators, and Verilator now supports a superset of all of their
functionalities (except for the 'shortreal' type).

Fixes issue verilator#2207 and more.
gezalore added a commit to gezalore/verilator that referenced this issue Apr 6, 2020
Verilator now generates DPI function signatures that adhere to the
standard (IEEE 1800-2017) for all primitive return types (35.5.5),
and primitive and other packed argument types (35.5.6). The generated
function signatures have been checked against the big 3 commercial
simulators, and Verilator now supports a superset of all of their
functionalities (except for the 'shortreal' type).

Fixes issue verilator#2207 and more.
gezalore added a commit to gezalore/verilator that referenced this issue Apr 6, 2020
Verilator now generates DPI function signatures that adhere to the
standard (IEEE 1800-2017) for all primitive return types (35.5.5),
and primitive and other packed argument types (35.5.6). The generated
function signatures have been checked against the big 3 commercial
simulators, and Verilator now supports a superset of all of their
functionalities (except for the 'shortreal' type).

Fixes issue verilator#2207 and more.
gezalore added a commit to gezalore/verilator that referenced this issue Apr 6, 2020
Verilator now generates DPI function signatures that adhere to the
standard (IEEE 1800-2017) for all primitive return types (35.5.5),
and primitive and other packed argument types (35.5.6). The generated
function signatures have been checked against the big 3 commercial
simulators, and Verilator now supports a superset of all of their
functionalities (except for the 'shortreal' type).

Fixes issue verilator#2207 and more.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.