[WebAssembly] Address review comments on r352930

[sunfish]

Summary

This patch addresses the review comments on r352930:

• Removes redundant diagnostic checking code
• Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.
• Adds a test for various invalid cases for import_name and import_module.

[aaron.ballman]

I love functional changes that remove code and add tests -- thank you for these!

Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.

If you need to keep the prohibition that these attributes not be applied to functions with definitions, there are ways to accomplish that (we merge declarations and can decide what to do at that point if we see a declaration that is a definition). Given that, do you still want to lift this restriction?

test/Sema/attr-wasm.c

3 ↗

(On Diff #191200)

Was this intended to be used somewhere? Probably can just be removed if not.

15 ↗

(On Diff #191200)

Same here.

[sunfish]

In D59520#1434854, @aaron.ballman wrote:

Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.

If you need to keep the prohibition that these attributes not be applied to functions with definitions, there are ways to accomplish that (we merge declarations and can decide what to do at that point if we see a declaration that is a definition). Given that, do you still want to lift this restriction?

These attributes don't make sense on definitions. So right now, they are silently ignored when applied to definitions. That's effectively the current behavior too, because the existing check doesn't work as intended, so the change here doesn't change anything in practice yet.

A diagnostic would be slightly tidier, but I'm not familiar enough with clang to do this quickly and didn't want to delay the rest of the patch while I investigated. Do you know the specific places we'd need to change to diagnose this?

[sunfish]

test/Sema/attr-wasm.c

3 ↗

(On Diff #191200)

No, you're right that we don't need this (it's copypasta from another test). I'll remove it from the patch.

[aaron.ballman]

In D59520#1435728, @sunfish wrote:

In D59520#1434854, @aaron.ballman wrote:

Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.

If you need to keep the prohibition that these attributes not be applied to functions with definitions, there are ways to accomplish that (we merge declarations and can decide what to do at that point if we see a declaration that is a definition). Given that, do you still want to lift this restriction?

These attributes don't make sense on definitions. So right now, they are silently ignored when applied to definitions. That's effectively the current behavior too, because the existing check doesn't work as intended, so the change here doesn't change anything in practice yet.

A diagnostic would be slightly tidier, but I'm not familiar enough with clang to do this quickly and didn't want to delay the rest of the patch while I investigated. Do you know the specific places we'd need to change to diagnose this?

I'd be fine addressing it in a follow-up patch, but silently ignoring attributes is something I consider a bug. Users have a *very* difficult time seeing the difference between "accepted and doing its thing" and "silently ignored" depending on the attribute semantics, so that's why we explicitly tell them when we're ignoring an attribute.

As for where to handle this, I'd follow our typical "merge" pattern for attributes. 1) Add Sema::mergeImportNameAttr() that does all your error checking for definitions and creates an attribute if there's no issues, 2) From handleImportNameAttr(), handle your typical semantic checks and then calls S.mergeImportNameAttr() to try to create the attribute, and if it is created, attach it to the declaration, 3) hook the new merge operation up in mergeDeclAttribute() in SemaDecl.cpp.

If you don't address it as part of this patch, can you add a bug to bugzilla so that we get this tracked (if you don't plan to fix the issue soon) and add a test case to this patch demonstrating that we are silently ignoring the attribute with a FIXME comment?

[sunfish]

Implemented proper diagnostics for import_name/import_module on functions with definitions, and updated the test.

I'm unsure of the DIAG_SIZE_SEMA change, but without it, the build fails with this error:

/llvm/tools/clang/lib/Basic/DiagnosticIDs.cpp:72:3: error: static assertion failed: DIAG_SIZE_SEMA is insufficient to contain all diagnostics, it may need to be made larger in DiagnosticIDs.h.
   static_assert(                                                               \
   ^
/llvm/tools/clang/lib/Basic/DiagnosticIDs.cpp:88:1: note: in expansion of macro ‘VALIDATE_DIAG_SIZE’
 VALIDATE_DIAG_SIZE(SEMA)
 ^~~~~~~~~~~~~~~~~~

[aaron.ballman]

include/clang/Basic/DiagnosticIDs.h

39 ↗

(On Diff #197215)

I think this should be a separate commit, and I'd recommend updating it to 4000 to give ourselves more wiggle room.

include/clang/Basic/DiagnosticSemaKinds.td

9649–9656 ↗

(On Diff #197215)

How about: "import %select{module|name}0 does not match previous declaration" and "import %select{module|name}0 cannot be applied to a function with a definition"?

include/clang/Sema/Sema.h

2616–2621 ↗

(On Diff #197215)

I'd rather see the typical pattern used here: one taking a ParsedAttr and the other taking a semantic attribute.

lib/Sema/SemaDeclAttr.cpp

5764–5765 ↗

(On Diff #197215)

const auto *

5769 ↗

(On Diff #197215)

I don't see anything testing this note or the preceding warning, can you add some tests that exercise it?

5773 ↗

(On Diff #197215)

I don't see any tests for this either.

5781–5783 ↗

(On Diff #197215)

I wonder if we want to generalize this with a template (for the attribute type) if we could generalize the diagnostic text a bit more (or add an additional parameter for it)?

5786–5787 ↗

(On Diff #197215)

const auto *

5790–5791 ↗

(On Diff #197215)

Missing tests.

5795 ↗

(On Diff #197215)

Missing tests.

[sunfish]

I apologize for the extraordinary delays here; at long last, I've now addressed your feedback.

lib/Sema/SemaDeclAttr.cpp

5781–5783 ↗

(On Diff #197215)

The diagnostic name is now generalized, but it seems awkward to pass around the numbers to pick which variant of the string to use, so I didn't implement this yet.

[sunfish]

Address review feedback.

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

2601–2604

You should probably have some tests for the redeclaration behavior somewhere. The way you usually do this is to declare the function with the attribute, then declare the function again without the attribute but see that it is in fact inherited. We sometimes use AST dumping tests for this.

clang/test/Sema/attr-wasm.c

12

Are you intending to fix this as part of the patch?

16

Uncertain how important you think this may be: should we include the import names in this diagnostic? e.g., import name (%0) does not match the import name (%1) of the previous declaration or somesuch? I don't know how often users will hide these attributes behind macros, which is the case I was worried might be confusing.

26

Same question about this FIXME.

[sunfish]

• Add tests for redeclaration behavior
• Remove disabled tests (previously marked with FIXMEs)
• Made the mismatch warning more informative.

[sunfish]

I apologize again for the major delay. I've now updated the patch and addressed all of your comments.

clang/lib/Sema/SemaDecl.cpp

2601–2604

I've now added tests for this as you suggested.

clang/test/Sema/attr-wasm.c

12

I've made a few attempts at fixing this, but I'm not very familiar with these parts of clang, and I've been unable to find another attribute with a similar diagnostic.

The weak_import attribute seems like it should be one, with its warn_attribute_invalid_on_definition warning, and there's code in clang to issue that warning for both functions and variables, however the function version of the diagnostic doesn't seem to work, and only the variable version has tests.

So for now I've just removed these tests, to at least unblock the rest of this patch. If anyone knows how to implement these diagnostics, I'd be interested.

16

I've now updated the patch to provide a more informative message, following your suggestion here.

[aaron.ballman]

LGTM, thank you!

[thakis]

This breaks check-clang everywhere, e.g. here http://lab.llvm.org:8011/builders/clang-cmake-x86_64-sde-avx512-linux/builds/39534/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Awarning-flags.c

Please take a look and revert if it takes a while to fix.

Going forward, please run check-clang before landing clang changes, or look at the permerge bot output which predicted this failure (https://reviews.llvm.org/B59001)

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_matrix_index_outside_range: Error<
   "matrix %select{row|column}0 index is outside the allowed range [0, %1)">;
 def err_matrix_incomplete_index: Error<
   "single subscript expressions are not allowed for matrix values">;
 def err_matrix_separate_incomplete_index: Error<
   "matrix row and column subscripts cannot be separated by any expression">;
 def err_matrix_subscript_comma: Error<
   "comma expressions are not allowed as indices in matrix subscript expressions">;
 
+def warn_mismatched_import : Warning<
+  "import %select{module|name}0 (%1) does not match the import %select{module|name}0 (%2) of the "
+  "previous declaration">;
+def warn_import_on_definition : Warning<
+  "import %select{module|name}0 cannot be applied to a function with a definition">;
+
 def err_preserve_field_info_not_field : Error<
   "__builtin_preserve_field_info argument %0 not a field access">;
 def err_preserve_field_info_not_const: Error<
   "__builtin_preserve_field_info argument %0 not a constant">;
 def err_btf_type_id_not_const: Error<
   "__builtin_btf_type_id argument %0 not a constant">;
 
 def err_bit_cast_non_trivially_copyable : Error<

clang/include/clang/Sema/Sema.h

mergeSpeculativeLoadHardeningAttr(Decl *D,
                                     const SpeculativeLoadHardeningAttr &AL);
   OptimizeNoneAttr *mergeOptimizeNoneAttr(Decl *D,
                                           const AttributeCommonInfo &CI);
   InternalLinkageAttr *mergeInternalLinkageAttr(Decl *D, const ParsedAttr &AL);
   InternalLinkageAttr *mergeInternalLinkageAttr(Decl *D,
                                                 const InternalLinkageAttr &AL);
   CommonAttr *mergeCommonAttr(Decl *D, const ParsedAttr &AL);
   CommonAttr *mergeCommonAttr(Decl *D, const CommonAttr &AL);
+  WebAssemblyImportNameAttr *mergeImportNameAttr(
+      Decl *D, const WebAssemblyImportNameAttr &AL);
+  WebAssemblyImportModuleAttr *mergeImportModuleAttr(
+      Decl *D, const WebAssemblyImportModuleAttr &AL);
 
   void mergeDeclAttributes(NamedDecl *New, Decl *Old,
                            AvailabilityMergeKind AMK = AMK_Redeclaration);
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
                             LookupResult &OldDecls);
   bool MergeFunctionDecl(FunctionDecl *New, NamedDecl *&Old, Scope *S,
                          bool MergeTypeWithOld);
   bool MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old,

clang/lib/Sema/SemaDecl.cpp

else if ((isa<DeprecatedAttr>(Attr) || isa<UnavailableAttr>(Attr)) &&
             AMK == Sema::AMK_ProtocolImplementation))
     NewAttr = nullptr;
   else if (const auto *UA = dyn_cast<UuidAttr>(Attr))
     NewAttr = S.mergeUuidAttr(D, *UA, UA->getGuid(), UA->getGuidDecl());
   else if (const auto *SLHA = dyn_cast<SpeculativeLoadHardeningAttr>(Attr))
     NewAttr = S.mergeSpeculativeLoadHardeningAttr(D, *SLHA);
   else if (const auto *SLHA = dyn_cast<NoSpeculativeLoadHardeningAttr>(Attr))
     NewAttr = S.mergeNoSpeculativeLoadHardeningAttr(D, *SLHA);
+  else if (const auto *IMA = dyn_cast<WebAssemblyImportModuleAttr>(Attr))
+    NewAttr = S.mergeImportModuleAttr(D, *IMA);
+  else if (const auto *INA = dyn_cast<WebAssemblyImportNameAttr>(Attr))
+    NewAttr = S.mergeImportNameAttr(D, *INA);
   else if (Attr->shouldInheritEvenIfAlreadyPresent() || !DeclHasAttr(D, Attr))
     NewAttr = cast<InheritableAttr>(Attr->clone(S.Context));
 
   if (NewAttr) {
     NewAttr->setInherited(true);
     D->addAttr(NewAttr);
     if (isa<MSInheritanceAttr>(NewAttr))
       S.Consumer.AssignInheritanceModel(cast<CXXRecordDecl>(D));

clang/lib/Sema/SemaDeclAttr.cpp

static void handleWebAssemblyExportNameAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   SourceLocation ArgLoc;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &ArgLoc))
     return;
 
   D->addAttr(::new (S.Context) WebAssemblyExportNameAttr(S.Context, AL, Str));
   D->addAttr(UsedAttr::CreateImplicit(S.Context));
 }
 
-static void handleWebAssemblyImportModuleAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  if (!isFunctionOrMethod(D)) {
-    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
-        << "'import_module'" << ExpectedFunction;
-    return;
-  }
+WebAssemblyImportModuleAttr *
+Sema::mergeImportModuleAttr(Decl *D, const WebAssemblyImportModuleAttr &AL) {
+  auto *FD = cast<FunctionDecl>(D);
+
+  if (const auto *ExistingAttr = FD->getAttr<WebAssemblyImportModuleAttr>()) {
+    if (ExistingAttr->getImportModule() == AL.getImportModule())
+      return nullptr;
+    Diag(ExistingAttr->getLocation(), diag::warn_mismatched_import) << 0
+      << ExistingAttr->getImportModule() << AL.getImportModule();
+    Diag(AL.getLoc(), diag::note_previous_attribute);
+    return nullptr;
+  }
+  if (FD->hasBody()) {
+    Diag(AL.getLoc(), diag::warn_import_on_definition) << 0;
+    return nullptr;
+  }
+  return ::new (Context) WebAssemblyImportModuleAttr(Context, AL,
+                                                     AL.getImportModule());
 
+WebAssemblyImportNameAttr *
+Sema::mergeImportNameAttr(Decl *D, const WebAssemblyImportNameAttr &AL) {
   auto *FD = cast<FunctionDecl>(D);
-  if (FD->isThisDeclarationADefinition()) {
-    S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;
-    return;
-  }
+
+  if (const auto *ExistingAttr = FD->getAttr<WebAssemblyImportNameAttr>()) {
+    if (ExistingAttr->getImportName() == AL.getImportName())
+      return nullptr;
+    Diag(ExistingAttr->getLocation(), diag::warn_mismatched_import) << 1
+      << ExistingAttr->getImportName() << AL.getImportName();
+    Diag(AL.getLoc(), diag::note_previous_attribute);
+    return nullptr;
+  }
+  if (FD->hasBody()) {
+    Diag(AL.getLoc(), diag::warn_import_on_definition) << 1;
+    return nullptr;
+  }
+  return ::new (Context) WebAssemblyImportNameAttr(Context, AL,
+                                                   AL.getImportName());
 
+static void
+handleWebAssemblyImportModuleAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  auto *FD = cast<FunctionDecl>(D);
+
   StringRef Str;
   SourceLocation ArgLoc;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &ArgLoc))
     return;
+  if (FD->hasBody()) {
+    S.Diag(AL.getLoc(), diag::warn_import_on_definition) << 0;
+    return;
+  }
 
   FD->addAttr(::new (S.Context)
                   WebAssemblyImportModuleAttr(S.Context, AL, Str));
 }
 
-static void handleWebAssemblyImportNameAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  if (!isFunctionOrMethod(D)) {
-    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
-        << "'import_name'" << ExpectedFunction;
-    return;
-  }
-
+static void
+handleWebAssemblyImportNameAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   auto *FD = cast<FunctionDecl>(D);
-  if (FD->isThisDeclarationADefinition()) {
-    S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;
-    return;
-  }
 
   StringRef Str;
   SourceLocation ArgLoc;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &ArgLoc))
     return;
+  if (FD->hasBody()) {
+    S.Diag(AL.getLoc(), diag::warn_import_on_definition) << 1;
+    return;
+  }
 
   FD->addAttr(::new (S.Context) WebAssemblyImportNameAttr(S.Context, AL, Str));
 }
 
 static void handleRISCVInterruptAttr(Sema &S, Decl *D,
                                      const ParsedAttr &AL) {
   // Warn about repeated attributes.
   if (const auto *A = D->getAttr<RISCVInterruptAttr>()) {

clang/test/AST/ast-dump-wasm-attr-export.c

+// RUN: %clang_cc1 -triple wasm32-unknown-unknown -ast-dump %s | FileCheck --strict-whitespace %s
+
+// Test that functions can be redeclared and they retain their attributes.
+
+__attribute__((export_name("export_red"))) void red(void) {}
+__attribute__((export_name("export_orange"))) void orange(void) {}
+__attribute__((export_name("export_yellow"))) void yellow(void) {}
+
+void red(void);
+void orange(void);
+void yellow(void);
+
+// CHECK: |-FunctionDecl {{.+}} used red 'void (void)'
+// CHECK: | |-CompoundStmt {{.+}}
+// CHECK: | |-WebAssemblyExportNameAttr {{.+}} "export_red"
+// CHECK: | `-UsedAttr {{.+}} Implicit
+// CHECK: |-FunctionDecl {{.+}} used orange 'void (void)'
+// CHECK: | |-CompoundStmt {{.+}}
+// CHECK: | |-WebAssemblyExportNameAttr {{.+}} "export_orange"
+// CHECK: | `-UsedAttr {{.+}} Implicit
+// CHECK: |-FunctionDecl {{.+}} used yellow 'void (void)'
+// CHECK: | |-CompoundStmt {{.+}}
+// CHECK: | |-WebAssemblyExportNameAttr {{.+}} "export_yellow"
+// CHECK: | `-UsedAttr {{.+}} Implicit
+// CHECK: |-FunctionDecl {{.+}} used red 'void (void)'
+// CHECK: | |-UsedAttr {{.+}} Inherited Implicit
+// CHECK: | `-WebAssemblyExportNameAttr {{.+}} Inherited "export_red"
+// CHECK: |-FunctionDecl {{.+}} used orange 'void (void)'
+// CHECK: | |-UsedAttr {{.+}} Inherited Implicit
+// CHECK: | `-WebAssemblyExportNameAttr {{.+}} Inherited "export_orange"
+// CHECK: `-FunctionDecl {{.+}} used yellow 'void (void)'
+// CHECK:   |-UsedAttr {{.+}} Inherited Implicit
+// CHECK:     `-WebAssemblyExportNameAttr {{.+}} Inherited "export_yellow"

clang/test/AST/ast-dump-wasm-attr-import.c

+// RUN: %clang_cc1 -triple wasm32-unknown-unknown -ast-dump %s | FileCheck --strict-whitespace %s
+
+// Test that functions can be redeclared and they retain their attributes.
+
+__attribute__((import_name("import_red"), import_module("mod"))) void red(void);
+__attribute__((import_name("import_orange"), import_module("mod"))) void orange(void);
+__attribute__((import_name("import_yellow"), import_module("mod"))) void yellow(void);
+
+void red(void);
+void orange(void);
+void yellow(void);
+
+void calls(void) {
+    red();
+    orange();
+    yellow();
+}
+
+// CHECK: |-FunctionDecl {{.+}} used red 'void (void)'
+// CHECK: | |-WebAssemblyImportNameAttr {{.+}} "import_red"
+// CHECK: | `-WebAssemblyImportModuleAttr {{.+}} "mod"
+// CHECK: |-FunctionDecl {{.+}} used orange 'void (void)'
+// CHECK: | |-WebAssemblyImportNameAttr {{.+}} "import_orange"
+// CHECK: | `-WebAssemblyImportModuleAttr {{.+}} "mod"
+// CHECK: |-FunctionDecl {{.+}} used yellow 'void (void)'
+// CHECK: | |-WebAssemblyImportNameAttr {{.+}} "import_yellow"
+// CHECK: | `-WebAssemblyImportModuleAttr {{.+}} "mod"
+// CHECK: |-FunctionDecl {{.+}} used red 'void (void)'
+// CHECK: | |-WebAssemblyImportNameAttr {{.+}} Inherited "import_red"
+// CHECK: | `-WebAssemblyImportModuleAttr {{.+}} Inherited "mod"
+// CHECK: |-FunctionDecl {{.+}} used orange 'void (void)'
+// CHECK: | |-WebAssemblyImportNameAttr {{.+}} Inherited "import_orange"
+// CHECK: | `-WebAssemblyImportModuleAttr {{.+}} Inherited "mod"
+// CHECK: |-FunctionDecl {{.+}} used yellow 'void (void)'
+// CHECK: | |-WebAssemblyImportNameAttr {{.+}} Inherited "import_yellow"
+// CHECK: | `-WebAssemblyImportModuleAttr {{.+}} Inherited "mod"

clang/test/Sema/attr-wasm.c

+// RUN: %clang_cc1 -triple wasm32-unknown-unknown -fsyntax-only -verify %s
+
+void name_a() __attribute__((import_name)); //expected-error {{'import_name' attribute takes one argument}}
+
+int name_b __attribute__((import_name("foo"))); //expected-error {{'import_name' attribute only applies to functions}}
+
+void name_c() __attribute__((import_name("foo", "bar"))); //expected-error {{'import_name' attribute takes one argument}}
+
+void name_d() __attribute__((import_name("foo", "bar", "qux"))); //expected-error {{'import_name' attribute takes one argument}}
+
+void name_z() __attribute__((import_name("foo"))); //expected-note {{previous attribute is here}}
+
+void name_z() __attribute__((import_name("bar"))); //expected-warning {{import name (bar) does not match the import name (foo) of the previous declaration}}
+
+void module_a() __attribute__((import_module)); //expected-error {{'import_module' attribute takes one argument}}
+
+int module_b __attribute__((import_module("foo"))); //expected-error {{'import_module' attribute only applies to functions}}
+
+void module_c() __attribute__((import_module("foo", "bar"))); //expected-error {{'import_module' attribute takes one argument}}
+
+void module_d() __attribute__((import_module("foo", "bar", "qux"))); //expected-error {{'import_module' attribute takes one argument}}
+
+void module_z() __attribute__((import_module("foo"))); //expected-note {{previous attribute is here}}
+
+void module_z() __attribute__((import_module("bar"))); //expected-warning {{import module (bar) does not match the import module (foo) of the previous declaration}}
+
+void both() __attribute__((import_name("foo"), import_module("bar")));