Fix undefined behaviour in OctNode

[RLThomaz]

Fixes an undefined behaviour in pcl::poisson::OctNode due to bit shifting greater than the number of bits of the type.

To check the fix please compile the following code with -fsanitize=undefined. It should throw runtime errors for the first function but not the second.

Note: I had to decrease the loop from i<32 to i<31 since i==31 would also cause an undefined behaviour for 64-bit variables.

#include <iostream>
#include <stdio.h>

long long _InterleaveBits( int p[3] )
{
  long long key = 0;
  for( int i=0 ; i<32 ; i++ ) 
  {
    key |= ( ( p[0] & (1<<i) )<<(2*i) ) | ( ( p[1] & (1<<i) )<<(2*i+1) ) | ( ( p[2] & (1<<i) )<<(2*i+2) );
  }
  return key;
}

long long _FixedInterleaveBits( int _p[3] )
{
  long long key = 0;
  long long p[3] = {_p[0],_p[1],_p[2]};
  for( int i=0 ; i<31 ; i++ ) 
  {
    key |= ( ( p[0] & (1ull<<i) )<<(2*i) ) | ( ( p[1] & (1ull<<i) )<<(2*i+1) ) | ( ( p[2] & (1ull<<i) )<<(2*i+2) );
  }
  return key;
}

int main() {
  int p[3] = {0,255,128};
  std::cout << _InterleaveBits(p) << std::endl;
  std::cout << _FixedInterleaveBits(p) << std::endl;
}

[RLThomaz]

@kunaltyagi, this is the pull request as we discussed. Please, note the change in the loop. Also, I have no idea why the pull request has my commits from the past PR, although the files are not there.

[kunaltyagi]

You might want to merge your master with the upstream and rebase using git rebase -i --onto <target: upstream/master> <commit before the one you want to keep>.

Example command:

git fetch upstream
git checkout my_branch
git rebase -i --onto upstream/master origin/master

Confirm using git log

[kunaltyagi]

Would it make sense to deprecate current _InterleaveBits in favor of std::int32_t and std::int64_t versions? Maybe send the non-deprecated one in a detail namespace

[RLThomaz]

I think so, since long long, and int are system dependent, right? Moving to std::intXX_t would be safer. But would we need a 32-bit version for this application?

[kunaltyagi]

long long, and int are system dependent, right?

Only int is system dependent. For 64 bit systems, long long is compulsorily 64 bits (LLP64, LP64, ILP64, SILP64)

Clarification

I meant std::int64_t as return value (aka long long int) and std::int32_t AND std::int64_t as input values. I noticed one singular form instead of ones. Sorry for the confusion. This is needed since some platforms have int as 64 bits (ILP64, SILP64)

But would we need a 32-bit version for this application?

Long story short: We do compile using a 32 bit compiler, but don't worry about this.

Long story: 32 bit compiler wouldn't affect anything since 64 bit arithmetic is simulated in software by the compiler using 2 32-bit words. 64 bits are guaranteed by the language spec contingent on a implementation in stdint.h (along with compiler support for in software simulation) sample header. If 64 bits are missing, long long by itself becomes a compiler error as per C99 standard. So, we're ok changing to fixed integer sizes.

[RLThomaz]

Ok, I got it now. I agree with having the std::int64_t as both input and output, although it would change the code more than just adding it and deprecating the old one.

Regarding the std::int32_t for input, which would be similar to what we have now, should we continue constructing a std::int64_t inside the function? That is:

std::int64_t _p[3] = {p[0],p[1],p[2]};

[kunaltyagi]

Yeah. And then call the 64-bit version. Seems like no adverse effect on godbolt

[RLThomaz]

Ok, cool. The issue will be naming the new functions since std::int32_t == int so one function would be a redefinition of the deprecated.

Maybe send the non-deprecated one in a detail namespace

I guess this is a solution, where should this namespace sit? Should it be in-place?

namespace pcl
{
  namespace poisson
  {
    .
    .
    .
    namespace detail
    {
      std::int64_t _InterleaveBits( std::int64_t p[3] ) ...
      std::int64_t _InterleaveBits( std::int32_t p[3] ) ...
    }
    [[deprecated("use detail::_InterleaveBits() overload that takes std::int32_t or std::int64_t instead.")]]
    long long _InterleaveBits( int p[3] ) ...
    .
    .
  }
}

[kunaltyagi]

Yeah. the deprecated one just calls the non deprecated one (magically works). And so do the actual call-sites. I think there are only 2 or 3 of them.

Maybe rename the functions in detail simply interleaveBits? The sole reason for that naming could be making it weird for the public API.

I wish we could say something like private API use. Will be removed in the future. We should not tell people to use a non-public API function. @taketwo what do you think?

EDIT: I think this is a thumbs up for the idea in general

[taketwo]

Maybe rename the functions in detail simply interleaveBits?

👍

I wish we could say something like private API use. Will be removed in the future. We should not tell people to use a non-public API function. @taketwo what do you think?

Not sure I understand the question. For me, everything in detail namespace is mmm... implementation detail, and as such is subject to change without notice.

[kunaltyagi]

This function isn't in detail namespace, but in the hpp aka implementation file. It's implicit that it was not for public consumption. Also, the naming is hinting towards that.

[RLThomaz]

Sorry, I was away this weekend. Should we keep it as it is then? Since this was merged already, I guess we assume this function as an implementation detail so we don't need to deprecate it or create alternatives, right?

[RLThomaz]

You might want to merge your master with the upstream and rebase using git rebase -i --onto <target: upstream/master> <commit before the one you want to keep>.

Example command:

git fetch upstream
git checkout my_branch
git rebase -i --onto upstream/master origin/master

Confirm using git log

Cool, thanks! I had to force the push after all, but I guess I did not break anything.

[kunaltyagi]

Forgot to say that you have to force push, but yeah. Nicely done!! Just 1 commit now