touchtypes.c:195:9: warning: array index 8 is past the end of the array

[dlmiles]

While building

Source line affected:
https://github.com/RTimothyEdwards/magic/blob/master/utils/touchtypes.c#L195
Line:
TTMaskSetType(&(parms->tfp_types),TT_SUBCELL);

clang18 warning:
touchtypes.c:195:9: warning: array index 8 is past the end of the array (that has type 'unsigned int[8]') [-Warray-bounds]

There is a special note near definition of TT_SUBCELL about it being a pseudo-value but it is being applied as-is.

When building with clang18

Macro/Preprocessor/Mathematical Working:

#define TT_MASKWORDS    ((TT_MAXTYPES + TT_BPW - 1) / TT_BPW)

                        // (256 + 32 - 1) / 32 = 8 as truncated integer

typedef struct
{
    unsigned int        tt_words[TT_MASKWORDS];
} TileTypeBitMask;


typedef struct touchingfuncparms
{
    Point               tfp_point;
    TileTypeBitMask     tfp_types;
} TouchingFuncParms;

TouchingFuncParms *parms = (TouchingFuncParms *) cdarg;


#define TT_MAXTYPES             256
/* Pseudo type signifying unexpanded subcells.  Never painted.  -  Only
   used in a few places, e.g.  TouchingTypes() and mzrouter spacing arrays.
 */
#define TT_SUBCELL      TT_MAXTYPES

-DSIZEOF_UNSIGNED_INT=4  // sizeof(unsigned int) == 4

#define TT_BPW          (8 * sizeof (unsigned int))
#define TT_WORDMASK     (TT_BPW - 1)

#define TT_WORDSHIFT    5                       /* LOG2(TT_BPW) */

#define ttBit(t)        ((t) & TT_WORDMASK)
#define ttMask(t)       ((unsigned int)1 << ttBit(t))

#define ttWord(t)       ((t) >> TT_WORDSHIFT)

#define TTMaskSetType(m, t)     ((m)->tt_words[ttWord(t)] |= ttMask(t))

utils/touchtypes.c:195
        TTMaskSetType(&(parms->tfp_types),TT_SUBCELL);

based on the above info some working:
        ((m)->tt_words[(256) >> TT_WORDSHIFT] |= ((unsigned int)1 << ((256) & TT_WORDMASK))

        ((m)->tt_words[(256) >> 5] |= ((unsigned int)1 << ((256) & 31))

        ((m)->tt_words[8] |= ((unsigned int)1 << (0))

// So yes clang18 can see this access beyond end of array and cause the warning

So I'm agreeing with clang18 ((m)->tt_words[8] |= ((unsigned int)1 << (0))

Was the original programming intention to do nothing ?
(which is what happens now, since it doesn't modify data within array bounds.
assume setting bit0 of the 32bit little-endian after the end of tt_words[]
this is the same as doing nothing, since it didn't actively edit the memory
it intended. resolution comment out the line

Or maybe the programming intention was to set bit0 of the index 0 of tt_word[0] as in
tt_word[0] |= 0x1. resolution might be to use TT_SUBNET & TT_WORDMASK

[RTimothyEdwards]

I think that the point of TT_SUBCELL was to add an extra bit to the type mask for use with the interactive router; this is consistent with uses elsewhere in the irouter and mzrouter code (e.g., mzrouter.h line 67 int rt_spacing[TT_MAXTYPES + 1]; is defining an array that is one larger than the number of tile types, and then using TT_SUBCELL to index it.

I'm inclined to suggest that the proper solution is to use one of the indexes that is lower than TT_TECHDEPBASE for TT_SUBCELL, since those type numbers should never be exercised in the irouter and mzrouter code; e.g.,

#define TT_SUBCELL  TT_CHECKSUBCELL

seems like it ought to work; then all the arrays defined with TT_MAXTYPES + 1 can be reduced to TT_MAXTYPES and there will be no issues with the bitmask.

[dlmiles]

TL;DR the current best understanding is.

Yes this will stomp over a byte-outside the default sizeof the mask bitmap.

It is preferable not to do this, as as to keep the mask size a nice round number of bits for SIMD operations. #377 (comment)
One way to change it would be to use a reserved bit-id for which a mechanism already exists.
The mzrouter module is thought to be the only consumer of this feature.
The mzrouter module has a special +1 addition to the structure it uses for this purpose, so it is technically not a problem there.
It is thought few people use mzrouter
In order to make changes to mzrouter really need unit tests or at least exercise the code, including the change, to then prove the change after - this is the strategy I work towards

It is low priority as there is no demonstrable test case proving incorrect behaviour.

Actions to take:

• Prove this claim, for example setup --enable-assertions and place an assert(false) and find a project as a testcase to trigger execution of this line of code.
• Confirm the conditions under which it might execute, thus creating exhibit A test case.
• Modify the code in the obvious way (using a fixed bit from a reserved group below the reserved threshold)
• Confirm the test case prove the function continue to work