Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value

[erik.pilkington]

Summary

This patch adds a new attribute called format_dynamic_key_arg. It's intended to be used instead of format_arg for methods/functions that load the formatting string from some external source based on the given key value. The attribute tells Clang that it has to avoid -Wformat warnings when key strings have no formatting specifiers. Otherwise it tells Clang that it can assume that the key strings use the same formatting layout as the external formatting string values (i.e. Clang can perform the normal -Wformat related checks).

This attribute is useful for certain Objective-C methods like NSBundle.localizedStringForKey that load the values dynamically based on a specified key value.

[arphaman]

Ping

[aaron.ballman]

include/clang/Basic/Attr.td

862

Does GCC support this attribute as well? If not, this should use the GNU spelling instead of the GCC one. Also, should this have a C++11 spelling in the clang namespace?

The name doesn't really convey much about what the attribute is doing, mostly because it doesn't seem obvious what "key" means.

It seems that the crux of what this attribute says is that the attributed function accepts a string argument of a format specifier and returns the same format specifier. Perhaps returns_format_specifier is a better name?

lib/Sema/SemaDeclAttr.cpp

5632

We ask that all of these handle*Attr() functions have the same signature, so adding extra parameters isn't something we do. At some point, we want to generate more boilerplate code from the attribute definition files, and so having different parameter lists makes that much harder to accomplish. A better approach would be to make a function called handleFormatDynamicKeyArg() that calls a helper function with the extra argument, and modify handleFormatArgAttr() to call that helper function as well.

[arphaman]

The updated patch renames the attribute to loads_format_specifier_value_using_key and addresses Aaron's comments

include/clang/Basic/Attr.td

862

Does GCC support this attribute as well? If not, this should use the GNU spelling instead of the GCC one.

No, it doesn't. Thanks for pointing that out, I fixed it.

Also, should this have a C++11 spelling in the clang namespace?

I don't know, is that required for new attributes? I don't need the C++11 spelling personally, and I don't know if there is anyone else who's interested in this attribute.

The name doesn't really convey much about what the attribute is doing, mostly because it doesn't seem obvious what "key" means.

It seems that the crux of what this attribute says is that the attributed function accepts a string argument of a format specifier and returns the same format specifier. Perhaps returns_format_specifier is a better name?

You're right, the name should convey the intended meaning better. I switched to loads_format_specifier_value_using_key, how does that sound?
I think it sounds better than just returns_format_specifier because I would like to see this attribute used only for a particular kind of function. This kind of function should load the value at runtime using the key from a platform-specific file/database that might also be accessible at compile-time. It shouldn't be used for functions that might just modify the given key, which, IMO, returns_format_specifier can imply.

[arphaman]

Update to fix a test failure.

[aaron.ballman]

include/clang/Basic/Attr.td

862

Also, should this have a C++11 spelling in the clang namespace?

I don't know, is that required for new attributes? I don't need the C++11 spelling personally, and I don't know if there is anyone else who's interested in this attribute.

It's not required, but not everyone likes GNU-style attributes, so having something a bit less awful is a kindness.

The name doesn't really convey much about what the attribute is doing, mostly because it doesn't seem obvious what "key" means.
It seems that the crux of what this attribute says is that the attributed function accepts a string argument of a format specifier and returns the same format specifier. Perhaps returns_format_specifier is a better name?

You're right, the name should convey the intended meaning better. I switched to loads_format_specifier_value_using_key, how does that sound?
I think it sounds better than just returns_format_specifier because I would like to see this attribute used only for a particular kind of function. This kind of function should load the value at runtime using the key from a platform-specific file/database that might also be accessible at compile-time. It shouldn't be used for functions that might just modify the given key, which, IMO, returns_format_specifier can imply.

It's a bit wordy, but I think it's better than the original name.

include/clang/Basic/AttrDocs.td

1759

value -> specifier (same below).

1766

Given that this is uses novel terminology like "key string", I think an example would be useful to add.

The docs should also mention that this attribute accepts an argument, and that the argument is 1-based instead of 0-based (I can't count how many times that's tripped me up).

lib/Sema/SemaChecking.cpp

4405

I think the original formatting was easier to read.

4415–4421

Same here.

4590

Can use const auto * here.

6297

The original formatting was better.

test/Sema/attr-format_arg.c

29

You should also add tests for:

it diagnoses if the attribute is added to something other than a function/method.
it diagnoses if the attribute has no arguments/arguments of the wrong type.
it works properly on a class member function in C++.

[erik.pilkington]

Stealing this (with Alex's permission). I think it makes more sense to add a special case for the method -[NSBundle localizedStringForKey:value:table:] rather then add a new general purpose attribute that'll only be used in one place.

[erik.pilkington]

Special case localizedStringForKey instead of using an attribute.

[rjmccall]

I agree with just special-casing this for now.

clang/lib/Sema/SemaChecking.cpp

6800 ↗

(On Diff #214925)

It's more efficient to do these checks with isStr instead of looking up an IdentifierInfo. You can add a Selector::isKeywordSelector(ArrayRef<StringRef>) method to make that equally convenient for selectors.

[erik.pilkington]

Address review comments.

[rjmccall]

One minor request (which will be dead code in your patch), but otherwise LGTM.

clang/include/clang/Basic/IdentifierTable.h

754 ↗

(On Diff #214977)

Thanks. Could you go ahead and add isUnarySelector(StringRef) for consistency?

[aaron.ballman]

I like this approach much better! LGTM, aside from a minor nit.

clang/include/clang/Basic/IdentifierTable.h

754 ↗

(On Diff #214977)

Also, the function should be marked const, I would imagine.

include/clang/Analysis/Analyses/FormatString.h

 
 bool ParsePrintfString(FormatStringHandler &H,
                        const char *beg, const char *end, const LangOptions &LO,
                        const TargetInfo &Target, bool isFreeBSDKPrintf);
 
 bool ParseFormatStringHasSArg(const char *beg, const char *end,
                               const LangOptions &LO, const TargetInfo &Target);
 
+/// Return true if the given string has at least one formatting specifier.
+bool parseFormatStringHasFormattingSpecifiers(const char *Begin,
+                                              const char *End,
+                                              const LangOptions &LO,
+                                              const TargetInfo &Target);
+
 bool ParseScanfString(FormatStringHandler &H,
                       const char *beg, const char *end, const LangOptions &LO,
                       const TargetInfo &Target);
 
 } // end analyze_format_string namespace
 } // end clang namespace
 #endif

include/clang/Basic/Attr.td

 def FormatArg : InheritableAttr {
   let Spellings = [GCC<"format_arg">];
   let Args = [IntArgument<"FormatIdx">];
   let Subjects = SubjectList<[ObjCMethod, HasFunctionProto], WarnDiag,
                              "ExpectedFunctionWithProtoType">;
   let Documentation = [Undocumented];
 }
 
+def FormatDynamicKeyArg : InheritableAttr {
+  let Spellings = [GCC<"format_dynamic_key_arg">];
+  let Args = [IntArgument<"FormatIdx">];
+  let Subjects = SubjectList<[ObjCMethod, HasFunctionProto], WarnDiag,
+                             "ExpectedFunctionWithProtoType">;
+  let Documentation = [FormatDynamicKeyArgDocs];
+}
+
 def GNUInline : InheritableAttr {
   let Spellings = [GCC<"gnu_inline">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
 }
 
 def Hot : InheritableAttr {
   let Spellings = [GCC<"hot">];

include/clang/Basic/AttrDocs.td

.. code-block:: c
      }
 
    In this case Clang does not warn because the format string ``s`` and
    the corresponding arguments are annotated.  If the arguments are
    incorrect, the caller of ``foo`` will receive a warning.
   }];
 }
 
+def FormatDynamicKeyArgDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The ``format_dynamic_key_arg`` attribute indicates that a function accepts a
+key string that corresponds to some external ``printf`` or ``scanf``-like format
+string and returns that format string.
+
+If a key string has formatting specifiers, Clang assumes that its formatting
+layout corresponds to the formatting layout that's used in the external string
+value, and performs ``-Wformat`` warning related checks using the formatting
+specifiers found in the key string.
+  }];
+}
+
 def AlignValueDocs : Documentation {
   let Category = DocCatType;
   let Content = [{
 The align_value attribute can be added to the typedef of a pointer type or the
 declaration of a variable of pointer or reference type. It specifies that the
 pointer will point to, or the reference will bind to, only objects with at
 least the provided alignment. This alignment value must be some positive power
 of 2.

lib/Analysis/PrintfFormatString.cpp

while (I != E) {
     const analyze_printf::PrintfSpecifier &FS = FSR.getValue();
     // Return true if this a %s format specifier.
     if (FS.getConversionSpecifier().getKind() == ConversionSpecifier::Kind::sArg)
       return true;
   }
   return false;
 }
 
+bool clang::analyze_format_string::parseFormatStringHasFormattingSpecifiers(
+    const char *Begin, const char *End, const LangOptions &LO,
+    const TargetInfo &Target) {
+  unsigned ArgIndex = 0;
+  // Keep looking for a formatting specifier until we have exhausted the string.
+  FormatStringHandler H;
+  while (Begin != End) {
+    const PrintfSpecifierResult &FSR =
+        ParsePrintfSpecifier(H, Begin, End, ArgIndex, LO, Target, false, false);
+    if (FSR.shouldStop())
+      break;
+    if (FSR.hasValue())
+      return true;
+  }
+  return false;
+}
+
 //===----------------------------------------------------------------------===//
 // Methods on PrintfSpecifier.
 //===----------------------------------------------------------------------===//
 
 ArgType PrintfSpecifier::getArgType(ASTContext &Ctx,
                                     bool IsObjCLiteral) const {
   const PrintfConversionSpecifier &CS = getConversionSpecifier();
 

lib/Sema/SemaChecking.cpp

public:
 
   SourceLocation getLocStart() const LLVM_READONLY {
     return FExpr->getLocStart().getLocWithOffset(Offset);
   }
   SourceLocation getLocEnd() const LLVM_READONLY { return FExpr->getLocEnd(); }
 };
 }  // end anonymous namespace
 
-static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr,
-                              const Expr *OrigFormatExpr,
-                              ArrayRef<const Expr *> Args,
-                              bool HasVAListArg, unsigned format_idx,
-                              unsigned firstDataArg,
-                              Sema::FormatStringType Type,
-                              bool inFunctionCall,
-                              Sema::VariadicCallType CallType,
-                              llvm::SmallBitVector &CheckedVarArgs,
-                              UncoveredArgHandler &UncoveredArg);
+static void CheckFormatString(
+    Sema &S, const FormatStringLiteral *FExpr, const Expr *OrigFormatExpr,
+    ArrayRef<const Expr *> Args, bool HasVAListArg, unsigned format_idx,
+    unsigned firstDataArg, Sema::FormatStringType Type, bool inFunctionCall,
+    Sema::VariadicCallType CallType, llvm::SmallBitVector &CheckedVarArgs,
+    UncoveredArgHandler &UncoveredArg, bool IgnoreStringsWithoutSpecifiers);
 
 // Determine if an expression is a string literal or constant string.
 // If this function returns false on the arguments to a function expecting a
 // format string, we will usually need to emit a warning.
 // True string literals are then checked by CheckFormatString.
-static StringLiteralCheckType
-checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
-                      bool HasVAListArg, unsigned format_idx,
-                      unsigned firstDataArg, Sema::FormatStringType Type,
-                      Sema::VariadicCallType CallType, bool InFunctionCall,
-                      llvm::SmallBitVector &CheckedVarArgs,
-                      UncoveredArgHandler &UncoveredArg,
-                      llvm::APSInt Offset) {
- tryAgain:
+static StringLiteralCheckType checkFormatStringExpr(
+    Sema &S, const Expr *E, ArrayRef<const Expr *> Args, bool HasVAListArg,
+    unsigned format_idx, unsigned firstDataArg, Sema::FormatStringType Type,
+    llvm::SmallBitVector &CheckedVarArgs, UncoveredArgHandler &UncoveredArg,
+    llvm::APSInt Offset, bool IgnoreStringsWithoutSpecifiers = false) {
   assert(Offset.isSigned() && "invalid offset");
 
   if (E->isTypeDependent() || E->isValueDependent())
     return SLCT_NotALiteral;
 
   E = E->IgnoreParenCasts();
 
   if (E->isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull))
case Stmt::ConditionalOperatorClass: {
     // We need to maintain the offsets for the right and the left hand side
     // separately to check if every possible indexed expression is a valid
     // string literal. They might have different offsets for different string
     // literals in the end.
     StringLiteralCheckType Left;
     if (!CheckLeft)
       Left = SLCT_UncheckedLiteral;
     else {
-      Left = checkFormatStringExpr(S, C->getTrueExpr(), Args,
-                                   HasVAListArg, format_idx, firstDataArg,
-                                   Type, CallType, InFunctionCall,
-                                   CheckedVarArgs, UncoveredArg, Offset);
+      Left = checkFormatStringExpr(S, C->getTrueExpr(), Args, HasVAListArg,
+                                   format_idx, firstDataArg, Type, CallType,
+                                   InFunctionCall, CheckedVarArgs, UncoveredArg,
+                                   Offset, IgnoreStringsWithoutSpecifiers);
       if (Left == SLCT_NotALiteral || !CheckRight) {
         return Left;
       }
     }
 
-    StringLiteralCheckType Right =
-        checkFormatStringExpr(S, C->getFalseExpr(), Args,
-                              HasVAListArg, format_idx, firstDataArg,
-                              Type, CallType, InFunctionCall, CheckedVarArgs,
-                              UncoveredArg, Offset);
+    StringLiteralCheckType Right = checkFormatStringExpr(
+        S, C->getFalseExpr(), Args, HasVAListArg, format_idx, firstDataArg,
+        Type, CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset,
+        IgnoreStringsWithoutSpecifiers);
 
     return (CheckLeft && Left < Right) ? Left : Right;
   }
 
   case Stmt::ImplicitCastExprClass: {
     E = cast<ImplicitCastExpr>(E)->getSubExpr();
     goto tryAgain;
   }
if (const VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl())) {
 
       if (isConstant) {
         if (const Expr *Init = VD->getAnyInitializer()) {
           // Look through initializers like const char c[] = { "foo" }
           if (const InitListExpr *InitList = dyn_cast<InitListExpr>(Init)) {
             if (InitList->isStringLiteralInit())
               Init = InitList->getInit(0)->IgnoreParenImpCasts();
           }
-          return checkFormatStringExpr(S, Init, Args,
-                                       HasVAListArg, format_idx,
+          return checkFormatStringExpr(S, Init, Args, HasVAListArg, format_idx,
                                        firstDataArg, Type, CallType,
                                        /*InFunctionCall*/ false, CheckedVarArgs,
-                                       UncoveredArg, Offset);
+                                       UncoveredArg, Offset,
+                                       IgnoreStringsWithoutSpecifiers);
         }
       }
 
       // For vprintf* functions (i.e., HasVAListArg==true), we add a
       // special check to see if the format string is a function parameter
       // of the function calling the printf function.  If the function
       // has an attribute indicating it is a printf-like function, then we
       // should suppress warnings concerning non-literals being used in a call
case Stmt::CXXMemberCallExprClass: {
     if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl())) {
       if (const FormatArgAttr *FA = ND->getAttr<FormatArgAttr>()) {
         unsigned ArgIndex = FA->getFormatIdx();
         if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(ND))
           if (MD->isInstance())
             --ArgIndex;
         const Expr *Arg = CE->getArg(ArgIndex - 1);
 
-        return checkFormatStringExpr(S, Arg, Args,
-                                     HasVAListArg, format_idx, firstDataArg,
-                                     Type, CallType, InFunctionCall,
-                                     CheckedVarArgs, UncoveredArg, Offset);
+        return checkFormatStringExpr(
+            S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
+            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset,
+            IgnoreStringsWithoutSpecifiers);
+      } else if (const auto *FKA = ND->getAttr<FormatDynamicKeyArgAttr>()) {
+        unsigned ArgIndex = FKA->getFormatIdx();
+        if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(ND))
+          if (MD->isInstance())
+            --ArgIndex;
+        const Expr *Arg = CE->getArg(ArgIndex - 1);
+        return checkFormatStringExpr(
+            S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
+            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset,
+            /*IgnoreStringsWithoutSpecifiers=*/true);
       } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
         unsigned BuiltinID = FD->getBuiltinID();
         if (BuiltinID == Builtin::BI__builtin___CFStringMakeConstantString ||
             BuiltinID == Builtin::BI__builtin___NSStringMakeConstantString) {
           const Expr *Arg = CE->getArg(0);
-          return checkFormatStringExpr(S, Arg, Args,
-                                       HasVAListArg, format_idx,
-                                       firstDataArg, Type, CallType,
-                                       InFunctionCall, CheckedVarArgs,
-                                       UncoveredArg, Offset);
+          return checkFormatStringExpr(
+              S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
+              CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset,
+              IgnoreStringsWithoutSpecifiers);
         }
       }
     }
 
     return SLCT_NotALiteral;
   }
   case Stmt::ObjCMessageExprClass: {
     const auto *ME = cast<ObjCMessageExpr>(E);
     if (const auto *ND = ME->getMethodDecl()) {
       if (const auto *FA = ND->getAttr<FormatArgAttr>()) {
         unsigned ArgIndex = FA->getFormatIdx();
         const Expr *Arg = ME->getArg(ArgIndex - 1);
         return checkFormatStringExpr(
             S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
-            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset);
+            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset,
+            IgnoreStringsWithoutSpecifiers);
+      } else if (const auto *FKA = ND->getAttr<FormatDynamicKeyArgAttr>()) {
+        unsigned ArgIndex = FKA->getFormatIdx();
+        const Expr *Arg = ME->getArg(ArgIndex - 1);
+        return checkFormatStringExpr(
+            S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
+            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset,
+            /*IgnoreStringsWithoutSpecifiers=*/true);
       }
     }
 
     return SLCT_NotALiteral;
   }
   case Stmt::ObjCStringLiteralClass:
   case Stmt::StringLiteralClass: {
     const StringLiteral *StrE = nullptr;
 
     if (const ObjCStringLiteral *ObjCFExpr = dyn_cast<ObjCStringLiteral>(E))
       StrE = ObjCFExpr->getString();
     else
       StrE = cast<StringLiteral>(E);
 
     if (StrE) {
       if (Offset.isNegative() || Offset > StrE->getLength()) {
         // TODO: It would be better to have an explicit warning for out of
         // bounds literals.
         return SLCT_NotALiteral;
       }
       FormatStringLiteral FStr(StrE, Offset.sextOrTrunc(64).getSExtValue());
       CheckFormatString(S, &FStr, E, Args, HasVAListArg, format_idx,
                         firstDataArg, Type, InFunctionCall, CallType,
-                        CheckedVarArgs, UncoveredArg);
+                        CheckedVarArgs, UncoveredArg,
+                        IgnoreStringsWithoutSpecifiers);
       return SLCT_CheckedLiteral;
     }
 
     return SLCT_NotALiteral;
   }
   case Stmt::BinaryOperatorClass: {
     llvm::APSInt LResult;
     llvm::APSInt RResult;
EmitFormatDiagnostic(S.PDiag(diag)
                          Ex->getLocStart(),
                          /*IsStringLocation*/ false,
                          getSpecifierRange(startSpecifier, specifierLen));
   }
 
   return true;
 }
 
-static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr,
-                              const Expr *OrigFormatExpr,
-                              ArrayRef<const Expr *> Args,
-                              bool HasVAListArg, unsigned format_idx,
-                              unsigned firstDataArg,
-                              Sema::FormatStringType Type,
-                              bool inFunctionCall,
-                              Sema::VariadicCallType CallType,
-                              llvm::SmallBitVector &CheckedVarArgs,
-                              UncoveredArgHandler &UncoveredArg) {
-  // CHECK: is the format string a wide literal?
-  if (!FExpr->isAscii() && !FExpr->isUTF8()) {
-    CheckFormatHandler::EmitFormatDiagnostic(
-      S, inFunctionCall, Args[format_idx],
-      S.PDiag(diag::warn_format_string_is_wide_literal), FExpr->getLocStart(),
-      /*IsStringLocation*/true, OrigFormatExpr->getSourceRange());
-    return;
-  }
-
+static void CheckFormatString(
+    Sema &S, const FormatStringLiteral *FExpr, const Expr *OrigFormatExpr,
+    ArrayRef<const Expr *> Args, bool HasVAListArg, unsigned format_idx,
+    unsigned firstDataArg, Sema::FormatStringType Type, bool inFunctionCall,
+    Sema::VariadicCallType CallType, llvm::SmallBitVector &CheckedVarArgs,
+    UncoveredArgHandler &UncoveredArg, bool IgnoreStringsWithoutSpecifiers) {
   // Str - The format string.  NOTE: this is NOT null-terminated!
   StringRef StrRef = FExpr->getString();
   const char *Str = StrRef.data();
   // Account for cases where the string literal is truncated in a declaration.
   const ConstantArrayType *T =
     S.Context.getAsConstantArrayType(FExpr->getType());
   assert(T && "String literal not of constant array type!");
   size_t TypeSize = T->getSize().getZExtValue();
   size_t StrLen = std::min(std::max(TypeSize, size_t(1)) - 1, StrRef.size());
   const unsigned numDataArgs = Args.size() - firstDataArg;
 
+  if (IgnoreStringsWithoutSpecifiers &&
+      !analyze_format_string::parseFormatStringHasFormattingSpecifiers(
+          Str, Str + StrLen, S.getLangOpts(), S.Context.getTargetInfo()))
+    return;
+
+  // CHECK: is the format string a wide literal?
+  if (!FExpr->isAscii() && !FExpr->isUTF8()) {
+    CheckFormatHandler::EmitFormatDiagnostic(
+        S, inFunctionCall, Args[format_idx],
+        S.PDiag(diag::warn_format_string_is_wide_literal), FExpr->getLocStart(),
+        /*IsStringLocation*/ true, OrigFormatExpr->getSourceRange());
+    return;
+  }
+
   // Emit a warning if the string literal is truncated and does not contain an
   // embedded null character.
   if (TypeSize <= StrRef.size() &&
       StrRef.substr(0, TypeSize).find('\0') == StringRef::npos) {
     CheckFormatHandler::EmitFormatDiagnostic(
         S, inFunctionCall, Args[format_idx],
         S.PDiag(diag::warn_printf_format_string_not_null_terminated),
         FExpr->getLocStart(),

lib/Sema/SemaDeclAttr.cpp

static void handleCleanupAttr(Sema &S, Decl *D, const AttributeList &Attr) {
 
   D->addAttr(::new (S.Context)
              CleanupAttr(Attr.getRange(), S.Context, FD,
                          Attr.getAttributeSpellingListIndex()));
 }
 
 /// Handle __attribute__((format_arg((idx)))) attribute based on
 /// http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
-static void handleFormatArgAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+static void handleFormatArgAttr(Sema &S, Decl *D, const AttributeList &Attr,
+                                bool IsDynamicKey = false) {
   Expr *IdxExpr = Attr.getArgAsExpr(0);
   uint64_t Idx;
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, IdxExpr, Idx))
     return;
 
   // Make sure the format string is really a string.
   QualType Ty = getFunctionOrMethodParamType(D, Idx);
 
static void handleFormatArgAttr(Sema &S, Decl *D, const AttributeList &Attr,
   }
 
   // We cannot use the Idx returned from checkFunctionOrMethodParameterIndex
   // because that has corrected for the implicit this parameter, and is zero-
   // based.  The attribute expects what the user wrote explicitly.
   llvm::APSInt Val;
   IdxExpr->EvaluateAsInt(Val, S.Context);
 
+  if (IsDynamicKey) {
+    D->addAttr(::new (S.Context) FormatDynamicKeyArgAttr(
+        Attr.getRange(), S.Context, Val.getZExtValue(),
+        Attr.getAttributeSpellingListIndex()));
+    return;
+  }
   D->addAttr(::new (S.Context)
              FormatArgAttr(Attr.getRange(), S.Context, Val.getZExtValue(),
                            Attr.getAttributeSpellingListIndex()));
 }
 
 enum FormatAttrKind {
   CFStringFormat,
   NSStringFormat,
case AttributeList::AT_Flatten:
     handleSimpleAttribute<FlattenAttr>(S, D, Attr);
     break;
   case AttributeList::AT_Format:
     handleFormatAttr(S, D, Attr);
     break;
   case AttributeList::AT_FormatArg:
     handleFormatArgAttr(S, D, Attr);
     break;
+  case AttributeList::AT_FormatDynamicKeyArg:
+    handleFormatArgAttr(S, D, Attr, /*IsDynamicKey=*/true);
+    break;
   case AttributeList::AT_CUDAGlobal:
     handleGlobalAttr(S, D, Attr);
     break;
   case AttributeList::AT_CUDADevice:
     handleSimpleAttributeWithExclusions<CUDADeviceAttr, CUDAGlobalAttr>(S, D,
                                                                         Attr);
     break;
   case AttributeList::AT_CUDAHost:

test/Sema/attr-format_arg.c

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 int printf(const char *, ...);
 
 const char* f(const char *s) __attribute__((format_arg(1)));
 
 void g(const char *s) {
   printf("%d", 123);
   printf("%d %d", 123); // expected-warning{{more '%' conversions than data arguments}}
 
   printf(f("%d"), 123);
   printf(f("%d %d"), 123); // expected-warning{{more '%' conversions than data arguments}}
 }
+
+const char *loadFormattingValue(const char *key) __attribute__((format_dynamic_key_arg(1)));
+
+void testFormatDynamicKeyArg() {
+  // Don't warn when the key has no formatting specifiers
+  printf(loadFormattingValue("key"), "foo");
+
+  // Warn normally when the key has formatting specifiers
+  printf(loadFormattingValue("%d"), 123);
+  printf(loadFormattingValue("%d %d"), 123); // expected-warning{{more '%' conversions than data arguments}}
+}
+
+const char *formatKeyArgError1(const char *format)
+  __attribute__((format_dynamic_key_arg("foo")));  // expected-error{{'format_dynamic_key_arg' attribute requires parameter 1 to be an integer constant}}
+const char *formatKeyArgError2(const char *format)
+  __attribute__((format_dynamic_key_arg(0)));  // expected-error{{'format_dynamic_key_arg' attribute parameter 1 is out of bounds}}

test/SemaObjC/format-strings-objc.m

- (void)foo {
   [RD23622446_Tester stringWithFormat:[self test: "%s %s"], "name"]; // expected-warning {{more '%' conversions than data arguments}}
   NSLog([self str: @"%@ %@"], @"name"); // expected-warning {{more '%' conversions than data arguments}}
   [RD23622446_Tester stringWithFormat:rd23622446("%d"), 1]; // ok
   [RD23622446_Tester stringWithFormat:[self test: "%d %d"], 1, 2]; // ok
   NSLog([self str: @"%@"], @"string"); // ok
 }
 
 @end
+
+@interface FormatDynamicKeyArg
+
+- (NSString *)load:(NSString *)key __attribute__((format_dynamic_key_arg(1)));
+
+@end
+
+void testFormatDynamicKeyArg(FormatDynamicKeyArg *m) {
+  // Don't warn when the key has no formatting specifiers
+  NSLog([m load:@"key"], @"foo");
+  NSLog([m load:@""]);
+
+  // Warn normally when the key has formatting specifiers
+  NSLog([m load:@"correct %d"], 2);
+  NSLog([m load:@"warn %d"], "off"); // expected-warning {{format specifies type 'int' but the argument has type 'char *'}}
+  NSLog([m load:@"%@ %@"], @"name"); // expected-warning {{more '%' conversions than data arguments}}
+  NSLog([m load:@"Warn again %@"], @"name", @"void"); // expected-warning {{data argument not used by format string}}
+}