[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

[Hans Wennborg]

On Tue, Dec 13, 2011 at 3:04 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Further motivating putting this checking in hasValidLengthModifier(), we
> should issue a diagnostic when this length modifier does not apply, not just
> abort parsing.

Hi Ted,

The trouble is that 'a' can mean two things in a scanf format string.
If it is before 's', 'S' or '[', it is a length modifier (as a C90 GNU
extensions), otherwise it is a conversion specifier (same as 'f').

The lookahead is not to do a semantic check, it is to decide whether
to parse the 'a' as a length modifier, or not.

Thanks,
Hans

>
> 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