#65 Rework of spice_dstring

[Jim Monte]

The spice_dstring functions use a stack-based buffer for managing strings of “ordinary” length but will perform dynamic allocations as required when an exceptionally long string is encountered. Thus, they offer the potential to avoid dynamic allocations most of the time without allocating a possibly unreasonable worst-case size and to provide dynamic growth for general string operations.

While the spice_dstring functions have the potential to eliminate buffer overruns and remove arbitrary length restrictions, the API has some issues. The function to determine the required buffer size when a printf-style fomat is appended, spice_format_length( ), underestimates the required buffer length in some cases, which will lead to buffer overruns. The case conversion is unnecessary complex. Another problem is the issue of an “ordinary” length. Depending on the situation, this value may differ, but the API uses a single macro to select the value for all instances. Also these functions have inconveniently long names that do not add much to explain their action and functions are missing to perform common tasks. These issues were addressed in this patch. There is also a function for unit testing that is conditionally compiled if DSTRING_UNIT_TEST is defined.

[Jim Monte]

A small addition will be needed for GCC for debug builds due to the changed semantics of inline for C99. These were not enforced by Microsoft. The declarations of ds_clear() and ds_free_move() are commented out because they are extensions to the dstring API that were written after this patch. They will be need to be uncommented when the inline definitions of these functions are added.

ds_clear() handles the common case of setting the string length to 0. ds_free_move() allows the dstring buffer, if allocated, to be "moved" to the caller when the dstring is being freed to avoid an unnecessary allocation and copy. This action is like move semantics in C++.

In dstring.c, the lines from the first comment / / comment, inclusinve, to the start of the second / / comment, exclusive, must be added.

#include "ngspice/dstring.h"

static int ds_reserve_internal(DSTRING *p_ds,
        size_t n_byte_alloc_opt, size_t n_byte_alloc_min);

/* Instantiations of dstring functions in case inlining is not performed */
int ds_cat_str(DSTRING *p_ds, const char *sz);
int ds_cat_char(DSTRING *p_ds, char c);
int ds_cat_ds(DSTRING *p_ds_dst, const DSTRING *p_ds_src);
int ds_cat_mem(DSTRING *p_ds, const char *p_src, size_t n_char);
int ds_set_length(DSTRING *p_ds, size_t length);
//void ds_clear(DSTRING *p_ds);
//char *ds_free_move(DSTRING *p_ds);
char *ds_get_buf(DSTRING *p_ds);
size_t ds_get_length(DSTRING *p_ds);
size_t ds_get_buf_size(DSTRING *p_ds);

/* This function initalizes a dstring using *p_buf as the initial backing

[Jim Monte]

Formatting characters adversely affected the description of the location in dstring.c in my previous comment. Below is the start of dstring.c with the changes added.

/* -----------------------------------------------------------------
FILE:    dstring.c
DESCRIPTION:This file contains the routines for manipulating dynamic strings.
----------------------------------------------------------------- */
#include <ctype.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "ngspice/dstring.h"

static int ds_reserve_internal(DSTRING *p_ds,
        size_t n_byte_alloc_opt, size_t n_byte_alloc_min);

/* Instantiations of dstring functions in case inlining is not performed */
int ds_cat_str(DSTRING *p_ds, const char *sz);
int ds_cat_char(DSTRING *p_ds, char c);
int ds_cat_ds(DSTRING *p_ds_dst, const DSTRING *p_ds_src);
int ds_cat_mem(DSTRING *p_ds, const char *p_src, size_t n_char);
int ds_set_length(DSTRING *p_ds, size_t length);
//void ds_clear(DSTRING *p_ds);
//char *ds_free_move(DSTRING *p_ds);
char *ds_get_buf(DSTRING *p_ds);
size_t ds_get_length(DSTRING *p_ds);
size_t ds_get_buf_size(DSTRING *p_ds);

/* This function initalizes a dstring using *p_buf as the initial backing
 *
 * Parameters
 * p_buf: Inital buffer backing the dstring
 * length_string: Length of string in the initial buffer
 * n_byte_data: Length of initial buffer. Must be at least 1
 * type_buffer: Type of buffer providing initial backing
 *
 * Return codes
 * DS_E_OK: Init OK
 * DS_E_INVALID: n_byte_data = 0 length_string too long,
 *      or unknown buffer type
 */
int ds_init(DSTRING *p_ds, char *p_buf, size_t length_string,
        size_t n_byte_buf, ds_buf_type_t type_buffer)

[Holger Vogt]

A first quick test of this patch failed. Adding it to current pre-master has been without problems, as well as compiling into shared ngspice. Running the resulting ngspice.dll from within KiCad immediately leads to a crash. All the available memory of my machine (16 G) is used up. Unloading the patch rervealed the previous status of being o.k.. Further investigations are required.

[Jim Monte]

Hi Holger,

I have tested the current version of dstring a lot by now, including some
limited testing in Linux and MinGW. I would suggest waiting on this patch
(and others I submitted) until I merge them into beta_jdm. I do not recall
any specific fix to the version you tried, although it is old as measured
by development work and there may have been one. The merging is coming
along OK. I had been tied up a bit the past couple of weeks but am making
progress now. Getting history substitution right, the last remaining issue
before making the branch, has been more work than expected. I found and
reported a few bugs with history substitution in Bash (and readline), which
I used as a reference, so until those are fixed, native ngspice history
substitution will be better than than the current version in Bash.

Incidentally, thank you for updating the table on OpenMP performance. It
was very informative when compared with the previous results. On the
positive side, it appears the Windows implementation of OpenMP has
improved: The overhead of using it with a single thread has been reduced
and adding threads up to the number of cores continues to reduce execution
time. However, both the standard and OpenMP times have increased by an
amount more than can be attributed to inherent variability in the timings.
The version of Linux changed, but since the version of Windows is the same
and it is unlikely that the compiler has become worse over time, that
leaves ngspice itself as the likely cause. Do you have any idea of what
contributed to the slowdown?

Jim

On Sat, Oct 5, 2019 at 9:27 AM Holger Vogt h_vogt@users.sourceforge.net
wrote:

A first quick test of this patch failed. Adding it to current pre-master
has been without problems, as well as compiling into shared ngspice.
Running the resulting ngspice.dll from within KiCad immediately leads to a
crash. All the available memory of my machine (16 G) is used up. Unloading
the patch rervealed the previous status of being o.k.. Further
investigations are required.

---

• [patches:#65] https://sourceforge.net/p/ngspice/patches/65/ Rework of
  spice_dstring*

Status: open
Group: Unstable (example)
Labels: cleanup buffer overrun feature
Created: Fri Apr 19, 2019 04:37 AM UTC by Jim Monte
Last Updated: Tue May 28, 2019 03:48 AM UTC
Owner: nobody
Attachments:

• new_dstring.patch
  https://sourceforge.net/p/ngspice/patches/65/attachment/new_dstring.patch
  (68.2 kB; application/octet-stream)

The spice_dstring functions use a stack-based buffer for managing strings
of “ordinary” length but will perform dynamic allocations as required when
an exceptionally long string is encountered. Thus, they offer the potential
to avoid dynamic allocations most of the time without allocating a possibly
unreasonable worst-case size and to provide dynamic growth for general
string operations.

While the spice_dstring functions have the potential to eliminate buffer
overruns and remove arbitrary length restrictions, the API has some issues.
The function to determine the required buffer size when a printf-style
fomat is appended, spice_format_length( ), underestimates the required
buffer length in some cases, which will lead to buffer overruns. The case
conversion is unnecessary complex. Another problem is the issue of an
“ordinary” length. Depending on the situation, this value may differ, but
the API uses a single macro to select the value for all instances. Also
these functions have inconveniently long names that do not add much to
explain their action and functions are missing to perform common tasks.
These issues were addressed in this patch. There is also a function for
unit testing that is conditionally compiled if DSTRING_UNIT_TEST is defined.

---

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/ngspice/patches/65/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

[Jim Monte]

After the KiCad EDA download finishes, I would like to try the same test that failed using the latest DSTRING updates. Please clarify what was being done while "Running the resulting ngspice.dll from within KiCad."

[Holger Vogt]

Compile shared ngspice as a 64 bit shared library (Visual Studio or MINGW)
Rename the resulting ngspice.dll to libngspice-0.dll
In C:\Program Files\KiCad\bin rename libngspice-0.dll to libngspice-0-orig.dll
Copy the new file to C:\Program Files\KiCad\bin and thus replace the lib stemming from the download.
Run any of the examples provided in C:\Program Files\KiCad\share\kicad\demos\simulation

[Jim Monte]

Thank you for the details. I worked through the 2-pole RC filter at http://ngspice.sourceforge.net/ngspice-eeschema.html to help familiarize myself with using ngspice with KiCad. The comment about the Frequency label being partially hidden is no longer valid with KiCad 5.1.4. The margins around the plot have been increased, and there is no clipping of the label. I am attaching an updated screenshot of the plot that could be used to replace the existing Fig. 3.

[Jim Monte]

I have made the clipping fix a separate "patch" with the updated image since it is unrelated to the dstring issue.

[Jim Monte]

Hmm, I ran the four examples in the simulation directory using a DLL made by Visual Studio without any abnormal memory usage. More testing must be done still, but it appears that the memory usage issue is not appearing. I am unsure why since I do not recall making any changes since the earlier testing that would have addressed that problem.

[Jim Monte]

Holger,

I have still not been able find a case of abnormal memory usage with the new dstring, and comparing the original patch line by line with the new changes did not reveal any obvious differences that would affect memory usage. Please provide any information you may recall regarding the build configuration (MinGW/MSVS, platform SDK and tool set versions, debug/release/build flags, etc.) that was used to create the DLL used for testing on 2019-10-05 so I may focus on those conditions.

[Jim Monte]

Hi Holger,

I have an unrelated question regarding changes in output compared to the original libngspice-0.dll that comes with KiCad. Have there been changes made in ngspice that would cause the simulation output to change a moderate amount, at times as large as after a few significant digits -- too much to be able to attribute to possible operation reordering? I was concerned that it may be somehow related to the dstring changes (via very selective buffer overruns, extremely unlikely but possible, at least in theory). However further testing shows the differences also with the ngspice.dll in the 31 release. When simulating the laser_driver example, I get the same output using the DLL in the 31 release, a debug MSVS build, a release MSVS build, a MinGW -O2, -s build, and a MinGW -g build. However, all of these differ from the output using the original DLL that came with KiCad. To ensure that the DLL changes were having an effect, I completely removed libngspice-0.dll and verified that the simulation would fail between tests.

[Holger Vogt]

Jim,

I am not aware right now of any significant code changes.

I suppose you are using ngspice-30.

I would have to scan the commitments. So you can do as well.

Holger

[Jim Monte]

For the benefit of anyone reading this page in the future, the small changes in output values were found to be due to updates of physical constants such as the speed of light to the current best estimates or defined values of them.

[Jim Monte]

The old pach with the additons described on 2019-05-28 was applied to the current pre-master source code and 4 ngspice shared DLLs were made: MSVS debug, MSVS release, MinGW debug (-g), and MinGW release (-O2, -s). Each of the 4 demo simulations was run multiple times with each of these DLLs for a total of nearly 100 runs. None of these showed any substantial memory usage, so I have not been able to duplicate the results found on 2019-10-05 with either the current version of dstring or the patch that was originally submitted. Please update the comments in this patch with a detailed instance of the excessive memory usage, if one can be found.

The current dstring revision can be found in branch beta_jdm in commit b87e6f4d2bab7501cd00e7a53b14dee4d77a0282

[Jim Monte]

For reference, I will attach the DLLs I made for testing the old patch with the additons described on 2019-05-28.

[Jim Monte]

MinGW debug

[Jim Monte]

MinGW release

[Jim Monte]

MSVS Debug

[Jim Monte]

MSVS Release

[Holger Vogt]

The main reason is that the example now uses short channel MOS transistors modelled with BSIM4. This model is more complex than BSIM3 used in the previous example.

[Holger Vogt]

I have just compiled the beta_jdm branch (VS2019, SDK 8.1 or 10, 64 bit).

Several examples do not run correctly, e.g.
ngspice\examples\xspice\pll\pll-xspice-fstep.cir or
ngspice\examples\xspice\table\table-model-bip-2d-1.sp.
The errors seem to occur during expanding some subcircuits.

The same happened after cherry-picking the the dstring patch from beta_jdm to the pre-master branch. With current pre-master branch the examples simulate without error.

[Jim Monte]

Thank you for the examples. I had been testing exclusively with the DLL looking for memory usge when running the examples given since that was where the issue was found earlier.

Using the ngspice\examples\xspice\pll\pll-xspice-fstep.cir example, I could see a difference between the current pre-master and beta_jdm. On the 4th call to translate() in subckt.c, the "formal" argument is longer in beta_jdm, which causes an error when settrans() is called. My guess is a missing null termination somewhere, but I will find out.

[Jim Monte]

So far dstring itself seems OK. However, one of its uses was missing a buffer reset (ds_clear()) and another called the wrong dstring function. With those changes, ngspice\examples\xspice\pll\pll-xspice-fstep.cir runs as it does with the (almost) current pre-master branch. The other example, ngspice\examples\xspice\table\table-model-bip-2d-1.sp, crashes with both pre-master and beta_jdm branches. They are missing a few days of updates (some changes on beta_jdm are preventing a merge there, and I am merging the branches with pre-master at SourceForge at the same time so they can be compared more easily), so hopefully that issue has been fixed recently.

[Holger Vogt]

ngspice\examples\xspice\table\table-model-bip-2d-1.sp will need running ngspice\examples\xspice\table\table-generator-q-2d.sp first to generate the data table.

There is an update to pre-master that prevents a seg fault when the table is missing.

[Jim Monte]

With the buffer clear and changed function described earlier, the new dstring appears to be working. These changes can be found in branch beta_jdm in commit c86b7692f5989eaca04b5ad6b178e0428ae16782