A nice example of one reason to const your parameters.

I’d done a test build of our code with the clang compiler, and tried out its static analyzer. It politely pointed out that the code like the following was likely wrong:

struct cpuinfo
{
   int   pkg_id ;
   int   core_id ;
   int   smt_id ;
   int   logicalId ;

   void init( int pkg, int core, int smt, int logical )
   {
       pkg_id    = pkg ;
       core_id   = core ;
       smt_id    = smt ;
       logical   = logicalId ;
   }
} ;

$ clang --analyze -c cpuinfo.C
cpuinfo.C:13:8: warning: Value stored to 'logical' is never read
       logical   = logicalId ;
       ^           ~~~~~~~~~
1 warning generated.

In fixing this, I made two changes. The first was const’ing the parameters:

   void init( const int pkg, const int core, const int smt, const int logical )

If that had been done, this runtime error wouldn’t have been possible:

clang -c cpuinfo.C
cpuinfo.C:13:18: error: read-only variable is not assignable
       logical   = logicalId ;
       ~~~~~~~   ^
1 error generated.

Had the member variables been named consistently I doubt this error would have been made. It would have been too obvious that something was wrong:

   void init( int pkg, int core, int smt, int logical )
   {
       pkg_id    = pkg ;
       core_id   = core ;
       smt_id    = smt ;
       logical   = logical_id ;
   }

so, I also followed up the const’ing, done as a preventive maintanance action, with a search and replace to fix up the logicalId member variable:

s/logicalId/logical_id/g

and then made the actual fix in question.

I could have fixed the code only changing the one assignment line in question. The final fix ended up just touching 5 additional lines, since the code was self contained.

I got asked by the original author why I did the const’ing part of this change. It suprised me to get that question, and reminded me of the time where I was once met with a significant objection to using “new-fangled C++” features like ‘const’ in our code.

At the time, most of our code was very C-like, despite being compiled with a C++ compiler. The component owner said that he’d only consider using const if I could prove there was a performance benefit to doing so. I don’t think I ever proved that to him, but I think here’s a nice demo of why it’s a good habit in general to use const whenever there isn’t explicit intention to allow modification.

cost of a global array access?

For an array where the value of

   myGlobalArrayOfInts[ THE_ARRAY_OFFSET_FOR_OPERATION__FOO ] == THE_VALUE_FOR_OPERATION__FOO

some developer used

   int value = THE_VALUE_FOR_OPERATION__FOO ; // for performance, just set it.

instead of

   int value = myGlobalArrayOfInts[ THE_ARRAY_OFFSET_FOR_OPERATION__FOO ]

At one point in time the assumption that myGlobalArrayOfInts[ THE_ARRAY_OFFSET_FOR_OPERATION__FOO ] was never have been any different than THE_VALUE_FOR_OPERATION__FOO was true. However, this bit of premature optimization inevitably broke when maintainance programming required that this array value be changed.

The developer fixing this asked me “How expensive is this array access”. I can’t exactly quantify the cost of this, although at least half of it in this case is probably going after the address of the global itself. On AIX I believe that this may require loading the TOC register if the last global access was in a different shared lib, and there’s probably some similar notion on most other systems. I happened to be using a Linux build at that point of time, so to help quantify the cost I took a look at the asm generated for a function of the form

int foo0()
{
   return myGlobalArrayOfInts[ THE_ARRAY_OFFSET_FOR_OPERATION__FOO ] ;
}

Here’s the code that was generated

0000000000002f50 :
    2f50:   48 8b 15 00 00 00 00    mov    0x0(%rip),%rdx        # 2f57 
    2f57:   48 63 42 14             movslq 0x14(%rdx),%rax
    2f5b:   c3                      retq

We’ve essentially got two dereferences required for this array dereference, one that’s probably getting the address of the global itself. I’d guess that this mov instruction ends up rewritten by the linker later, putting in actual values for the location of this global instead of the ’00 00 00 00′ seen in the instruction dump. Then we’ve got one more dereference to get at the global value itself.

Would this really have made a measurable difference in a function that was 841 lines of code long, (1067 straight line instruction count)? Doubtful.

C++11 play. Simple hashing, a comparison with perl

I’m very impressed with how easy C++11 makes it possible to implement a simple hash

#include <set>
#include <string>
#include <iostream>

using namespace std ;

int main()
{
   set< string > d ;

   d.insert( "uu:long" ) ;
   d.insert( "uu:long" ) ;
   d.insert( "uu:long long" ) ;

   d.insert( "qq:int" ) ;
   d.insert( "Test:int" ) ;

   for ( auto & kv : d )
   {
      cout << kv << endl ;
   }
}

If I had to do hashing like this before I’d probably have seen if I could generate a text file, and then post process it with perl, where the same hashing would look like:

#!/usr/bin/perl

my %d ;

$d{'uu:long'}++ ;
$d{'uu:long'}++ ;
$d{'uu:long long'}++ ;

$d{'qq:long'}++ ;
$d{'Test:int'}++ ;

foreach ( keys %d )
{
   print "$_\n" ;
}

Other the the header includes and namespace statement, it’s not really any harder to do this in C++ now. I just have to wait 5 years before all our product compilers catch up with the standard, I could start thinking about using this sort of code in production. Unfortunately in production I’d also have to deal with exceptions, and the (hidden) fact that the std::allocator is not generally appropriate for use within DB2 code.

ease of getting strncat wrong.

I’d done a test build with the clang compiler of our db2 code and fired off some defects and emails to some of the code owners about messages from that compiler like:

foo.C:1220:15: warning: the value of the size argument in ‘strncat’ is too large, might lead to a buffer overflow [-Wstrncat-size]

It’s a great message, clear and unambiguous: “your code is wrong”!

Here’s a sample of the code in question that clang was complaining about:

   strncat( dirPath, ptrToDirectories, ( sizeof( dirPath ) - strlen( dirPath ) ) );

The compiler could just as well complain about simpler strncat calls.  Imagine at first that dirPath was all zeros, then the above code would be equivalent to:

   strncat( dirPath, ptrToDirectories, sizeof( dirPath ) ) ;

It’s not immediately obvious reading the strncat man page that this is wrong, since that man page says “[strncat] will use at most n characters from src”. However, it also says “As with strcat(), the resulting string in dest is always null terminated.”

Does this mean that it will use no more than n characters from src, but that it null terminates within the range of those n-characters, or does this mean use (if strlen(src) > n) n characters from src and then add a null terminator after that?

To answer that question, and rule out a possible false positive warning from the compiler I wrote the following simple bit of test code:

#include <string.h>
#include <stdio.h>

struct string
{
   char dest[2] ;
   char overflow ;
} ;

int main()
{
   string o ;
   o.dest[0] = 0  ;
   o.dest[1] = 1  ;
   o.overflow = 1 ;

   printf( "%d\n", (int)o.overflow ) ;

   strncat( o.dest, "abc", sizeof(o.dest) ) ;

   printf( "%d\n", (int)o.overflow ) ;

   return 0 ;
}

And the verdict was:

$ g++ -g t.C ; a.out
1
0

Yup, the code was bad, and the compiler is without fault. We actually have a different function for strncat’ing in our code that takes the actual buffer size, to make this harder to get wrong, but the code in question was not using that.

macro expansion order, and evil macros producing commas.

I was looking at a bit of code, the problematic portion, after wading through many layers of nested macro hell, was roughly of the form:

    void traceFunction( int n, ... )
    {
    }

    int main()
    {
       #define TUPLE( size, arg )   (size), (arg)

       #define traceInt( sz1, ptr1 )    traceFunction( 1, sz1, ptr1 )
       #define trace( arg1 )            traceInt( arg1 )

       int x = 3 ;

       trace( TUPLE( sizeof(x), &x ) ) ;

       return 0 ;
    }

Observe that one of the macros produces a comma separated list of arguments, and that the `trace` macro, which ends up actually called with two parameters (the result of the `TUPLE` macro), is declared as if it has a single parameter.

Somewhat mysteriously, this actually compiles on many different platforms and compilers (about 16 different combinations), but breaks with the intel 13 compiler beta (although that compiler does have a compatibility mode that I’d prefer not to depend on)

I figured I could fix this with:

    #define trace( sz1, ptr1 )       traceFunction( 1, sz1, ptr1 )

eliminating the middle man, but this gives me, for C++:

t.C(23): error: identifier “trace” is undefined

and for C compilation:

t.c(23): error #54: too few arguments in invocation of macro “trace”

error, indicating that the attempt to expand the macro `trace` occurs before the attempt to expand the `TUPLE` macro. I think I can fix this provided I rely on C99 macro varargs like so:

    #define traceInt( sz1, ptr1 )    traceFunction( 1, sz1, ptr1 )
    #define trace( ... )             traceInt( __VA_ARGS__ )

That’s likely an acceptable solution, given that we’ve now got other dependencies on C99 __VA_ARGS__ in the code.

It appears that, rather luckily, I never needed to know exactly what order nested macro expansion happens in before this.

very deceptive indenting.

Check out the following mismatched indenting (counting carefully if it doesn’t stand out obviously … it didn’t to me) :

#ifdef SQLUNIX
   #ifdef OSS_AIXPPC
      #ifdef OSS_ARCH_P64
         #define SQLO_SHR_OBJECT "(shr_64.o)"
      #else
         #define SQLO_SHR_OBJECT "(shr.o)"
    #endif
#endif

The ending #endif was actually 550 lines away in the file!

Poking around to see how much stack to corrupt to alter a local variable.

I’ve got a scenerio where it appears that the last stack variable declared appears to be have been corrupted (the highest order 32-bits of this 64-bit integer look like they’ve been zeroed). That got me wondering how far a calling function would have to corrupt to muck up this variable. Here’s what I wrote to test this:

#include <stdio.h>

int foo( int r )
{
   Uint64         x ;
   Uint64         y ;
   Uint64         z ;
   Uint64         w = 0 ;

   w = 1 ;

   printf( "&x: 0x%0lx\n", (long)&x ) ;
   printf( "&w: 0x%0lx\n", (long)&w ) ;

   if ( r )
   {
      foo( r - 1 ) ;
   }

   return w ;
}

int main()
{
   foo( 2 ) ;

   return 0 ;
}

and the results on this (linuxamd64) system:

&x: 0x7fffffffd490
&w: 0x7fffffffd488
&x: 0x7fffffffd450
&w: 0x7fffffffd448
&x: 0x7fffffffd410
&w: 0x7fffffffd408

So, it looks like I need about at least a (0×88-0×50 =) 56 byte corruption to do the job.

A quirk: Also see how the variables in my function actually got laid out in reverse address order on the stack. I’d not have expected that. However, since I don’t really have any reason to expect any specific stack layout so perhaps I shouldn’t be surprised.

How to find exported symbols in Windows dlls

One liner:

WSDB::E:\snap\> dumpbin /exports db2app64.dll | grep DiagWhat
        510  212 00BD1152 pdDiagWhatIsRc = pdDiagWhatIsRc

This isn’t an nm equivalent, instead is more like the AIX command to dump just the exported symbols from a shared object (dump -TvHX32_64), but enough to tell me that I shouldn’t have a link error this iteration of my build.

I found it curious that ‘dumpbin /symbols’ didn’t produce any output for this dll, as is does for a .obj file, and don’t really know what the reason for that is.

C++ syntax, nested loops and continue — an embarrassing experiment?

For whatever reason, the C++ continue keyword is not used often in the portions of the code I normally deal with.  I was moving code in a function that had gotten waaay to big into a helper function. This changed the scope of some continue and break usage, and thought I’d double check that these only impact the inner loop:

#include <stdio.h>

int main()
{
   for ( int i = 0 ; i < 5 ; i++ )
   {
      for ( int j = 0 ; j < 5 ; j++ )
      {
         if ( j % 2 )
         {
            continue ;
         }

         printf( "i,j = %d, %d\n", i, j ) ;
      }
   }

   printf( "\n\n" ) ;
   for ( int i = 0 ; i < 5 ; i++ )
   {
      for ( int j = 0 ; j < 5 ; j++ )
      {
         if ( 3 == j )
         {
            break ;
         }

         printf( "i,j = %d, %d\n", i, j ) ;
      }
   }

   return 0 ;
}

I see:

i,0 = 0, 0
i,2 = 0, 2
i,4 = 0, 4
i,0 = 1, 0
i,2 = 1, 2
i,4 = 1, 4
i,0 = 2, 0
i,2 = 2, 2
i,4 = 2, 4
i,0 = 3, 0
i,2 = 3, 2
i,4 = 3, 4
i,0 = 4, 0
i,2 = 4, 2
i,4 = 4, 4


i,0 = 0, 0
i,1 = 0, 1
i,2 = 0, 2
i,0 = 1, 0
i,1 = 1, 1
i,2 = 1, 2
i,0 = 2, 0
i,1 = 2, 1
i,2 = 2, 2
i,0 = 3, 0
i,1 = 3, 1
i,2 = 3, 2
i,0 = 4, 0
i,1 = 4, 1
i,2 = 4, 2

This makes logical sense, and protects me from breaking code containing the inner loop in question should I move it to a helper function (where there will no longer be an outer loop in scope).

Should it be embarrassing that I actually did this experiment after so many years of programming?

Ease of screwing up C string operations.

Here’s a new way to mess up a strncpy that I hadn’t seen before. I found about 50 instances of this in the component I was working on today:

   ossStrNCopy( pszTarget, pszSource, strlen( pszSource ) + 1 ) ;

Our ossStrNCopy function is a lot like strncpy, but unlike the library interface, ours is explicitly truncating and won’t leave a dangling non-null terminated string. It’s supposed to be called like:

   ossStrNCopy( pszTarget, pszSource, sizeof(pszTarget) ) ;

(where pszTarget is a char [] array and not a char *). The idea is to avoid buffer overflows that would occur if you were to copy more bytes than the target can hold.

This incorrect use is an excellent example of what was probably clueless cut and paste. Perhaps the thought process was: “I don’t know what this last parameter is for. It’s too much work to read the documentation … I’ll just put something.”

The caller is requesting to have one plus the null terminator of the source string copied into the target. If you are going to do that, why not just call strcpy?