libember_slim: fields == GlowFieldFlag_All does not always work

[mywave82]

Enumeration GlowFieldFlags had member GlowFieldFlag_All defined to be 0xffffffff. For some variants of gcc, this will be expanded to 64bit in order to represent this 32bit number, since enumeration defaults to signed integer.

This becomes a problem when incomming signed 32bit integer flags, are typecasted, causing sign extension (value will stored is then 0xffffffffffffffff), so parsers that checks if packet.fields == GlowFieldFlag_All fails, since 0xffffffffff != 0xffffffffffffffff.

Easiest fix for now is this patch for now is this patch.

Best permanent fix will probably to define GlowFieldFlags as a uint32_t, and the different flags as #define, to ensure bitsize and avoid sign-extensions. Please enter the commit message for your changes. Lines starting

[mywave82]

before this patch, sizeof() gave 8, afterward it gave 4 gcc 4.6.3 (older raspbian version)

0xffffffff is not -1, but 4294967295

[mywave82]

Further investigation:

Problem is when mixing -2 and 0xffffffff in the same enum, since the first is an signed number, and the hex one an unsigned, and that makes the range span 32 bits + sign, so it needs 64 bit to store it

#include <stdio.h>

enum a
{

        a_1 = 0,
        a_2 = 0xffffffff
};

enum b
{
        b_1 = 0,
        b_2 = -1
};

enum c
{
        c_1 = 0,
        c_2 = -1,
        c_3 = 0xffffffff
};

int main(int argc, char *argv[])
{
        fprintf (stderr, "%ld %ld %ld\n", sizeof (enum a), sizeof (enum b), sizeof (enum c));
        return 0;
}

$ ./test
4 4 8

[mywave82]

I can make a new PR with a cleaner commit message, after you decide if you want them in hex, or negative numbers. I assume you prefer them in hex, without the typecast in-front

[KimonHoffmann]

To be able to judge the best variant to solve this accurately I need more information regarding one statement from the original summary of this PR:

This becomes a problem when incomming signed 32bit integer flags, are typecasted, causing sign extension (value will stored is then 0xffffffffffffffff), so parsers that checks if packet.fields == GlowFieldFlag_All fails, since 0xffffffffff != 0xffffffffffffffff.

Which function is this statement referring to? Does this function return a signed or an unsigned integral type? And lastly does the function return an explicitly sized integral type, such as int32_t?

[mywave82]

glow_writeQualifiedBarnfindMatrixImpl1 is based on glow_writeQualifiedMatrixImpl (have modified it, to use multiple glow packages, since our matrix node was too big to fit into a single package)

LIBEMBER_API void glow_writeQualifiedBarnfindMatrixBase(GlowOutput *pOut,
                                                        const BarnfindGlowMatrix *pMatrix,
                                                        GlowFieldFlags fields,
                                                        const berint *pPath,
                                                        int pathLength)
{
   ASSERT(pOut != NULL);
   ASSERT(pMatrix != NULL);
   ASSERT(pPath != NULL || pathLength == 0);
   ASSERT(pOut->positionHint == 0);

   glow_writeQualifiedBarnfindMatrixImpl1(&pOut->base.base.base, pMatrix, fields, pPath, pathLength);
}

LIBEMBER_API void glow_writeQualifiedBarnfindMatrixImpl1(BerOutput *pOut,
                                                         const BarnfindGlowMatrix *_pMatrix,
                                                         GlowFieldFlags fields,
                                                         const berint *pPath,
                                                         int pathLength);
{
/* removed non-important stuff */
   if(fields == GlowFieldFlag_All)
   {
     // This is never reached
     ember_writeInteger(pOut, &glowTags.matrixContents.type, pMatrix->type);

     ember_writeInteger(pOut, &glowTags.matrixContents.addressingMode, pMatrix->addressingMode);
   } else {
     // however this was just hit, before GDB breakpoint
     fprintf (stderr, "fields != GlowFieldFlag_All   fields=0x%llx GlowFieldFlag_All=0x%llx\n", fields, GlowFieldFlag_All);
   }
/* removed non-important stuff */
}

fields != GlowFieldFlag_All   fields=0xffffffffffffffff GlowFieldFlag_All=0xffffffff

// we can see how gcc treat these two, and they do not match

gdb$ bt
#0  glow_writeQualifiedBarnfindMatrixImpl1 (pOut=0x7efff2e4, _pMatrix=0x67740, fields=GlowFieldFlag_All, pPath=0x7efff264, pathLength=0x3) at tree.c:582
#1  0x00024100 in glow_writeQualifiedBarnfindMatrixBase (pOut=0x7efff2e4, pMatrix=0x67740, fields=GlowFieldFlag_All, pPath=0x7efff264, pathLength=0x3) at tree.c:743
#2  0x00009bc4 in onCommand (pCommand=0x246ad8, pPath=0x246a54, pathLength=0x2, state=0x2469d8) at client.c:272
#3  0x0002b248 in onItemReady_Command (pThis=0x246a10) at ../externals/ember-plus/libember_slim/Source/glowrx.c:470
#4  0x0002d61c in onItemReady (pBase=0x246a10) at ../externals/ember-plus/libember_slim/Source/glowrx.c:1187
#5  0x00027f80 in endContainer (pThis=0x246a10) at ../externals/ember-plus/libember_slim/Source/emberasyncreader.c:359
#6  0x00028238 in emberAsyncReader_readByte (pThis=0x246a10, b=0x0) at ../externals/ember-plus/libember_slim/Source/emberasyncreader.c:465
#7  0x000282a0 in emberAsyncReader_readBytes (pThis=0x246a10, pBytes=0x246c4f "", count=0xd) at ../externals/ember-plus/libember_slim/Source/emberasyncreader.c:474
#8  0x0002da48 in onPackageReceived (pPackage=0x246c21 "", length=0x3b, state=0x246a10) at ../externals/ember-plus/libember_slim/Source/glowrx.c:1315
#9  0x00028ffc in readFramedByte (pThis=0x246bd8, b=0xff) at ../externals/ember-plus/libember_slim/Source/emberframing.c:451
#10 0x000291e8 in emberFramingReader_readBytes (pThis=0x246bd8, pBytes=0x7efff494 "\377\376", count=0xe) at ../externals/ember-plus/libember_slim/Source/emberframing.c:530
#11 0x0002dc20 in glowReader_readBytes (pThis=0x246a10, pBytes=0x7efff454 "\376", count=0x4e) at ../externals/ember-plus/libember_slim/Source/glowrx.c:1379
#12 0x0000a2f8 in client_callback (data=0x2469d8, revents=0x1) at client.c:456
#13 0x0000bc9c in main_loop_fd_iterate (timeout_ms=0xffffffff) at main-loop.c:193
#14 0x0000b4f4 in main (argc=0x2, argv=0x7efff804) at main.c:256

And the source for fields being 0xffffffffffffffff is here

static void onItemReady_Command(NonFramingGlowReader *pThis)
{
   const BerReader *pBase = &pThis->base.base;

   if(pBase->isContainer)
   {
      if(pThis->glow.command.number == GlowCommandType_GetDirectory
      && pThis->glow.command.options.dirFieldMask == 0)
      {
         fprintf (stderr, "onItemReady_Command: dirFieldMask was 0, setting to GlowFieldFlag_All\n");
         pThis->glow.command.options.dirFieldMask = GlowFieldFlag_All;
      }

      if(pThis->onCommand != NULL)
         pThis->onCommand(&pThis->glow.command, pThis->path, pThis->pathLength, pThis->state);

      glowCommand_free(&pThis->glow.command);
   }
   else
   {
      if(berTag_equals(&pBase->tag, &glowTags.command.number))
      {
         pThis->glow.command.number = berReader_getInteger(pBase);
      }
      else if(berTag_equals(&pBase->tag, &glowTags.command.dirFieldMask))
      {
         pThis->glow.command.options.dirFieldMask = (GlowFieldFlags)berReader_getInteger(pBase);
         fprintf (stderr, "onItemReady_Command: just received glow.command.options.dirFieldMask=%llx\n", pThis->glow.command.options.dirFieldMask);
      }
      else
      {
         if(pThis->onUnsupportedTltlv != NULL)
            pThis->onUnsupportedTltlv(pBase, pThis->path, pThis->pathLength, GlowReaderPosition_Command, pThis->state);
      }
   }
}

Gives this output:

onItemReady_Command: just received glow.command.options.dirFieldMask=ffffffffffffffff

[mywave82]

So, should I make a new PR, using all hex-values and a clean commit message?

[KimonHoffmann]

I finally had some time to evaluate this topic a little bit and came up with this test on Godbolt: https://godbolt.org/z/XdT2_v

According to the output (tested in 64 bit as well as 32 bit builds) variant A exhibits the behavior described, when mixing signed negative literals and unsigned literals that have the MSB set.

All remaining variants (B through E) should produce the desired result, so I'm inclined to favor variant B or E over the other solutions proposed.

[KimonHoffmann]

This PR has been obsoleted by #100.