trafodion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hans Zeller <hans.zel...@esgyn.com>
Subject Re: Set_SqlParser_Flags
Date Thu, 19 May 2016 15:08:16 GMT
+1 it would be good to move (more or less gradually) to the new methods
that are easier to understand.

Hans

On Wed, May 18, 2016 at 3:55 PM, Dave Birdsall <dave.birdsall@esgyn.com>
wrote:

> Hi,
>
> I'm concerned that these bugs propagate to some extent by code cloning. For
> that, documentation doesn't help, and a review of all calls is still
> necessary to make sure there are no bugs to clone. The example I debugged
> today had a copy with exactly the same variable name and comment so I
> assume
> it was cloned.
>
> By the way, the earlier example I debugged some months ago is here:
> https://issues.apache.org/jira/browse/TRAFODION-1678.
>
> Perhaps I can make a slightly different proposal?
>
> Perhaps we provide replacements for Set_SqlParser_Flags and
> Reset_SqlParser_Flags that have cleaner semantics. But leave the old ones
> there. And we can encourage folks to change their usage over time.
>
> What I have in mind is something like:
>
> inline static ULng32 TurnOn_SqlParser_Flags(ULng32 flagbits)
> {
>    ULng32 newlyOn = flagbits & ~SqlParser_Flags; // remember the bits that
> weren't already turned on
>    SqlParser_Flags |= flagbits;
>    return newlyOn;
> }
>
> inline static void TurnOff_SqlParser_Flags(ULng32 flagbits)
> {
>    SqlParser_Flags &= ~flagbits;
> }
>
> The usage model is like this:
>
> Class1::method1()
> {
>     ULng32 turnOffOnExit = TurnOn_SqlParser_Flags(<whatever bits I want to
> turn on>);
>
>     ... do some stuff ...
>
>     TurnOff_SqlParser_Flags(turnOffOnExit);
> }
>
> This is in comparison to current examples:
>
> Class1::method1()
> {
>   ULng32 savedParserFlags = Get_SqlParser_Flags(0xFFFFFFFF);  // have to
> remember to use 0xFFFFFFFF here
>
>    Set_SqlParser_Flags(<whatever bits I want to turn on but it needs to be
> non-zero to work right>);
>
>    … do a bunch of stuff …
>
>    // Restore parser flags to prior settings.
>
>    Assign_SqlParser_Flags(savedParserFlags);  // note Assign, not Set
> }
>
> Or, the following, which is incorrect since it doesn't take into account
> that some of the bits might have already been on when the method is
> entered:
>
> Class1::method1()
> {
>     Set_SqlParser_Flags(<whatever bits I want to turn on but it needs to be
> non-zero to work right>);
>
>     ... do some stuff ...
>
>     Reset_SqlParserFlags(<whatever bits I turned on above>);
> }
>
> What do you think?
>
> Dave
>
> -----Original Message-----
> From: Anoop Sharma [mailto:anoop.sharma@esgyn.com]
> Sent: Wednesday, May 18, 2016 2:36 PM
> To: dev@trafodion.incubator.apache.org
> Subject: RE: Set_SqlParser_Flags
>
> Set and Reset parserflags have been in use for a long time, probably for
> multiple years.
>
> Initially, use of parserflags was very limited and restricted to parser
> component only. And the current Set and Reset functionality was correct for
> that usage.
>
> But over time, parserflags started to get used in many other components and
> code.
>
> Some of the places did use Set incorrectly as has been mentioned Dave's
> email.
> The method AssignParserFlags method was added later to correctly save and
> restore parserflags, if that is what was needed.
>
> We need to be bit careful if all of traf code is to be fixed for Set/Reset
> usage. One would need to make sure that they understand it well in terms of
> its impact on the immediate code as well as surrounding code.
> It may be good to do that though at the same time I don’t think the code is
> broken because of incorrect usage of parserflags.
>
> It maybe good enough to document the Set/Reset/Assign functionality so
> folks
> who are adding new code do the right thing.
>
> anoop
>
> -----Original Message-----
> From: Dave Birdsall [mailto:dave.birdsall@esgyn.com]
> Sent: Wednesday, May 18, 2016 2:02 PM
> To: dev@trafodion.incubator.apache.org
> Subject: Set_SqlParser_Flags
>
> 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