[Sema] Catch a case when 'volatile' qualifier is dropped while binding

[davide]

Summary

This is an attempt to fix https://llvm.org/bugs/show_bug.cgi?id=23882
Sema doesn't diagnose that binding 'volatile W' to to reference of type 'const W&' will discard qualifier and we end up in an infinite loop later on (until we run out of stack).

InitSequence::Perform() -> ... -> GatherArgumentsForCall() -> InitSequence::Perform()

I'm not sure if the check is OK as is, or needs to be generalized and/or moved.
I'll add the testcase in the PR if this review is accepted.

Thanks,

Davide

[davide]

Very gentle ping.

[majnemer]

This needs a test case.

[davide]

Testcase.

[rjmccall]

That doesn't seem like a good diagnostic. The actual problem is that you can't copy-construct a W from a volatile W&, but the error is being reported at a lower level than overload resolution; and even if you accept that as given, it's still complaining about the qualifier mismatch as if that's more important that the fact that the types don't match up.

I think this is probably just not the right place to bail out of the loop.

Also, does the bug not reproduce if the S constructor's parameter isn't meaninglessly volatile-qualified? It's possible that we're just not stripping that properly in some place.

[cynecx]

Hi :),

First of all, I would like to say that I am new into all the clang stuff (software architecture/internal concepts) but I wanted to start hacking clang so I went here to check out some trivial clang bugs to fix so I can learn internal stuff about clang, with internal stuff I mean how AST/Sema are implemented and kinda these things.

So basically, I tried all the day long to find a better approach as @rjmccall explained that the key issue is that you can't copy-construct a W from a volatile W&. The issue might be in SemaInit.cpp -> ResolveConstructorOverload because when the NamedDecl* loop reaches the const S& constructor it doesn't know that it's actually copy-constructed so it's not setting SuppressUserConversions to true. This has the effect that AddTemplateOverloadCandidate also includes user-conversion functions, so in this case: S(volatile W) and this is actually what causes the infinity loop: PerformCopyInitialization -> InitializationSequence::Perform ->CompleteConstructorCall -> GatherArgumentsForCall -> ....
While debugging I basically changed the SuppressUserConversions variable to true for the W (const S&) constructor iteration and this solved the infinity-loop problem and produced a gcc similar diagnostic message:

simplified.cpp:13:7: error: no matching constructor for initialization of 'W'
  S s(*w);
      ^~
simplified.cpp:3:8: note: candidate constructor (the implicit copy constructor) not viable: 1st argument ('volatile W')
      would lose volatile qualifier
struct W {
       ^
simplified.cpp:3:8: note: candidate constructor (the implicit move constructor) not viable: 1st argument ('volatile W')
      would lose volatile qualifier
struct W {
       ^
simplified.cpp:4:3: note: candidate constructor not viable: no known conversion from 'volatile W' to 'const S &' for
      1st argument
  W( const S& ) {}
  ^
simplified.cpp:8:15: note: passing argument to parameter here
  S(volatile W) {}

It would be nice if anyone could check if this is a right approach for fixing this issue.
Thank you.

[rjmccall]

This is outside of my expertise, but I've asked Doug Gregor to take a look.

lib/Sema/SemaInit.cpp

   bool DerivedToBase = false;
   bool ObjCConversion = false;
   bool ObjCLifetimeConversion = false;
   Expr::Classification InitCategory = Initializer->Classify(S.Context);
   Sema::ReferenceCompareResult RefRelationship
     = S.CompareReferenceRelationship(DeclLoc, cv1T1, cv2T2, DerivedToBase,
                                      ObjCConversion, ObjCLifetimeConversion);
 
+
+  if (isLValueRef && (!T1Quals.hasVolatile() && T2Quals.hasVolatile())) {
+    Sequence.SetFailed(InitializationSequence::FK_ReferenceInitDropsQualifiers);
+    return;
+  }
+
   // C++0x [dcl.init.ref]p5:
   //   A reference to type "cv1 T1" is initialized by an expression of type
   //   "cv2 T2" as follows:
   //
   //     - If the reference is an lvalue reference and the initializer
   //       expression
   // Note the analogous bullet points for rvalue refs to functions. Because
   // there are no function rvalues in C++, rvalue refs to functions are treated

test/SemaCXX/bind-infinite-recursion.cpp

+// RUN: %clang_cc1 %s -fsyntax-only -verify
+
+struct S;
+
+struct W {
+  W( const S& ) {}  // expected-note {{passing argument to parameter here}}
+};
+
+struct S {
+  S(volatile W) {}
+};
+
+int main() {
+  volatile W *w = 0;
+  S s(*w);  // expected-error {{binding value of type 'volatile W' to reference to type 'const S' drops 'volatile' qualifier}}
+  return 0;
+}
+