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

[Richard Smith]

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