Subject: RE: [sv-cc] Comments w.r.t. Section 29, SystemVerilog Data Read API
From: Bassam Tabbara (bassam@novas.com)
Date: Tue Jan 13 2004 - 16:09:33 PST
[BT] Thx Michael a lot for the write-up (and the suggested corrections),
just some notes if any (to serve as a change reference from previous rev).
(Main) comments:
- Table 29-2: New Reader VPI routines, 3rd row, column "To" (pg 9)
the last sentence in brackets ("except in the case of calling the
routines in the default library") is misleading, since it suggests the
existance of a default library (what is this?) as well as it looks like an
exception to the access indirection through the reader_extension_ptr for
this library. My understanding is that a) there is no default library and b)
only 'native' VPI functions can be directly called.
Suggest changing the sentence "All following VPI routines are called by
indirection through the returned pointer (except in the case of calling the
routines in the default library)" to "All VPI routines defined by the reader
extension library are to be called by indirection through the returned
pointer; only {native/built-in/standard/or whatever} VPI routines can be
called directly"
[BT] Done.
- sub-bullet a) of bullet 1) on page 10 saying "Name of the reader library
to be used specified as a character string. A NULL entry means that the
default library (i.e. tool (e.g. simulator) the application is running
under) is used.".
Here is again the default library, where I simply don't understand
whether we request the existance of such a library. Also, it is not
mentioned what is the 'name of the reader library'. My understanding until
now is that this is either a static or shared library implementing the
reader extension. It could therefore be a) a pathname to this file or b)
just the filename [assuming some search rules for libraries] with or without
an extension. There are some problems with this:
* the requirement to specify an extension immediately means the
corresponding code is platform dependent (Urrrgh)
* the requirement to specify a full pathname might result in non portable
code + might implicitely introduce again platform dependencies (this can be
easily a killer for reuse ...)
Suggest to change the sentence from "Name of the reader library to be
used specified as a character string. A NULL entry means that the default
library (i.e. tool (e.g. simulator) the application is running under) is
used." to "Name of the reader library to be used specified as a character
string. This is either a full pathname to this library or the single
filename (without path information) of this library, assuming a vendor
specific way of defining the location of such a library; the latter method
is more portable and therefore recommended. Neither the full pathname, nor
the single filename shall include an extension, the name of the library must
be unique and the appropriate extension for the actual platform should be
provided by the application loading this library."
I am still unsure about the usage/definition of a "default library" and
its implication. Suggest to remove anything relating to such a library.
Please review also the sample code 29.7.4.1 where this is used ...
Also suggest to modify 29.10.2 (page 23) accordingly
[BT] Change above is done. I changed all the "default" to "built-in". I
think there is a case to be made to use (and have examples) for the
"built-in" library without any pointer de-reference... it makes this chapter
approachable and familiar to people using the current day VPI, or may be
people who just want to get the "overview idea" of the functionality and
will leave the actual load of the extension and so on to someone else ...
- sub-bullet b) of bullet 1) on page 10 saying "Database file with stored
data in case of vpiAccessPostProcess mode (...)"
Here are two problems
a) I am confused whether I just need to specify a name here for
vpiAccessPostProcess, the description of the Access mode below suggests the
usage of a 'scratch file' for vpiAccessInteractive. We should clearly state
when it is required to provide a name here and when not. No suggestion,
sorry
b) My understanding is that this is a logical database name, this
should be reflected and explained here, so it is clear that this is not
neccessarily the name of a file in the file system. Suggest to rephrase to
"Name of the logical database holding the stored data in case of <list the
required access modes here>; a NULL can be used in case of <list optional
access modes>. This is the name of a database, not the name of a file in the
file system. It is implementation dependent whether there is any
relationship to an actual on-disk object and the provided name." There are
several references to a "data file" in the text below, suggest to change
these to "database", similarly in 29.7.5 (sample code) and 29.9 (example)
Also suggest to modify 29.10.2 (page 23) accordingly
[BT] All done.
- suggest to call vpi_read_close() for _all_ access modes. Suggest to
change the second block after the example on how to call
reader_extension_ptr->vpi_get(...) in the second half of page 10
accordingly. There might be cleanup actions required for other access modes
as well. We should permit also implementations requiring this ...
[BT] I added that it should be called in the modes that rely on database (to
close and cleanup) i.e. vpiAccessPostProcess and vpiAccessInteractive. I
shyed away from adding it to vpiAccessInteractive because that is the same
mode of a regular VPI access to simulator. Calling a vpi_close() would
invalidate all the handles created and that does not seem a friendly way to
deal with things in that specific mode.
- 29.9 second page 20: the content of the s_vpi_extension structure is
underspecified. There should be at least a statement on how the functions
within this structure are to be ordered. It looks to me like it is intended
to be exactly in the order of VPI functions defined in the IEEE 1364
standard (vpi_chk_error is the first, vpi_printf is the last such function),
but it would be not a bad idea to state this by a sentence. We must be
rather exact here...
[BT] ok.
- 29.9 two sections before the big example code at the end of this
section. The sentence "Note that, as stated earlier ..."
Suggest to reword "default routine implementation" to "built-in routine
implementation"
[BT] OK.
- It might be good to state in the description vpi_load_extension() on
page 23, that the generic definition of this function permits later, non
reader specific extension; all other arguments are specific and defined for
the reader extension.
[BT] ok.
(Very) minor comments
- GENERAL
I am unsure how common the abbreviation VC for value change is. I am used
to VC for virtual component, so this stuff could be rather confusing. You
might want to consider a 'search and replace for VC to value change'...
[BT] I agree with you, but this is used a lot in the chapter an abbreviation
is valid, no idea on how best to address this.
- 29.1.1 Requirements (pg 1)
The section starts with "SystemVerilog adds several design and
verification constructs including:", but the following list just mentions
data type extensions. I think it would be good to change "- User defined
data types" to "- User defined data types and corresponding methods" and "-
Test bench data types and facilities" to "- Data types and facilities
enhancing the creation and functionality of testbenches" to make the first
sentence a little more true.
[BT] ok.
- 29.2.1, 29.2.2.1, and 29.2.2.2.1
I would leave some holes in the #defines for the types (e.g. by starting
a new block at a new 10 number), to leave some space for later extensions.
Suggest to start vpiIsLoaded at 810, vpiAccessLimitedInteractive at 820,
vpiMember at 830, and vpiMinTime at 840.
[BT] Ok great idea I've added more gaps for things that capture different
notions and potentially can take on extension.
- 29.7.1 section after the example on how to call vpi_get (middle of page
10)
I think it should say "is the reader library pointer returned by" instead
of "is the reader library pointer returned after"
[BT] done.
- 29.7.5 Sample code:
in the call of vpi_load_extension() the last two arguments are switched
...
[BT] oops, good catch.
- 29.9 3rd example showing the use of vpi_load_extension():
A perfect check would first check the struct_size to be greater or equal
than the expected value (to make sure we are on the save side before
dereferencing), and then check extension_version and extension_name to be
non-NULL before dereferencing it ... Then we might want to check the version
to be greater or equal to 1 (or have a switch statement processing possible
further versions ...), but this is of second order ...
[BT] I added the struct_size part ...
--NOTE: The content of this message may contain personal views which are not neccessarily the views of Motorola, unless specifically stated. ___________________________________________________ | | _ | Michael Rohleder Tel: +49-89-92103-259 | _ / )| Software Technologist Fax: +49-89-92103-680 |( \ / / | Motorola, Semiconductor Products, System Design | \ \ _( (_ | _ Schatzbogen 7, D-81829 Munich, Germany _ | _) )_ (((\ \>|_/ > < \_|</ /))) (\\\\ \_/ / mailto:Michael.Rohleder@motorola.com \ \_/ ////) \ /_______________________________________________\ / \ _/ \_ / / / \ \
The information contained in this email has been classified as: Motorola General Business Information (x) Motorola Internal Use Only ( ) Motorola Confidential Proprietary ( )
*** This note may contain Motorola Confidential Proprietary or Motorola Internal Use Only Information and is intended to be reviewed by only the individual or organization named above. If you are not the intended recipient or an authorized representative of the intended recipient, you are hereby notified that any review, dissemination or copying of this email and its attachments, if any, or the information contained herein is prohibited. If you have received this email in error, please immediately notify the sender by return email and delete this email from your system. Thank you! ***
This archive was generated by hypermail 2b28 : Tue Jan 13 2004 - 16:10:52 PST