<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Nice to see this improvement! Some comments, but mostly just style things:</div><div><br></div><div>+/// Check if we are casting to a struct with a flexible array at the end.</div><div>+///</div><div>+/// struct foo {</div><div>+/// size_t len;</div><div>+/// struct bar data[];</div><div>+/// };</div><div>+///</div><div>+/// or</div><div>+///</div><div>+/// struct foo {</div><div>+/// size_t len;</div><div>+/// struct bar data[0];</div><div>+/// }</div><div>+///</div><div>+/// In these cases it is also valid to allocate size of struct foo + a multiple</div><div>+/// of struct bar.</div><div><br></div><div>Since these are Doxygen comments, please surround code blocks with \code...\endcode.</div><div><br></div><div><br></div><div><div>+static bool evenFlexibleArraySize(ASTContext &Ctx, CharUnits regionSize,</div><div>+ CharUnits typeSize, QualType ToPointeeTy) {</div></div><div><br></div><div><a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly">LLVM naming conventions</a> require all variables and parameters to start with a capital letter. (This occurs throughout the function.)</div><div><br></div><div><br></div><div><div>+ const RecordType *RT = ToPointeeTy.getTypePtr()->getAs<RecordType>();</div></div><div><br></div><div>QualType has an overloaded operator->, so you can just say "ToPointeeTy->getAs<RecordType>()".</div><div><br></div><div><br></div><div><div>+ FieldDecl *last = 0;</div></div><div><br></div><div>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.)</div><div><br></div><div><br></div><div><div>+ for (; iter != end; ++iter)</div><div>+ last = *iter;</div></div><div><br></div><div>What if the struct is empty?</div><div><br></div><div><div><br></div><div>+ const Type *elemType = last->getType()->getArrayElementTypeNoTypeQual();</div></div><div><br></div><div>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.</div><div><br></div><div><br></div><div><div>+ if (const ConstantArrayType *ArrayTy =</div></div><div><div>+ Ctx.getAsConstantArrayType(last->getType())) {</div></div><div><br></div><div>I don't think there's a rule about this, but I think a wrapped initialization or assignment should have the next line indented. "Whatever clang-format does" is also almost always an acceptable answer.</div><div><br></div><div><br></div><div><div>+ if (ArrayTy->getSize() != 0)</div></div><div><br></div><div>People sometimes write this pattern with a <a href="http://blogs.msdn.com/b/oldnewthing/archive/2004/08/26/220873.aspx">last element of 1</a> instead of 0. Since the analyzer tries to avoid false positives, we should probably detect that as a similar pattern and perform the same check.</div><div><br></div><div><br></div><div><div>+ BT.reset(new BuiltinBug("Cast region with wrong size.",</div><div>+ "Cast a region whose size is not a multiple of the"</div><div>+ " destination type size."));</div></div><div><br></div><div>While you're here, please re-align the string literals so all the arguments to BuiltinBug() line up. (Also, after Alex's recent patch you'll need to pass the current checker to BuiltinBug as well.</div><div><br></div><div><br></div><div><div>Index: test/Analysis/malloc-annotations.c</div><div>===================================================================</div><div>--- test/Analysis/malloc-annotations.c<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 201043)</div><div>+++ test/Analysis/malloc-annotations.c<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div></div><div><br></div><div>Don't worry about adding test cases to malloc-annotations.c. That's only supposed to test custom malloc and free wrappers. On the other hand, please include some test cases with flexible-array structs where the allocation size is too small to begin with, and some where the allocation size has zero additional capacity over the sizeof of the struct.</div><div><br></div><div>Thanks for working on this!</div><div>Jordan</div><div><br></div><br><div><div>On Feb 10, 2014, at 11:45 , Daniel Fahlgren <<a href="mailto:daniel@fahlgren.se">daniel@fahlgren.se</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Hi,<br><br>I got a false positive at work and this patch fixes it. CastSizeChecker<br>will now handle structs ending with a flexible array. It will no longer<br>warn when you allocate sizeof the struct + a multiple of the array's<br>element size.<br><br>Best regards,<br>Daniel Fahlgren<br></blockquote></div></body></html>