[cfe-commits] r149268 - in /cfe/trunk: lib/Sema/SemaChecking.cpp test/SemaObjC/format-strings-objc.m

[Jean-Daniel Dupas]

Le 31 janv. 2012 à 01:36, Eli Friedman a écrit :

> On Mon, Jan 30, 2012 at 4:02 PM, Jean-Daniel Dupas
> <devlists at shadowlab.org> wrote:
>> 
>> Le 30 janv. 2012 à 22:10, Eli Friedman a écrit :
>> 
>> 
>> On Mon, Jan 30, 2012 at 11:46 AM, Jean-Daniel Dupas
>> <devlists at shadowlab.org> wrote:
>> 
>> Author: jddupas
>> 
>> Date: Mon Jan 30 13:46:17 2012
>> 
>> New Revision: 149268
>> 
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=149268&view=rev
>> 
>> Log:
>> 
>> Disable "non literal format string" for NSString that result from a macro
>> expansion.
>> 
>> This is to prevent diagnostic when using NSLocalizedString or
>> CFCopyLocalizedString
>> 
>> macros which are usually used in place of NS and CF strings literals.
>> 
>> 
>> 
>> Modified:
>> 
>>    cfe/trunk/lib/Sema/SemaChecking.cpp
>> 
>>    cfe/trunk/test/SemaObjC/format-strings-objc.m
>> 
>> 
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> 
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=149268&r1=149267&r2=149268&view=diff
>> 
>> ==============================================================================
>> 
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> 
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Mon Jan 30 13:46:17 2012
>> 
>> @@ -1586,6 +1586,13 @@
>> 
>>                              format_idx, firstDataArg, Type))
>> 
>>     return;  // Literal format string found, check done!
>> 
>> 
>> +  // Do not emit diag when the string param is a macro expansion and the
>> 
>> +  // format is either NSString or CFString. This is a hack to prevent
>> 
>> +  // diag when using the NSLocalizedString and CFCopyLocalizedString macros
>> 
>> +  // which are usually used in place of NS and CF string literals.
>> 
>> +  if (Type == FST_NSString && Args[format_idx]->getLocStart().isMacroID())
>> 
>> +    return;
>> 
>> +
>> 
>>   // If there are no arguments specified, warn with -Wformat-security,
>> otherwise
>> 
>>   // warn only with -Wformat-nonliteral.
>> 
>>   if (NumArgs == format_idx+1)
>> 
>> 
>> Modified: cfe/trunk/test/SemaObjC/format-strings-objc.m
>> 
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/format-strings-objc.m?rev=149268&r1=149267&r2=149268&view=diff
>> 
>> ==============================================================================
>> 
>> --- cfe/trunk/test/SemaObjC/format-strings-objc.m (original)
>> 
>> +++ cfe/trunk/test/SemaObjC/format-strings-objc.m Mon Jan 30 13:46:17 2012
>> 
>> @@ -1,4 +1,4 @@
>> 
>> -// RUN: %clang_cc1 -fsyntax-only -verify %s
>> 
>> +// RUN: %clang_cc1 -Wformat-nonliteral -fsyntax-only -verify %s
>> 
>> 
>>  //===----------------------------------------------------------------------===//
>> 
>>  // The following code is reduced using delta-debugging from
>> 
>> @@ -107,3 +107,11 @@
>> 
>>   NSString * ns3 = ns1;
>> 
>>   NSLog(ns3); // expected-warning {{format string is not a string literal}}}
>> 
>>  }
>> 
>> +
>> 
>> +// Do not emit warnings when using NSLocalizedString
>> 
>> +extern NSString *GetLocalizedString(NSString *str);
>> 
>> +#define NSLocalizedString(key) GetLocalizedString(key)
>> 
>> +
>> 
>> +void check_NSLocalizedString() {
>> 
>> +  [Foo fooWithFormat:NSLocalizedString(@"format"), @"arg"]; // no-warning
>> 
>> +}
>> 
>> 
>> I'm not sure this is the right approach... see also
>> http://llvm.org/bugs/show_bug.cgi?id=3814 , which is essentially the
>> same issue.
>> 
>> 
>> gettext convention are not the same as Cocoa ones.
>> 
>> From gettext man page:  "By convention, [msgid] is the English version of
>> the message, with non-ASCII characters replaced by ASCII approximations."
>> which means that the gettext macro argument should be a valid format string
>> when used as printf argument.
>> If so, the gettext functions declaration should use the format_arg()
>> attribute which is already handled in clang.
>> 
>> Indeed, using a recent version of gettext (which use format_arg), I get the
>> correct behavior:
>> 
>> ---- fmt.c ---
>> #include <stdio.h>
>> #include <libintl.h>
>> #include <gettext-po.h>
>> 
>> int test(int arg) {
>> return printf(gettext("this is a format string: %s"), arg);
>> }
>> ---------------
>> 
>> fmt.c:7:51: warning: format specifies type 'char *' but the argument has
>> type 'int'
>>         return printf(gettext("this is a format string: %s"), arg);
>>                                                         ~^    ~~~
> 
> Oh, cool; didn't realize that was there.
> 
>> The problem with Cocoa localization macro is that there is not such
>> convention, and it is very common to use a key that is not a version of the
>> localized string at all. So even if they were declared using format_arg
>> (which is not the case), it would not help.
> 
> The issue I'm worried about is that -Wformat-security is supposed to
> warn about any format argument we can't reason about.  The behavior
> with your patch subverts the warning; we don't know that whatever
> random macro is used in the first argument is doing something safe.


I'm not quite happy with this hack too.
If you like, I can revert this patch until we find some cleaner solution.

-- Jean-Daniel