Use of "assert" in scott

[cspiegel]

When building Gargoyle with the new Scott Adams interpreter, the following occurs:

In function ‘CopyBytesOut’,
    inlined from ‘LoadNibbleTrack’ at /home/chris/github/garglk/terps/scott/saga/ciderpress.c:637:13,
    inlined from ‘ReadNibbleSector’ at /home/chris/github/garglk/terps/scott/saga/ciderpress.c:674:13:
/home/chris/github/garglk/terps/scott/saga/ciderpress.c:532:5: warning: ‘memcpy’ specified bound 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
  532 |     memcpy(buf, rawdata + offset, size);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

What's happening is that gcc thinks that "size" can be -1 in some cases, which gets passed as SIZE_MAX to memcpy(). The reason it thinks so is this function:

static int GetNibbleTrackLength(PhysicalFormat physical, int track)
{
    if (physical == kPhysicalFormatNib525_6656)
        return kTrackLenNib525;
    else {
        assert(0);
        return -1;
    }
}

It sees the -1 as a possible return, and follows that to the copy. The assert(0) is meant to avoid that possibility, but Gargoyle uses CMake, and when CMake builds in release mode, it sets the NDEBUG macro, which disables assert(). Disabling NASSERT in CMake is hacky and I'd prefer not to work around things that way.

The reason NDEBUG disables assertions is that assert() is meant to capture "impossible" states, ones which should only happen during development and thus ones that will only be caught when release mode isn't turned on. If that's the case here, if all the asserts will truly never trigger, then I'd ask that the return -1 here be replaced with abort(), to appease gcc.

On the other hand, if the assertions should be called, I'd request that they be replaced with non-conditional calls to a custom assert. Something like the following would work, I think:

#define sassert(e) ((e) ? (void)0 : (fprintf(stderr, "assertion failed: %s, function %s, file %s, line %d.\n", #e, __func__, __FILE__, __LINE__), abort()))

If you'd rather not make any of these changes, I'll just locally modify Gargoyle to call abort() in the path that causes gcc to complain.

[angstsmurf]

Thanks! The GetNibbleTrackLength() function is modified from the original Ciderpress source, but I was apparently too careless when cobbling together the version used in ScottFree. I guess the original has other code in place to handle the -1 path.

As the function currently only either returns kTrackLenNib525 (6656) or asserts, it is pretty redundant, so I think the best solution is to remove it altogether. I'll make a new pull request over at the garglk repository.

[cspiegel]

OK, merged into garglk. Thank you!

[angstsmurf]

Fixed in d61b0a1. Thanks for the repor!