-
Notifications
You must be signed in to change notification settings - Fork 143
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
Minimal change to suppress -Wformat-truncation #649
Conversation
Ah, cool, thanks. The warning is indicating that the buffer only has room for one digit of Also, can you say what you mean by real C++, that you'd prefer to see? |
Yeah, I was going for a very minimal change I could think of. A more decent fix would've used |
Any objection if instead of your changes we just change line 1002 to specify a larger buffer?:
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. |
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) |
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. |
18f812b
to
09be573
Compare
No worries. I pushed the updated fix. |
Oops, I think the latest commit still has the |
09be573
to
c1eeb46
Compare
Sorry about the churn. |
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); | ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
c1eeb46
to
57d248f
Compare
Thank you! |
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.
A much better fix would be to switch to real C++, but it's a battle for another day.