r261448 - Lex: Check buckets on header map construction

[Duncan P. N. Exon Smith via cfe-commits]

> On 2016-Feb-22, at 10:39, Adrian Prantl <aprantl at apple.com> wrote:
> 
>> 
>> On Feb 20, 2016, at 1:00 PM, Duncan P. N. Exon Smith via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>> 
>> Author: dexonsmith
>> Date: Sat Feb 20 15:00:58 2016
>> New Revision: 261448
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=261448&view=rev
>> Log:
>> Lex: Check buckets on header map construction
>> 
>> If the number of buckets is not a power of two, immediately recognize
>> the header map as corrupt, rather than waiting for the first lookup.  I
>> converted the later check to an assert.
>> 
>> Modified:
>>   cfe/trunk/lib/Lex/HeaderMap.cpp
>>   cfe/trunk/unittests/Lex/HeaderMapTest.cpp
>> 
>> Modified: cfe/trunk/lib/Lex/HeaderMap.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/HeaderMap.cpp?rev=261448&r1=261447&r2=261448&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Lex/HeaderMap.cpp (original)
>> +++ cfe/trunk/lib/Lex/HeaderMap.cpp Sat Feb 20 15:00:58 2016
>> @@ -19,6 +19,7 @@
>> #include "llvm/Support/DataTypes.h"
>> #include "llvm/Support/MathExtras.h"
>> #include "llvm/Support/MemoryBuffer.h"
>> +#include "llvm/Support/SwapByteOrder.h"
>> #include <cstdio>
>> #include <memory>
>> using namespace clang;
>> @@ -82,6 +83,15 @@ bool HeaderMapImpl::checkHeader(const ll
>>  if (Header->Reserved != 0)
>>    return false;
>> 
>> +  // Check the number of buckets.
>> +  auto NumBuckets = NeedsByteSwap
>> +                        ? llvm::sys::getSwappedBytes(Header->NumBuckets)
>> +                        : Header->NumBuckets;
>> +
>> +  // If the number of buckets is not a power of two, the headermap is corrupt.
> 
> Support/MathExtras.h defines isPowerOf2_32 which is IMHO more readable and also checks for 0 (which may or may not be what we want here).

r261585.  Thanks.

> -- adrian
>> +  if (NumBuckets & (NumBuckets - 1))
>> +    return false;
>> +
>>  // Okay, everything looks good.
>>  return true;
>> }
>> @@ -191,10 +201,8 @@ StringRef HeaderMapImpl::lookupFilename(
>>  const HMapHeader &Hdr = getHeader();
>>  unsigned NumBuckets = getEndianAdjustedWord(Hdr.NumBuckets);
>> 
>> -  // If the number of buckets is not a power of two, the headermap is corrupt.
>> -  // Don't probe infinitely.
>> -  if (NumBuckets & (NumBuckets-1))
>> -    return StringRef();
>> +  // Don't probe infinitely.  This should be checked before constructing.
>> +  assert(!(NumBuckets & (NumBuckets - 1)) && "Expected power of 2");
>> 
>>  // Linearly probe the hash table.
>>  for (unsigned Bucket = HashHMapKey(Filename);; ++Bucket) {
>> 
>> Modified: cfe/trunk/unittests/Lex/HeaderMapTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/HeaderMapTest.cpp?rev=261448&r1=261447&r2=261448&view=diff
>> ==============================================================================
>> --- cfe/trunk/unittests/Lex/HeaderMapTest.cpp (original)
>> +++ cfe/trunk/unittests/Lex/HeaderMapTest.cpp Sat Feb 20 15:00:58 2016
>> @@ -91,4 +91,13 @@ TEST(HeaderMapTest, checkHeaderValidButE
>>  ASSERT_TRUE(NeedsSwap);
>> }
>> 
>> +TEST(HeaderMapTest, checkHeader3Buckets) {
>> +  MapFile<3, 1> File;
>> +  ASSERT_EQ(3 * sizeof(HMapBucket), sizeof(File.Buckets));
>> +
>> +  File.init();
>> +  bool NeedsSwap;
>> +  ASSERT_FALSE(HeaderMapImpl::checkHeader(*File.getBuffer(), NeedsSwap));
>> +}
>> +
>> } // end namespace
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits