[clang][Sema] Remove dead diagnostic for loss of __unaligned qualifier

[hazohelet]

Summary

D120936 has made the loss of __unaligned qualifier NOT a bad-conversion.
Because of this, the bad-conversion note about the loss of this qualifier does not take effect.
e.g.

void foo(int *ptr);

void func(const __unaligned int *var) { foo(var); }

BEFORE this patch:

source.cpp:3:41: error: no matching function for call to 'foo'
    3 | void func(const __unaligned int *var) { foo(var); }
      |                                         ^~~
source.cpp:1:6: note: candidate function not viable: 1st argument ('const __unaligned int *') would lose __unaligned qualifier
    1 | void foo(int *ptr);
      |      ^
    2 |
    3 | void func(const __unaligned int *var) { foo(var); }
      |                                             ~~~

AFTER this patch:

source.cpp:3:41: error: no matching function for call to 'foo'
    3 | void func(const __unaligned int *var) { foo(var); }
      |                                         ^~~
source.cpp:1:6: note: candidate function not viable: 1st argument ('const __unaligned int *') would lose const qualifier
    1 | void foo(int *ptr);
      |      ^
    2 |
    3 | void func(const __unaligned int *var) { foo(var); }
      |                                             ~~~

Please note the different mentions of __unaligned and const in notes.

[rnk]

lgtm

[cjdb]

Can we get a snippet of code to demonstrate what isn't being diagnosed anymore in the commit message, please?

[hazohelet]

Ah looking at the code again, I noticed this change is not NFC. This suppresses inappropriate diagnostics (example added in summary).

[hazohelet]

Added release note and test because this change is not NFC

[cjdb]

Excellent, thanks! LGTM, do you need assistance with merging?

[hazohelet]

In D153690#4453001, @cjdb wrote:

Excellent, thanks! LGTM, do you need assistance with merging?

Thanks for the review. I'll land this myself.

[michael-jabbour-sonarsource]

The removal of the early return in this patch causes a crash when parsing the following example:

struct S {
  bool operator==(int) const;
};

void func() {
  S __unaligned s;
  s == 42;
}

See this Compiler Explorer link with a stacktrace: https://godbolt.org/z/n9vscYoYa.

Maybe we want to avoid the call to DiagnoseBadConversion here? I think that we don't want OverloadCandidateSet::BestViableFunction (called in Sema::CreateOverloadedBinOp) to return OR_No_Viable_Function in this case?

[hazohelet]

In D153690#4624726, @michael-jabbour-sonarsource wrote:

The removal of the early return in this patch causes a crash when parsing the following example:

struct S {
  bool operator==(int) const;
};

void func() {
  S __unaligned s;
  s == 42;
}

See this Compiler Explorer link with a stacktrace: https://godbolt.org/z/n9vscYoYa.

Maybe we want to avoid the call to DiagnoseBadConversion here? I think that we don't want OverloadCandidateSet::BestViableFunction (called in Sema::CreateOverloadedBinOp) to return OR_No_Viable_Function in this case?

Thank you for the report and reproducer!
@rnk This reproducer code should be accepted for clang to be aligned with msvc's behavior, correct?

About the fix, there's probably some code that makes a candidate that loses const qualifier invalid, and the __unaligned is likely to be handled in the same place. (I haven't checked the code, so may be off base)

[rnk]

In D153690#4627938, @hazohelet wrote:

@rnk This reproducer code should be accepted for clang to be aligned with msvc's behavior, correct?

Yes, I think so.

[hazohelet]

I submitted PR to fix the crash: https://github.com/llvm/llvm-project/pull/65248

clang/docs/ReleaseNotes.rst

 - Clang now diagnoses unexpected tokens after a
   ``#pragma clang|GCC diagnostic push|pop`` directive.
   (`#13920: <https://github.com/llvm/llvm-project/issues/13920>`_)
 - Clang now does not try to analyze cast validity on variables with dependent alignment (`#63007: <https://github.com/llvm/llvm-project/issues/63007>`_).
 - Clang constexpr evaluator now displays member function calls more precisely
   by making use of the syntactical structure of function calls. This avoids display
   of syntactically invalid codes in diagnostics.
   (`#57081: <https://github.com/llvm/llvm-project/issues/57081>`_)
+- Clang no longer emits inappropriate notes about the loss of ``__unaligned`` qualifier
+  on overload resolution, when the actual reason for the failure is loss of other qualifiers.
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix segfault while running clang-rename on a non existing file.
   (`#36471 <https://github.com/llvm/llvm-project/issues/36471>`_)
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def note_ovl_candidate_bad_cvr_this : Note<
     "%select{const|restrict|const or restrict|volatile|const or volatile|"
     "volatile or restrict|const, volatile, or restrict}4">;
 def note_ovl_candidate_bad_cvr : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "%ordinal5 argument (%3) would lose "
     "%select{const|restrict|const and restrict|volatile|const and volatile|"
     "volatile and restrict|const, volatile, and restrict}4 qualifier"
     "%select{||s||s|s|s}4">;
-def note_ovl_candidate_bad_unaligned : Note<
-    "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
-    "%ordinal5 argument (%3) would lose __unaligned qualifier">;
 def note_ovl_candidate_bad_base_to_derived_conv : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "cannot %select{convert from|convert from|bind}3 "
     "%select{base class pointer|superclass|base class object of type}3 %4 to "
     "%select{derived class pointer|subclass|derived class reference}3 %5 for "
     "%ordinal6 argument">;
 def note_ovl_candidate_bad_target : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "

clang/lib/Sema/SemaOverload.cpp

if (FromQs.getObjCGCAttr() != ToQs.getObjCGCAttr()) {
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
           << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
           << FromQs.getObjCGCAttr() << ToQs.getObjCGCAttr()
           << (unsigned)isObjectArgument << I + 1;
       MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
       return;
     }
 
-    if (FromQs.hasUnaligned() != ToQs.hasUnaligned()) {
-      S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_unaligned)
-          << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-          << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
-          << FromQs.hasUnaligned() << I + 1;
-      MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
-      return;
-    }
-
     unsigned CVR = FromQs.getCVRQualifiers() & ~ToQs.getCVRQualifiers();
     assert(CVR && "expected qualifiers mismatch");
 
     if (isObjectArgument) {
       S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_cvr_this)
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
           << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
           << (CVR - 1);

clang/test/SemaCXX/MicrosoftExtensions.cpp

 };
 
 __unaligned int aligned_type4::*p1_aligned_type4 = &aligned_type4::i;
 int aligned_type4::* __unaligned p2_aligned_type4 = &aligned_type4::i;
 __unaligned int aligned_type4::* __unaligned p3_aligned_type4 = &aligned_type4::i;
 void (aligned_type4::*__unaligned p4_aligned_type4)();
 
 // Check that __unaligned qualifier can be used for overloading
-void foo_unaligned(int *arg) {}
-void foo_unaligned(__unaligned int *arg) {}
+void foo_unaligned(int *arg) {} // expected-note {{not viable: 1st argument ('const __unaligned int *') would lose const qualifier}}
+void foo_unaligned(__unaligned int *arg) {} // expected-note {{not viable: 1st argument ('const __unaligned int *') would lose const qualifier}}
 void foo_unaligned(int arg) {} // expected-note {{previous definition is here}}
-void foo_unaligned(__unaligned int arg) {} // expected-error {{redefinition of 'foo_unaligned'}}
+void foo_unaligned(__unaligned int arg) {} // expected-error {{redefinition of 'foo_unaligned'}} \
+                                           // expected-note {{not viable: no known conversion from 'const __unaligned int *' to '__unaligned int'}}
 class A_unaligned {};
 class B_unaligned : public A_unaligned {};
-int foo_unaligned(__unaligned A_unaligned *arg) { return 0; }
-void *foo_unaligned(B_unaligned *arg) { return 0; }
+int foo_unaligned(__unaligned A_unaligned *arg) { return 0; } // expected-note {{not viable: no known conversion from 'const __unaligned int *' to '__unaligned A_unaligned *'}}
+void *foo_unaligned(B_unaligned *arg) { return 0; } // expected-note {{not viable: no known conversion from 'const __unaligned int *' to 'B_unaligned *'}}
 
 void test_unaligned() {
   int *p1 = 0;
   foo_unaligned(p1);
 
+  const __unaligned int *const_p1 = 0;
+  foo_unaligned(const_p1); // expected-error {{no matching function for call to 'foo_unaligned'}}
+
   __unaligned int *p2 = 0;
   foo_unaligned(p2);
 
   __unaligned B_unaligned *p3 = 0;
   int p4 = foo_unaligned(p3); // expected-error {{cannot initialize a variable of type 'int' with an rvalue of type 'void *'}}
   // expected-warning@-1 {{implicit cast from type '__unaligned B_unaligned *' to type 'B_unaligned *' drops __unaligned qualifier}}
 
   B_unaligned *p5 = p3;