[llvm-commits] Review request: dominance frontier computation speedup

[Chris Lattner]

On Nov 24, 2010, at 12:21 PM, Jakob Stoklund Olesen wrote:

>> 
>> I also noticed that there is an extra pointer wasted for SmallStorage:
>> 
>> /// SmallStorage - Fixed size storage used in 'small mode'.  The extra element
>> /// ensures that the end iterator actually points to valid memory.
>> const void *SmallStorage[SmallSizePowTwo+1];
>> 
>> Isn't a pointer to 1 past valid memory valid for comparisons in C++? Nothing should be dereferencing it.
> 
> Author: Duncan Sands <baldrick at free.fr>
> Date:   Tue Jun 29 13:12:02 2010
> 
>    Fix a buffer overflow noticed by gcc-4.6: zero is written into
>    SmallArray[SmallSize] in the SmallPtrSetIteratorImpl, and this is
>    one off the end of the array.  For those who care, right now gcc
>    warns about writing off the end because it is confused about the
>    declaration of SmallArray as having length 1 in the parent class
>    SmallPtrSetIteratorImpl.  However if you tweak code to unconfuse
>    it, then it still warns about writing off the end of the array,
>    because of this buffer overflow.  In short, even with this fix
>    gcc-4.6 will warn about writing off the end of the array, but now
>    that is only because it is confused.

FWIW, the issue here is that SmallPtrSet always ensures the last element is a valid one by setting it to null:

void SmallPtrSetImpl::shrink_and_clear() {
...
  // The end pointer, always valid, is set to a valid element to help the
  // iterator.
  CurArray[CurArraySize] = 0;
}


Without this, the iterator (which is currently just a Bucket*) would have to be increased to keep track of the end of the array, because of:

  /// AdvanceIfNotValid - If the current bucket isn't valid, advance to a bucket
  /// that is.   This is guaranteed to stop because the end() bucket is marked
  /// valid.
  void AdvanceIfNotValid() {
    while (*Bucket == SmallPtrSetImpl::getEmptyMarker() ||
           *Bucket == SmallPtrSetImpl::getTombstoneMarker())
      ++Bucket;
  }

I don't know a way to get rid of the extra slot without making the iterator more expensive.

-Chris