[clang] Fix a crash on passing C structure of incompatible type to function with reference parameter

[ArcsinX]

Summary

__builtin_va_*() and __builtin_ms_va_*() declared as functions with reference parameters.

This patch fix clang crashes at using these functions in C and passing incompatible structures as parameters in case of __builtin_va_list/__builtin_ms_va_list are structures.

[riccibruno]

clang/lib/Sema/SemaInit.cpp

4562

I cannot reproduce this with the provided test case. Can you explain why T2RecordDecl would not be a CXXRecordDecl since we should only get there with c++?

[ArcsinX]

Fix format

[ArcsinX]

clang/lib/Sema/SemaInit.cpp

4562

You can reproduce it with

bin/clang -xc --target=arm-unknown-gnu -c ../clang/test/Sema/init-ref-c.c

We can get there with C in case of using builtins declared as functions with reference parameters (e.g. __builtin_va_end())

[riccibruno]

clang/lib/Sema/SemaInit.cpp

4562

Okay I see it now. Thank you.

I think it would be better to only call TryRefInitWithConversionFunction in C++ mode since conversion functions don't exist in C (only the first call on line 4773).

4707

Can you add a comment here explaining that we can get there in C with some builtins?

4774

Here

clang/test/Sema/init-ref-c.c

8

I think it would be better to name this test varargs-arm.c since this bug can only happen with the varargs-related builtins (__builtin_addressof takes a reference but has custom type-checking).

Additional test case:

const __builtin_va_list cva;
__builtin_va_end(cva); // expected-error {{binding reference of type '__builtin_va_list' to value of type 'const __builtin_va_list' drops 'const' qualifier}}

[ArcsinX]

clang/test/Sema/init-ref-c.c

8

It's not only about ARM, its about __builtin_va_list type which depends on architecture. E.g. on MIPS it also crashes.

Also it's not about __builtin_va_*() and __builtin_ms_va_*(), its about builtins with reference parameters, when we pass incompatible C structures to these builtin calls. May be there could be other builtins with reference parameters in the future.

We could not pass incompatible structure to __builtin_addressof() that is the reason why we could not reproduce the problem with this builtin.

[riccibruno]

clang/test/Sema/init-ref-c.c

8

It's not only about ARM, its about __builtin_va_list type which depends on architecture.
E.g. on MIPS it also crashes.

Sure.

Also it's not about __builtin_va_*() and __builtin_ms_va_*(), its about built-ins with
reference parameters, when we pass incompatible C structures to these builtin calls.
May be there could be other built-ins with reference parameters in the future.

Right, but currently only these built-ins and __builtin_addressof() have a parameter of reference type, and as you said __builtin_addressof() doesn't have this problem.

That said it's not a big deal so I think it's fine if you want to leave it here.

[ArcsinX]

Try conversion function only in C++.

[ArcsinX]

clang/lib/Sema/SemaInit.cpp

4562

Agree. Fixed.

4707

Added extra description for TryReferenceInitializationCore() with explanation that we can get here in C.

4774

Done.

[ArcsinX]

Ping

[riccibruno]

This LGTM, with a few wording nits.

This patch fix clang crashes at using these functions in C and passing incompatible structures as parameters in case of builtin_va_list/builtin_ms_va_list are structures.

This patch fixes a crash when using these functions in C where an argument of structure type is incompatible with the parameter type.

Do you want me to commit it for you? If so I need a name and an email for the attribution.

clang/lib/Sema/SemaInit.cpp

4698

s/call builtin/call a builtin/
s/with reference arguments/with a parameter of reference type/

The second wording change is important since "argument" != "parameter".

[ArcsinX]

In D82805#2139243, @riccibruno wrote:

Do you want me to commit it for you?

Yes, thank you.

If so I need a name and an email for the attribution.

Aleksandr Platonov <[e-mail]>

clang/lib/Sema/SemaInit.cpp

for (auto I = Conversions.begin(), E = Conversions.end(); I != E; ++I) {
       // If the conversion function doesn't return a reference type,
       // it can't be considered for this conversion unless we're allowed to
       // consider rvalues.
       // FIXME: Do we need to make sure that we only consider conversion
       // candidates with reference-compatible results? That might be needed to
       // break recursion.
       if ((AllowRValues ||
            Conv->getConversionType()->isLValueReferenceType())) {
         if (ConvTemplate)
           S.AddTemplateConversionCandidate(
               ConvTemplate, I.getPair(), ActingDC, Initializer, DestType,
               CandidateSet,
               /*AllowObjCConversionOnExplicit=*/false, AllowExplicitConvs);
         else
           S.AddConversionCandidate(
               Conv, I.getPair(), ActingDC, Initializer, DestType, CandidateSet,
               /*AllowObjCConversionOnExplicit=*/false, AllowExplicitConvs);

 /// which a reference can never bind). Attempting to bind a reference to
 /// such a glvalue will always create a temporary.
 static bool isNonReferenceableGLValue(Expr *E) {
   return E->refersToBitField() || E->refersToVectorElement() ||
          E->refersToMatrixElement();
 }
 
 /// Reference initialization without resolving overloaded functions.
+///
+/// We also can get here in C if we call builtin which is declared as
+/// a function with reference arguments (e.g. __builtin_va_end()).
 static void TryReferenceInitializationCore(Sema &S,
                                            const InitializedEntity &Entity,
                                            const InitializationKind &Kind,
                                            Expr *Initializer,
                                            QualType cv1T1, QualType T1,
                                            Qualifiers T1Quals,
                                            QualType cv2T2, QualType T2,
                                            Qualifiers T2Quals,
                                            InitializationSequence &Sequence) {
   QualType DestType = Entity.getType();
   SourceLocation DeclLoc = Initializer->getBeginLoc();
 
   // Compute some basic properties of the types and the initializer.
   bool isLValueRef = DestType->isLValueReferenceType();
   bool isRValueRef = !isLValueRef;
   Expr::Classification InitCategory = Initializer->Classify(S.Context);
 
if (isLValueRef || T1Function) {
     //       reference-related to T2, and can be implicitly converted to an
     //       lvalue of type "cv3 T3," where "cv1 T1" is reference-compatible
     //       with "cv3 T3" (this conversion is selected by enumerating the
     //       applicable conversion functions (13.3.1.6) and choosing the best
     //       one through overload resolution (13.3)),
     // If we have an rvalue ref to function type here, the rhs must be
     // an rvalue. DR1287 removed the "implicitly" here.
     if (RefRelationship == Sema::Ref_Incompatible && T2->isRecordType() &&
         (isLValueRef || InitCategory.isRValue())) {
+      if (S.getLangOpts().CPlusPlus) {
+        // Try conversion functions only for C++
         ConvOvlResult = TryRefInitWithConversionFunction(
             S, Entity, Kind, Initializer, /*AllowRValues*/ isRValueRef,
             /*IsLValueRef*/ isLValueRef, Sequence);
         if (ConvOvlResult == OR_Success)
           return;
         if (ConvOvlResult != OR_No_Viable_Function)
           Sequence.SetOverloadFailure(
               InitializationSequence::FK_ReferenceInitOverloadFailed,
               ConvOvlResult);
+      } else {
+        ConvOvlResult = OR_No_Viable_Function;
+      }
     }
   }
 
   //     - Otherwise, the reference shall be an lvalue reference to a
   //       non-volatile const type (i.e., cv1 shall be const), or the reference
   //       shall be an rvalue reference.
   //       For address spaces, we interpret this to mean that an addr space
   //       of a reference "cv1 T1" is a superset of addr space of "cv2 T2".

clang/test/Sema/init-ref-c.c

+// RUN: %clang_cc1 -triple arm-unknown-gnu -fsyntax-only -verify %s
+
+void f() {
+  struct EmptyStruct {};
+  struct EmptyStruct S;
+  __builtin_va_end(S); // no-crash, expected-error {{non-const lvalue reference to type '__builtin_va_list' cannot bind to a value of unrelated type 'struct EmptyStruct'}}
+}
 No newline at end of file