[PATCH] Teach CastSizeChecker about flexible arrays

[Jordan Rose]

On Feb 13, 2014, at 15:23 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

> Hi Jordan,
> 
> On Thu, 2014-02-13 at 09:18 -0800, Jordan Rose wrote:
>> +static bool evenFlexibleArraySize(ASTContext &Ctx, CharUnits
>> regionSize,
>> +                                  CharUnits typeSize, QualType
>> ToPointeeTy) {
> 
>> LLVM naming conventions require all variables and parameters to start
>> with a capital letter. (This occurs throughout the function.)
> 
> Should I fix the rest of the file as well or should that be done in a
> separate commit (if it should be done at all)?

No, just make sure new code is conforming. We don't usually reformat entire existing files, only code that's been touched. (The idea is that "someday" we'll have a code style tool that we can run over the entire code base to enforce the naming conventions, but for now we're letting things be to avoid revision churn.)



>> +  const RecordType *RT =
>> ToPointeeTy.getTypePtr()->getAs<RecordType>();
> 
>> QualType has an overloaded operator->, so you can just say
>> "ToPointeeTy->getAs<RecordType>()".
> 
> 
>> +  FieldDecl *last = 0;
> 
>> Within the analyzer, we try to make all AST class pointers const. (I
>> actually try to do that everywhere I can. Const-correctness is a nice
>> thing.)
> 
>> 
>> +  for (; iter != end; ++iter)
>> +    last = *iter;
> 
>> What if the struct is empty?
> 
> Then the size of the struct will be 0 or 1, depending on C or C++. Types
> with these sizes will be handled in checkPreStmt and never reach
> evenFlexibleArraySize. But I agree it is far from obvious and have added
> a check to make sure last has been set.

Ah, good point. Yes, an assert would still be nice. (And a test case, if we don't have one.)


>> 
>> 
>> +  const Type *elemType =
>> last->getType()->getArrayElementTypeNoTypeQual();
> 
>> It looks like you could move this (and the flexSize calculation) to
>> after you've already checked that this record has a flexible or
>> zero-length array.
> 
> With the added support for last element of size 1 it needs to stay where
> it is.

Hm. I was thinking that it wouldn't matter, but it does if someone tries to allocate a struct where the trailing length is actually 0. That's probably not usually a good idea for other reasons (if you tried to load from a pointer to such an allocation, you could get in trouble), but I guess we still shouldn't warn. Okay.


> Thanks for your feedback, attached is a new version of the patch.

Committed (with very minor changes) in r201583!

Jordan