[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

4539

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

4539

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

4539

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).

4706

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

4773

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

4539

Agree. Fixed.

4706

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

4773

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

4700

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

static OverloadingResult TryRefInitWithConversionFunction(
   if (T1RecordType && T1RecordType->getDecl()->isInvalidDecl())
     return OR_No_Viable_Function;
 
   const RecordType *T2RecordType = nullptr;
   if ((T2RecordType = T2->getAs<RecordType>()) &&
       S.isCompleteType(Kind.getLocation(), T2)) {
     // The type we're converting from is a class type, enumerate its conversion
     // functions.
-    CXXRecordDecl *T2RecordDecl = cast<CXXRecordDecl>(T2RecordType->getDecl());
-
+    if (CXXRecordDecl *T2RecordDecl =
+        dyn_cast<CXXRecordDecl>(T2RecordType->getDecl())) {
       const auto &Conversions = T2RecordDecl->getVisibleConversionFunctions();
       for (auto I = Conversions.begin(), E = Conversions.end(); I != E; ++I) {
         NamedDecl *D = *I;
         CXXRecordDecl *ActingDC = cast<CXXRecordDecl>(D->getDeclContext());
         if (isa<UsingShadowDecl>(D))
           D = cast<UsingShadowDecl>(D)->getTargetDecl();
 
         FunctionTemplateDecl *ConvTemplate = dyn_cast<FunctionTemplateDecl>(D);
         CXXConversionDecl *Conv;
         if (ConvTemplate)
           Conv = cast<CXXConversionDecl>(ConvTemplate->getTemplatedDecl());
         else
           Conv = cast<CXXConversionDecl>(D);
 
         // 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.
+        // 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);
+                Conv, I.getPair(), ActingDC, Initializer, DestType,
+                CandidateSet, /*AllowObjCConversionOnExplicit=*/false,
+                AllowExplicitConvs);
+        }
       }
     }
   }
   if (T2RecordType && T2RecordType->getDecl()->isInvalidDecl())
     return OR_No_Viable_Function;
 
   SourceLocation DeclLoc = Initializer->getBeginLoc();
 

 static bool isNonReferenceableGLValue(Expr *E) {
   return E->refersToBitField() || E->refersToVectorElement() ||
          E->refersToMatrixElement();
 }
 
 /// Reference initialization without resolving overloaded functions.
 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())) {
       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,

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