Minimal change to suppress -Wformat-truncation

[blackgnezdo]

The compilers are smart enough to see that this might be a problem but not to know that the range of the value is limited.

Bluesim/bs_prim_mod_reg.h: In member function 'unsigned int MOD_mkValidVectorTest_Dut::dump_VCD_defs(unsigned int)': Bluesim/bs_prim_mod_reg.h:1007:31: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=]
 1007 |       snprintf(buf, 8, "Q_OUT_%u", i);
      |                               ^~
In member function 'unsigned int MOD_CReg<T>::dump_VCD_defs(unsigned int) [with T = unsigned char]',
    inlined from 'unsigned int MOD_mkValidVectorTest_Dut::dump_VCD_defs(unsigned int)' at bazel-out/k8-opt/bin/rtl/lib/test/mkValidVectorTest_Dut.cxx:193:39:
Bluesim/bs_prim_mod_reg.h:1007:24: note: directive argument in the range [0, 4294967294]
 1007 |       snprintf(buf, 8, "Q_OUT_%u", i);
      |                        ^~~~~~~~~~
Bluesim/bs_prim_mod_reg.h:1007:15: note: 'snprintf' output between 8 and 17 bytes into a destination of size 8
 1007 |       snprintf(buf, 8, "Q_OUT_%u", i);
      |       ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~

A much better fix would be to switch to real C++, but it's a battle for another day.

[quark17]

Ah, cool, thanks. The warning is indicating that the buffer only has room for one digit of i at the end; this is okay, because the maximum port number is 5 (currently). You resolved the warning by changing to printing a character, which fits; but another way to resolve the warning would be to just increase the size of buf. Your change is no worse than the existing code, but I'd like to have some assertion that the number of ports is less than 10. If we increase the buffer size, we don't have to worry about it.

Also, can you say what you mean by real C++, that you'd prefer to see?

[blackgnezdo]

Yeah, I was going for a very minimal change I could think of. A more decent fix would've used std::string and std::to_string but I didn't want to look at any more code than I absolutely had to.

[quark17]

Any objection if instead of your changes we just change line 1002 to specify a larger buffer?:

char* buf = (char*) malloc(17);

Can you confirm that that resolves the warning? (I don't think I'm seeing this warning on any of my systems.)

[blackgnezdo]

Any objection if instead of your changes we just change line 1002 to specify a larger buffer?:

char* buf = (char*) malloc(17);

Can you confirm that that resolves the warning? (I don't think I'm seeing this warning on any of my systems.)

Sure, that's fine too. I'm running most recent Debian. It probably takes gcc-12. I'll test later.

[blackgnezdo]

Well, that's not quite all you need to do, so this is also an option, but not longer as minimal as the original:

--- /home/greg/s/bsc/inst/lib/Bluesim/bs_prim_mod_reg.h 2023-11-27 17:33:49.502140131 -0800
+++ bs_prim_mod_reg.h   2023-12-23 09:16:57.935381937 -0800
@@ -999,26 +999,25 @@

     // buffer for auto-generating the signal names
     // (longest name is Q_OUT_# plus room for null terminator)
-    char* buf = (char*) malloc(8);
+    char buf[17];

     vcd_write_scope_start(sim_hdl, inst_name);
     for (unsigned int i = 0; i < ports; i++) {
       // start with Q_OUT, so that the alias' number is reused
-      snprintf(buf, 8, "Q_OUT_%u", i);
+      snprintf(buf, sizeof(buf), "Q_OUT_%u", i);
       vcd_set_clock(sim_hdl, num, __clk_handle_0);
       vcd_write_def(sim_hdl, num++, buf, bits);

-      snprintf(buf, 8, "EN_%u", i);
+      snprintf(buf, sizeof(buf), "EN_%u", i);
       vcd_set_clock(sim_hdl, num, __clk_handle_0);
       vcd_write_def(sim_hdl, num++, buf, 1);

-      snprintf(buf, 8, "D_IN_%u", i);
+      snprintf(buf, sizeof(buf), "D_IN_%u", i);
       vcd_set_clock(sim_hdl, num, __clk_handle_0);
       vcd_write_def(sim_hdl, num++, buf, bits);
     }
     vcd_write_scope_end(sim_hdl);

-    free(buf);
     return num;
   }
   void dump_VCD(tVCDDumpType dt, MOD_CReg<T>& backing)

[quark17]

I like that diff, and vote we go with that. Are you OK pushing it to this PR, or did you want me to?

I'm not sure I follow what you mean by "minimal change". I do get that you don't want to spend a lot of time on this (particularly as it's relatively insignificant), so I do appreciate you taking the time, and I'm sorry for making it taking longer! This second change seems within "minimal" to me, though, and I think it's more readable and less error-prone, so I say we go with it. And, incidentally, I like that you replaced the malloc, as I had also been wondering about that -- looking at the (pre-github) commit log, it seems I was the one who wrote that code and probably just wasn't thinking.

[blackgnezdo]

No worries. I pushed the updated fix.

[quark17]

Oops, I think the latest commit still has the %c and '0'+ parts?

[blackgnezdo]

Oops, I think the latest commit still has the %c and '0'+ parts?

Sorry about the churn.

[quark17]

Thank you!