[cfe-commits] r146326 - in /cfe/trunk: include/clang/Analysis/Analyses/FormatString.h lib/Analysis/ScanfFormatString.cpp lib/Sema/SemaChecking.cpp test/Analysis/taint-generic.c test/Analysis/taint-tester.c test/Sema/format-strings-fixit.c test/Se

[Ted Kremenek]

Further motivating putting this checking in hasValidLengthModifier(), we should issue a diagnostic when this length modifier does not apply, not just abort parsing.

On Dec 13, 2011, at 6:16 AM, Ted Kremenek <kremenek at apple.com> wrote:

> Hi Hans,
> 
> Thanks for looking at this.  I'm concerned about this piece of the patch:
> 
>> --- a/lib/Analysis/FormatString.cpp
>> +++ b/lib/Analysis/FormatString.cpp
>> @@ -196,6 +196,13 @@ clang::analyze_format_string::ParseLengthModifier(FormatSpecifier &FS,
>>      case 't': lmKind = LengthModifier::AsPtrDiff;    ++I; break;
>>      case 'L': lmKind = LengthModifier::AsLongDouble; ++I; break;
>>      case 'q': lmKind = LengthModifier::AsLongLong;   ++I; break;
>> +    case 'a':
>> +      ++I;
>> +      if (I != E && (*I == 's' || *I == 'S' || *I == '[')) {
>> +        lmKind = LengthModifier::AsAllocate; break;
>> +      }
>> +      --I;
>> +      return false;
>>    }
> 
> I think this kind of checking should go in the semantic phase, e.g. hasValidLengthModifier(), not in parsing.  We should not need to do lookahead to perform this semantic checking, and breaks the separation of concerns between the different parsing methods and ParseScanfSpecifier().  Was there a particular reason you put it here?
> 
> Cheers,
> Ted
> 
> On Dec 13, 2011, at 4:13 AM, Hans Wennborg wrote:
> 
>> On Tue, Dec 13, 2011 at 6:47 AM, Ted Kremenek <kremenek at apple.com> wrote:
>>> It is also possible that those tests need to be modified if Clang's behavior
>>> ends up being more desirable than what is reflected in those tests.  I
>>> haven't looked at them, so I don't know off hand.
>> 
>> The failing tests use the 'a' length modifier, a GNU extension
>> available for C90 which can be used with strings and scanlists, that I
>> didn't know about before.
>> 
>> Clang would mis-parse this as the 'a' conversion specifier, and after
>> my commit it would warn, incorrectly, that it expected a float*
>> argument.
>> 
>> The attached patch makes Clang parse this extension correctly, which
>> should fix the test. We can revisit this later to teach it what type
>> to expect, warn about it being an extension, not available in C99,
>> etc. There's also a similar 'm' modifier which me may wish to support.
>> 
>> Thanks,
>> Hans
>> <scanf-alloc-modifier.diff>
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111213/9766a8db/attachment.html>