trafodion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dave Birdsall <dave.birds...@esgyn.com>
Subject Set_SqlParser_Flags
Date Wed, 18 May 2016 21:02:21 GMT
Hi,



In parser/SqlParserGlobalsCmn.h are three functions:



  inline static void Set_SqlParser_Flags(ULng32 flagbits)

  {

    if (flagbits)

      SqlParser_Flags |= flagbits;

    else

      SqlParser_Flags = 0;

  }



  inline static void Assign_SqlParser_Flags(ULng32 flagbits)

  {

    SqlParser_Flags = flagbits;

  }



inline static void Reset_SqlParser_Flags(ULng32 flagbits)

  {

    if (flagbits)

      SqlParser_Flags &= ~flagbits;

    else

      SqlParser_Flags = 0;

  }



These functions have some error-prone semantics. First problem is that they
modify a global variable, but I’ll leave that discussion for another day.
The second problem is some odd semantics. Set_SqlParser_Flags turns on
bits, unless you pass it a zero value. Then it turns everything off.
Reset_SqlParser_Flags turns off some bits, unless you pass it a zero value.
Then it turns off EVERY bit. The third problem is that the name
“Set_SqlParser_Flags” is misleading. I have debugged more than one set of
code where the author appears to have thought that this function set ALL
the bits to his particular input value.



Here’s an example of a bug that I debugged today:



Class1::method1()

{

  ULng32 savedParserFlags = Get_SqlParser_Flags(0xFFFFFFFF);

   Set_SqlParser_Flags(0x100000); // ORs this bit into the flags



   … do a bunch of stuff …



   // Restore parser flags to prior settings.

   Set_SqlParser_Flags(savedParserFlags);

}



Here’s what this code does. If the parser flags happen to be zero on entry,
the savedParserFlags variable gets a value of zero. The first
Set_SqlParser_Flags call turns on bit 0x100000. The last
Set_SqlParser_Flags call resets the parser flags to zero *because of the
weird semantic concerning a zero value.*



If the parser flags happen to be non-zero on entry, the savedParserFlags
variable gets a non-zero value. The first Set_SqlParser_Flags call turns on
the 0x100000 bit. The last Set_SqlParser_Flags call *does nothing!* Why?
Because we pass a non-zero value, it just turns on those bits. But those
bits were already on! Notice that the 0x100000 bit remains on at exit, in
contrast to the first case above where it is reset to zero.



It’s clear from the comment that the author intended to restore the parser
flags to their previous state. And most of the time, the parser flags are
zero, so the code works as intended. But if there is another bug somewhere
else that leaves a bit set, then this code will leave another bit set
unintentionally, and we snowball.



The bug is, the author should have used Assign_SqlParser_Flags for the last
call, instead of Set_SqlParser_Flags.



Debugging mis-set global variables is hard in general, and this is the
second instance of this kind of bug that I’ve run into.



I think the misleading function names confuse developers as to the right
call to use. And the weird zero semantics tends to hide the bugs so they
occur only in weird situations.



So I would like to propose the following refactoring:



1.       Rename the functions. Set_SqlParser_Flags à
TurnOn_SqlParser_FlagBits, and similarly for Reset. Leave
Assign_SqlParser_Flags as-is.

2.       Remove the weird zero semantics. In TurnOn_SqlParser_FlagBits, if
a zero value is passed in, it does nothing. This is more mathematically
consistent; it is the behavior of the Boolean OR operator, and the Set
UNION operator. Similarly, in TurnOff_SqlParser_FlagBits, if a zero value
is passed in, it does nothing. That’s more like a Set DIFFERENCE.

3.       Go through the Trafodion code, looking for places that call
Set_SqlParser_Flags that seem to intend to assign rather than OR, and
replace those calls with Assign_SqlParser_Flags.



If the community does not object, I will create a JIRA for this work and
assign it to myself.



In the meantime, please be careful when coding Set_SqlParser_Flags calls.



Thanks,



Dave

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message