Allow only duplicates, whether on the same decl or redecls

Author: mysterymath

clang/include/clang/Basic/AttrDocs.td

@@ -9661,6 +9661,10 @@ become a call to ``__modular_printf`` with the same arguments, as would
 relocation against the symbol ``__printf_float``, which would bring floating
 point support for ``printf`` into the link.
 
+If the attribute appears more than once on a declaration, or across a chain of
+redeclarations, it is an error for the attributes to have different arguments,
+excepting that the aspects may be in any order.
+
 The following aspects are currently supported:
 
 - ``float``: The call has a floating point argument

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -11270,6 +11270,8 @@ def warn_duplicate_attribute_exact : Warning<
 def warn_duplicate_attribute : Warning<
   "attribute %0 is already applied with different arguments">,
   InGroup<IgnoredAttributes>;
+def err_duplicate_attribute : Error<
+  "attribute %0 is already applied with different arguments">;
 def err_disallowed_duplicate_attribute : Error<
   "attribute %0 cannot appear more than once on a declaration">;
 

clang/include/clang/Sema/Sema.h

@@ -4957,6 +4957,11 @@ class Sema final : public SemaBase {
                                             IdentifierInfo *Format,
                                             int FormatIdx,
                                             StringLiteral *FormatStr);
+  ModularFormatAttr *mergeModularFormatAttr(Decl *D,
+                                            const AttributeCommonInfo &CI,
+                                            IdentifierInfo *ModularImplFn,
+                                            StringRef ImplName,
+                                            MutableArrayRef<StringRef> Aspects);
 
   /// AddAlignedAttr - Adds an aligned attribute to a particular declaration.
   void AddAlignedAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E,

clang/lib/Sema/SemaDecl.cpp

@@ -58,6 +58,7 @@
 #include "clang/Sema/SemaSwift.h"
 #include "clang/Sema/SemaWasm.h"
 #include "clang/Sema/Template.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLForwardCompat.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallPtrSet.h"
@@ -2901,6 +2902,10 @@ static bool mergeDeclAttribute(Sema &S, NamedDecl *D,
   else if (const auto *FMA = dyn_cast<FormatMatchesAttr>(Attr))
     NewAttr = S.mergeFormatMatchesAttr(
         D, *FMA, FMA->getType(), FMA->getFormatIdx(), FMA->getFormatString());
+  else if (const auto *MFA = dyn_cast<ModularFormatAttr>(Attr))
+    NewAttr = S.mergeModularFormatAttr(
+        D, *MFA, MFA->getModularImplFn(), MFA->getImplName(),
+        MutableArrayRef<StringRef>{MFA->aspects_begin(), MFA->aspects_size()});
   else if (const auto *SA = dyn_cast<SectionAttr>(Attr))
     NewAttr = S.mergeSectionAttr(D, *SA, SA->getName());
   else if (const auto *CSA = dyn_cast<CodeSegAttr>(Attr))

clang/lib/Sema/SemaDeclAttr.cpp

@@ -6973,31 +6973,40 @@ static void handleVTablePointerAuthentication(Sema &S, Decl *D,
       CustomDiscriminationValue));
 }
 
-static bool modularFormatIsSame(const ModularFormatAttr *Existing,
-                                IdentifierInfo *ModularImplFn,
-                                StringRef ImplName,
-                                ArrayRef<StringRef> Aspects) {
-  if (Existing->getModularImplFn() != ModularImplFn)
-    return false;
-  if (Existing->getImplName() != ImplName)
-    return false;
-  if (Existing->aspects_size() != Aspects.size())
-    return false;
-  unsigned I = 0;
-  for (const auto &ExistingAspect : Existing->aspects()) {
-    if (ExistingAspect != Aspects[I++])
-      return false;
+static bool modularFormatAttrsEquiv(const ModularFormatAttr *Existing,
+                                    IdentifierInfo *ModularImplFn,
+                                    StringRef ImplName,
+                                    ArrayRef<StringRef> Aspects) {
+  return Existing->getModularImplFn() == ModularImplFn &&
+         Existing->getImplName() == ImplName &&
+         Existing->aspects_size() == Aspects.size() &&
+         llvm::equal(Existing->aspects(), Aspects);
+}
+
+ModularFormatAttr *
+Sema::mergeModularFormatAttr(Decl *D, const AttributeCommonInfo &CI,
+                             IdentifierInfo *ModularImplFn, StringRef ImplName,
+                             MutableArrayRef<StringRef> Aspects) {
+  if (const auto *Existing = D->getAttr<ModularFormatAttr>()) {
+    if (!modularFormatAttrsEquiv(Existing, ModularImplFn, ImplName, Aspects)) {
+      Diag(Existing->getLocation(), diag::err_duplicate_attribute) << *Existing;
+      Diag(CI.getLoc(), diag::note_conflicting_attribute);
+    }
+    // Drop the existing attribute on the declaration in favor of the newly
+    // inherited one.
+    D->dropAttr<ModularFormatAttr>();
   }
-  return true;
+  return ::new (Context) ModularFormatAttr(Context, CI, ModularImplFn, ImplName,
+                                           Aspects.data(), Aspects.size());
 }
 
 static void handleModularFormat(Sema &S, Decl *D, const ParsedAttr &AL) {
+  bool Valid = true;
   StringRef ImplName;
   if (!S.checkStringLiteralArgumentAttr(AL, 1, ImplName))
-    return;
+    Valid = false;
   SmallVector<StringRef> Aspects;
   llvm::DenseSet<StringRef> SeenAspects;
-  bool HasDuplicate = false;
   for (unsigned I = 2, E = AL.getNumArgs(); I != E; ++I) {
     StringRef Aspect;
     if (!S.checkStringLiteralArgumentAttr(AL, I, Aspect))
@@ -7006,27 +7015,26 @@ static void handleModularFormat(Sema &S, Decl *D, const ParsedAttr &AL) {
       S.Diag(AL.getArgAsExpr(I)->getExprLoc(),
              diag::err_modular_format_duplicate_aspect)
           << Aspect;
-      HasDuplicate = true;
+      Valid = false;
       continue;
     }
     Aspects.push_back(Aspect);
   }
+  if (!Valid)
+    return;
 
   // Store aspects sorted.
   llvm::sort(Aspects);
-
   IdentifierInfo *ModularImplFn = AL.getArgAsIdent(0)->getIdentifierInfo();
 
   if (const auto *Existing = D->getAttr<ModularFormatAttr>()) {
-    if (!modularFormatIsSame(Existing, ModularImplFn, ImplName, Aspects)) {
-      S.Diag(AL.getLoc(), diag::warn_duplicate_attribute) << AL;
-      S.Diag(Existing->getLocation(), diag::note_conflicting_attribute);
+    if (!modularFormatAttrsEquiv(Existing, ModularImplFn, ImplName, Aspects)) {
+      S.Diag(AL.getLoc(), diag::err_duplicate_attribute) << *Existing;
+      S.Diag(Existing->getLoc(), diag::note_conflicting_attribute);
     }
-    D->dropAttr<ModularFormatAttr>();
-  }
-
-  if (HasDuplicate)
+    // Ignore the later declaration in favor of the earlier one.
     return;
+  }
 
   D->addAttr(::new (S.Context) ModularFormatAttr(
       S.Context, AL, ModularImplFn, ImplName, Aspects.data(), Aspects.size()));

clang/test/CodeGen/attr-modular-format.c

@@ -15,11 +15,12 @@ void test_explicit_format(void) {
   myprintf("hello");
 }
 
-int redecl(const char *fmt, ...) __attribute__((modular_format(__first_impl, "__first", "one"), format(printf, 1, 2)));
-int redecl(const char *fmt, ...) __attribute__((modular_format(__second_impl, "__second", "two", "three")));
+int redecl(const char *fmt, ...) __attribute__((format(printf, 1, 2)));
+int redecl(const char *fmt, ...) __attribute__((modular_format(__dupe_impl, "__dupe", "1")));
+int redecl(const char *fmt, ...) __attribute__((modular_format(__dupe_impl, "__dupe", "1")));
 
 // CHECK-LABEL: define dso_local void @test_redecl(
-// CHECK:    {{.*}} = call i32 (ptr, ...) @redecl(ptr noundef @.str) #[[ATTR_REDECL:[0-9]+]]
+// CHECK:    {{.*}} = call i32 (ptr, ...) @redecl(ptr noundef @.str) #[[ATTR_DUPE_IDENTICAL:[0-9]+]]
 void test_redecl(void) {
   redecl("hello");
 }
@@ -35,15 +36,14 @@ void test_order(void) {
   order2("hello");
 }
 
-int overwrite(const char *fmt, ...) __attribute__((modular_format(__impl1, "__name1", "1"), modular_format(__impl2, "__name2", "2"), format(printf, 1, 2)));
+int duplicate_identical(const char *fmt, ...) __attribute__((modular_format(__dupe_impl, "__dupe", "1"), modular_format(__dupe_impl, "__dupe", "1"), format(printf, 1, 2)));
 
-// CHECK-LABEL: define dso_local void @test_overwrite(
-// CHECK:    {{.*}} = call i32 (ptr, ...) @overwrite(ptr noundef @.str) #[[ATTR_OVERWRITE:[0-9]+]]
-void test_overwrite(void) {
-  overwrite("hello");
+// CHECK-LABEL: define dso_local void @test_duplicate_identical(
+// CHECK:    {{.*}} = call i32 (ptr, ...) @duplicate_identical(ptr noundef @.str) #[[ATTR_DUPE_IDENTICAL]]
+void test_duplicate_identical(void) {
+  duplicate_identical("hello");
 }
 
 // CHECK: attributes #[[ATTR]] = { "modular-format"="printf,1,2,__modular_printf,__printf,float" }
-// CHECK: attributes #[[ATTR_REDECL]] = { "modular-format"="printf,1,2,__second_impl,__second,three,two" }
+// CHECK: attributes #[[ATTR_DUPE_IDENTICAL]] = { "modular-format"="printf,1,2,__dupe_impl,__dupe,1" }
 // CHECK: attributes #[[ATTR_ORDER]] = { "modular-format"="printf,1,2,__modular_printf,__printf,a,b" }
-// CHECK: attributes #[[ATTR_OVERWRITE]] = { "modular-format"="printf,1,2,__impl2,__name2,2" }

clang/test/Sema/attr-modular-format.c

@@ -11,11 +11,16 @@ int multi_dupe(const char *fmt, ...)  __attribute__((modular_format(__modular_pr
 int same_attr(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float"), modular_format(__modular_printf, "__printf", "float"), format(printf, 1, 2))); // no-warning
 
 // Test with multiple different attributes on the same declaration.
-int diff_attr(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float"), format(printf, 1, 2), modular_format(__modular_printf, "__printf", "int"))); // expected-warning {{attribute 'modular_format' is already applied with different arguments}} expected-note {{conflicting attribute is here}}
+int diff_attr(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float"), format(printf, 1, 2), modular_format(__modular_printf, "__printf", "int"))); // expected-error {{attribute 'modular_format' is already applied with different arguments}} expected-note {{conflicting attribute is here}}
 
-int diff_attr2(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float"), format(printf, 1, 2), modular_format(__modular_printf, "__other", "float"))); // expected-warning {{attribute 'modular_format' is already applied with different arguments}} expected-note {{conflicting attribute is here}}
+int diff_attr2(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float"), format(printf, 1, 2), modular_format(__modular_printf, "__other", "float"))); // expected-error {{attribute 'modular_format' is already applied with different arguments}} expected-note {{conflicting attribute is here}}
 
-int diff_attr3(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float"), format(printf, 1, 2), modular_format(__other, "__printf", "float"))); // expected-warning {{attribute 'modular_format' is already applied with different arguments}} expected-note {{conflicting attribute is here}}
+int diff_attr3(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float"), format(printf, 1, 2), modular_format(__other, "__printf", "float"))); // expected-error {{attribute 'modular_format' is already applied with different arguments}} expected-note {{conflicting attribute is here}}
 
 // Test with same attributes but different aspect order.
-int diff_order(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float", "int"), format(printf, 1, 2), modular_format(__modular_printf, "__printf", "int", "float"))); // no-warning
+int diff_order(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float", "int"), format(printf, 1, 2), modular_format(__modular_printf, "__printf", "int", "float"))); // no-error
+
+// Test with multiple different attributes on a declaration and a redeclaration
+int redecl(const char *fmt, ...) __attribute__((format(printf, 1, 2))); // no-error
+int redecl(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "float"))); // expected-note {{conflicting attribute is here}}
+int redecl(const char *fmt, ...) __attribute__((modular_format(__modular_printf, "__printf", "int"))); // expected-error {{attribute 'modular_format' is already applied with different arguments}}