On the Effectiveness of nonnull

dankblog! 2021-11-05, 0236 EDT, at the danktower

I add annotations to my C programs pretty religiously, making most frequent use of:

• const and pure -- facilitate hoisting and memoization
• malloc -- helps aliasing analysis and warning generation
• warn_unused_result -- warning generation
• nonstring -- warning generation
• uninitialized in conjunction with -ftrivial-auto-var-init
• format -- warning generation for printf-style functions in conjunction with -Wformat, which ought probably be mandatory
• nothrow -- improves C++ code generation when exceptions are enabled. ought be on every C function you declare that doesn't call C++ (beware user-provided function pointers!)
• returns_nonnull -- improves code generation and static analysis
• sentinel in the unfortunate situations where it's applicable (did you really need varargs there? that's not how we raised you, son)

and of course the ubiquitous visibility("default") (roughly) ala Microsoft's __declspec(dllexport), which you ought be diligently employing on your shared libraries as mr. drepper taught you.

access lets you get fine-grained, Ada-like controls on your variable access types, but if i were writing Ada i'd be fucking with gnat and working for Northrop Grumman and questioning my life choices.

finally, nonnull is all over the place, but it's usually added merely to satisfy the spergy part of my brain that also forces me to compulsively run ls every time i switch between terminals just to, like, ensure nothing shifted around. a pleasant surprise today when it caught a bug in my existing code:

// for now, we just run the top half of notcurses_render(), and copy out the
// memstream from within rstate. we want to allocate our own here, and return
// it, to avoid the copy, but we need feed the params through to do so FIXME.
int ncpile_render_to_buffer(ncplane* p, char** buf, size_t* buflen){
  if(ncpile_render(p)){
    return -1;
  }
  notcurses* nc = ncplane_notcurses(p);
  unsigned useasu = false; // no SUM with file
  fbuf_reset(&nc->rstate.f);
  int bytes = notcurses_rasterize_inner(nc, ncplane_pile(p), &nc->rstate.f, &useasu);
  pthread_mutex_lock(&nc->stats.lock);
    update_render_bytes(&nc->stats.s, bytes);
  pthread_mutex_unlock(&nc->stats.lock);
  if(bytes < 0){
    return -1;
  }
  *buf = memdup(nc->rstate.f.buf, nc->rstate.f.used);
  if(buf == NULL){
    return -1;
  }
  *buflen = nc->rstate.f.used;
  return 0;
}

ignore the nonsensical mouthfoam in the opening comment. do you see the bug? it ought require no knowledge of the content beyond standard ANSI C to do so.

...see it yet?

  *buf = memdup(nc->rstate.f.buf, nc->rstate.f.used);
  if(buf == NULL){
    return -1;
  }

OH SHIT, THERE GOES THE PLANET. that error check is completely pointless; indeed, the compiler might well elide it entirely (if buf is NULL, dereferencing it in the memdup line is undefined behavior, and the compiler can do whatever it wants. if it is not NULL, the check is pointless. thus work the mysterious minds of the Compiler People). a static analyzer arguably ought have picked up on this (and i run several analyzers regularly), but there's good reasons why not, also.

either way, adding nonnull(2) immediately generated the warning:

/home/dank/src/dankamongmen/notcurses/src/lib/render.c: In function ‘ncpile_render_to_buffer’:
/home/dank/src/dankamongmen/notcurses/src/lib/render.c:1561:5: warning: ‘nonnull’ argument ‘buf’ compared to NULL [-Wnonnull-compare]
 1561 |   if(buf == NULL){
      |     ^

and thus the inevitable "what? oh fuck what is this. no i just assigned to buf, you dumbass compiler, it could be NULL now, computer piece of shit, oh wait no fuck me in the ass, i assigned to *buf and checked buf like a witless simpleton asshole, fuck me in the ass twice, awww fuck piece of shit brain, thank you compiler you're a good compiler here's some more page cache, awww look at you chomping those pages you're my only friend, compiler."

ps i ended up getting rid of the copy when i fixed this, too. w00t! =]

previously: "four questions for a wednesday" 2021-10-13