Skip to content

Silence "unused result" warning in bluesim probe module #779

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

Merged
merged 1 commit into from
Apr 28, 2025

Conversation

danderson
Copy link
Contributor

In modern glibc, asprintf's return value is annotated as "must use", because it can fail with -1 in OOM conditions.

This is not a correctness issue on modern OSes, since the surrounding code will at worst lead to a null pointer deref and crash in this condition. Further, most modern OSes allocate virtual memory optimistically, and crash later on failure to fault in rather than returning -1 to an allocation call. So, this change's main purpose is to declutter bluesim compilation logs.

@danderson
Copy link
Contributor Author

danderson commented Apr 12, 2025

Sample of the log clutter this removes:

In file included from /nix/store/ksabz0s564dzcsiaj04gmkb46iq7s0kn-bluespec-2024.07/lib/Bluesim/bluesim_primitives.h:12,
                 from out/bluesim/test/DMAChannel/mkTB.cxx:7:
/nix/store/ksabz0s564dzcsiaj04gmkb46iq7s0kn-bluespec-2024.07/lib/Bluesim/bs_prim_mod_probe.h: In member function ‘unsigned int MOD_Probe<T>::dump_VCD_defs(unsigned int) [with T = unsigned char]’:
/nix/store/ksabz0s564dzcsiaj04gmkb46iq7s0kn-bluespec-2024.07/lib/Bluesim/bs_prim_mod_probe.h:45:13: warning: ignoring return value of ‘int asprintf(char**, const char*, ...)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
   45 |     asprintf(&buf, "%s$PROBE", inst_name);
      |     ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Github actually makes it slightly nicer by declining to linewrap it. With bonus linewraps it ends up much less readable, and mixed in with more interesting elaboration warnings that I'd like to see better :)

@quark17
Copy link
Collaborator

quark17 commented Apr 25, 2025

Sounds good, thanks! Can I suggest, though, not using assert, and instead just do something like this:

if (sz < 0)
  perror("dump_VCD_defs: asprintf");

I didn't see any place else that uses assert in the Bluesim code, and we don't need to ability to turn off the assertion, I wouldn't think, so might as well inline the abort call. It'd be best to match whatever convention Bluesim uses for aborting, but I didn't see many such places (only a few uses of perror and nothing else). I assume that asprintf would set errno if it failed, so I assume perror would be OK to use here.

In modern glibc, asprintf's return value is annotated as "must use",
because it can fail with -1 in OOM conditions.

This is not a correctness issue on modern OSes, since the surrounding
code will at worst lead to a null pointer deref and crash in this
condition. Further, most modern OSes allocate virtual memory
optimistically, and crash later on failure to fault in rather than
returning -1 to an allocation call. So, this change's main purpose
is to declutter bluesim compilation logs.

Signed-off-by: David Anderson <[email protected]>
@danderson
Copy link
Contributor Author

Works for me! I wasn't sure how to report an error from here, since as you say there's no obvious precedent. And, I was reluctant to start doing deeper surgery to plumb error results around for a condition that really isn't ever going to happen :)

Updated the implementation as you suggested, make install-src and make check-suite are happy here.

@quark17 quark17 merged commit d5a43d6 into B-Lang-org:main Apr 28, 2025
73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants