[cfe-commits] [patch] Added a suggestion when a std::string is passed to printf()

[Sam Panzer]

Here is the next version of this patch. Changes over the last one include
Richard's suggestions for variadicArgumentPODCheck, some cleanups that were
available after this adjustment, and more unification of
Check{Constructor,Function,Block,ObjCMethod}Call(), to make sure that the
warning is issued in a uniform way for various variadic function-like
things.

-Sam

On Sun, Jun 17, 2012 at 4:44 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> Hi Sam,
>
> On Thu, Jun 14, 2012 at 3:09 PM, Sam Panzer <panzer at google.com> wrote:
> [...]
> >> >> -          TrivialEnough = true;
> >> >> -        }
> >> >> -      }
> >> >> -    }
> >> >> -
> >> >> -    if (!TrivialEnough &&
> >> >> -        getLangOpts().ObjCAutoRefCount &&
> >> >> -        E->getType()->isObjCLifetimeType())
> >> >> -      TrivialEnough = true;
> >> >> -
> >> >> -    if (TrivialEnough) {
> >> >> -      // Nothing to diagnose. This is okay.
> >> >> +    break;
> >> >> +  case PK_NON_POD:
> >> >> +    if (HasFormatString) {
> >> >> +      // If there is a format string argument, non-POD argument
> >> >> checking
> >> >> is
> >> >> +      // handled along with format string checking.
> >> >>
> >> >> Does this do the right thing for arguments which aren't format
> >> >> arguments,
> >> >> for instance sprintf(my_cstr1, "%d", 1); ? Do we reach the check in
> the
> >> >> -Wformat code if the format string is not a constant?
> >> >
> >> >
> >> > Yes, the sprintf() case is handled correctly - that's what
> >> > getFormatStringInfo() is responsible for. The -Wformat check wasn't
> >> > reached
> >> > if the format string wasn't const-qualified, though. This is fixed in
> >> > the
> >> > new patch.
> >> > I added test cases for these two situations to printf-cstr.cpp.
> >>
> >> I would imagine you still won't get a diagnostic about the non-pod
> >> argument in a case like this:
> >>
> >> extern const char str[30];
> >> printf(str, my_std_string);
> >>
> >> I think you need essentially the same set of checks which are
> >> performed by SemaCheckStringLiteral in order to get this exactly
> >> right. Perhaps the easiest approach would be to defer all POD checking
> >> of vararg types to CheckFunctionArguments (after the call to
> >> SemaCheckStringLiteral) if the callee has a FormatAttr.
> >
> >
> > I factored out the checks from DefaultVariadicArgumentPromotion so that
> they
> > are reused right after SemaCheckStringLiteral in CheckFormatArguments as
> > Richeard suggested. Now any format string arguments that the old warning
> > would have caught but the format string matching misses should now be
> > caught, and there's a few extra test to catch this sort of thing.
>
> Great, thanks. I think this is nearly ready now. One more functional
> change: in the VAK_Invalid case, the argument is transformed from E to
> (__builtin_trap(), E), but we lose this handling when the function
> call has a format string. I think you should split up
> variadicArgumentPODCheck slightly differently, and keep the code to
> modify the expression in DefaultVariadicArgumentPromotion. The
> ObjCObjectType check and RequireCompleteType should be moved outside
> variadicArgumentPODCheck too; we want those errors to be emitted
> regardless of what might happen with the format string.
>
> With that rearrangement done (so variadincArgumentPODCheck just
> produces the cannot_pass_non_pod_to_vararg warnings), I think it would
> be straightforward to move the code which generates the non-POD
> argument diagnostic into SemaChecking's CheckFunctionCall, and return
> a flag from CheckFormatArguments to indicate if we checked a format
> string so the diagnostics can be suppressed, rather than calling into
> the diagnostics code from two different places.
>
> There are also a few sets of redundant braces in this patch, which are
> generally avoided in Clang code.
>
> Thanks!
> Richard
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120620/66eb8ff0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: printf-diag.patch
Type: application/octet-stream
Size: 52554 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120620/66eb8ff0/attachment.obj>