len_subst in FST loader #422

rfuest · 2025-04-04T18:01:56Z

I'm currently trying to wrap my head around how len_subst in the FST loader is supposed to work for the pseudo 2D array support.

The FST loader detects if the length of a variable is a multiple of $|msi - lsi| + 1$ and changes the Nodes bounds to [var_length - 1:0], but keeps the original bounds in the variable name. What I don't understand is why only the node name is changed to reflect the original bounds and not the name in the SST:

Note the [15:0] in the SST and [3:0] in the trace list. The FST file I used: array2d.zip

In a related note, there doesn't seem to be any bounds checking in case $|msi - lsi| + 1 > varlength$. Here is a test file for that case: invalid_bounds.zip

invalid[5..0] contains random uninitialized data.

The code that generated invalid_bounds.fst:

#include <fstapi.h>
#include <glib.h>

int main(int argc, char *argv[]) {
  g_assert(argc == 2);
  void *writer = fstWriterCreate(argv[1], 1);

  // Use fixed date to make output reproducible
  fstWriterSetDate(writer, "Thu Jan  1 00:00:00 1970");

  fstHandle invalid = fstWriterCreateVar(writer, FST_VT_VCD_WIRE,
                                         FST_VD_IMPLICIT, 2, "invalid[7:0]", 0);

  guint64 t = 0;

  for (gint i = 0; i < 256; i++) {
    fstWriterEmitTimeChange(writer, t++);
    fstWriterEmitValueChange32(writer, invalid, 8, i);
  }

  fstWriterEmitTimeChange(writer, t++);

  fstWriterClose(writer);
}

The text was updated successfully, but these errors were encountered:

tbybell · 2025-04-04T21:33:48Z

For the 2D array one, it should be dumped as [3:0][3:0] because that's how it would be declared in Verilog. It's still wrong in the SST though and shows up as [3:0][15:0] with that fix of the signal name. Note that VCS unrolls any dimension past the first one, so you will see: [3][3:0], ..., [0][3:0] in waves from VCS.

For the SST, it appears as [3:0][3:0] for VCD, so it points to a problem in the fst.c loader code, but it's more than that. If you extract individual bits, savefiles don't work. It's definitely a broken feature that will turn out broken in more ways than one.

Way way way back, I thought there were some experiments I had with with Steve Williams from Icarus or Cary R and there was talk about dumping such arrays as a 15:0 still with those [3:0][3:0] bounds in the $VAR declaration but it might not ever have happened? It's been a while since I've looked at 2D arrays on Icarus. IIRC, my main complaint was sending out one long string of bits could make some arrays chew up huge amounts of file space in the dumpfile. If I have a 1kB SRAM and one bit changes, I have to dump all 8k bits. That said, I don't know if anybody is even using flattened 2D arrays in dumps like that. I just tried the hand edited VCD (for [3:0][3:0]) on nWave (Debussy) and it malfunctions on there, too. The first signal is [3:0][3] and the last one is [3:0][-12]. I'd say len_subst is useless and can be commented out or removed.

For the invalid bounds one,

fstWriterEmitValueChange32(writer, invalid, 8, i);

You're dumping out 8 bits on a 2b signal. Beyond that though, there needs to be a check in fst.c where the bit range is probably truncated from the highest numbered index (typically MSB side, but LSB side for the IBM bit ordering). Simulators know what they're doing (and in some cases I was responsible for testing such as on Icarus and CVC) so dumpfiles from those simulators won't be malformed. Likewise, VCD files used as input have been correct as well. That's a good catch. I would have expected that one from a security researcher.

This fix is against LTS's fst.c, but it is easy to find that sgnb usage to sync up on.
./lib/libgtkwave/src/gw-fst-loader.c line 224 in handle_var():

548:/home/bybell/gtkwave/code/gtkwave3-gtk3/src> diff -cNb fst.c.1 fst.c
*** fst.c.1	2025-04-04 17:08:00.191212163 -0400
--- fst.c	2025-04-04 17:24:10.612513743 -0400
***************
*** 295,300 ****
--- 295,313 ----
  						}
  					*lsb = acc * sgnb;
  					}
+ 
+ 				unsigned int abslen = (*msb >= *lsb) ? (*msb - *lsb + 1) : (*lsb - *msb + 1);
+ 				if((h->u.var.length != abslen) && (h->u.var.length))
+ 					{
+ 					if (*msb >= *lsb)
+ 						{
+ 						*msb = *lsb + h->u.var.length - 1;
+ 						}
+ 						else
+ 						{
+ 						*lsb = *msb + h->u.var.length - 1;
+ 						}
+ 					}
  				}
  
  			if(lb_last)

rfuest · 2025-04-05T01:56:07Z

I'd say len_subst is useless and can be commented out or removed.

OK, great, one special case less to worry about.

For the invalid bounds one, fstWriterEmitValueChange32(writer, invalid, 8, i); You're dumping out 8 bits on a 2b signal.

I had noticed that after I submitted the issue, but it didn't seem to change anything on the GTKWave side. I've applied a similar path to what you suggested and created a new FST file file for the automated test for this issue. Here is the result of opening this file in my development build:

tbybell · 2025-04-05T04:09:47Z

The screen grab from your development build looks good given the input from that file.

rfuest · 2025-04-30T18:50:04Z

I just noticed that there is more code for the 2d array feature in ExpandNode and ExtractNodeSingleBit, which was also added in the same commit: rfuest/gtkwave-archive@e6b5f76 I've removed this code in my development branch.

tbybell · 2025-05-03T21:23:01Z

For the 2d arrays, I identified the problem and fixed it here:
8bb04d3..3a67eab lts -> lts

However, I'm keeping it disabled as I don't see a compelling usage case for it at the moment and I'd like to minimize differences that I can avoid with VCD. I'd say keep it out of dev as there's nothing that can use it at the moment. If it turns out such a strategy could help with some parts of optimizing GHW memory usage, we can dust it off then and have some reference code to stare at so we don't have to reinvent some of the analysis or algorithms.

What it does is if you define a signal like x[0:3][15:0] (any permutation of index directions is acceptable) on a 64b vector, the value stays internally as a 1d 64b vector, but when you expand it, it'll unroll itself bitwise as x[0][15], x[0][14], ..., x[0][0], x[1][15], x[1][14], ..., x[3][1], x[3][0]. It's up to the user to combine down to get the individual elements specified in the rightmost dimension back into vectors, so it's not immediately as useful as a user would want. My best guess is when I implemented it, it was half done and I never finished it as some other issue came up and I forgot about it.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

len_subst in FST loader #422

len_subst in FST loader #422

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

len_subst in FST loader #422

len_subst in FST loader #422

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!