[clang] Fix crash in concept deprecation #98622

Endilll · 2024-07-12T12:13:15Z

There is a gap between getAs<AutoType>() and getConstrainedAutoType() that the original patch #92295 was not aware of.

Fixes #98164

Apparently we can't assume that `AutoTypeLoc` from `AutoType` is always valid. Fixes llvm#98164

llvmbot · 2024-07-12T12:13:50Z

@llvm/pr-subscribers-clang

Author: Vlad Serebrennikov (Endilll)

Changes

Apparently we can't assume that AutoTypeLoc from AutoType is always valid.

Fixes #98164

Full diff: https://github.com/llvm/llvm-project/pull/98622.diff

3 Files Affected:

(modified) clang/lib/Sema/SemaDecl.cpp (+5-4)
(modified) clang/lib/Sema/SemaType.cpp (+5-5)
(modified) clang/test/SemaCXX/cxx-deprecated.cpp (+7)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 66eeaa8e6f777..7e4328cc176a2 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -7416,10 +7416,11 @@ NamedDecl *Sema::ActOnVariableDeclarator(
     tryToFixVariablyModifiedVarType(TInfo, R, D.getIdentifierLoc(),
                                     /*DiagID=*/0);
 
-  if (const AutoType *AutoT = R->getAs<AutoType>())
-    CheckConstrainedAuto(
-        AutoT,
-        TInfo->getTypeLoc().getContainedAutoTypeLoc().getConceptNameLoc());
+  if (const AutoType *AutoT = R->getAs<AutoType>()) {
+    AutoTypeLoc Loc = TInfo->getTypeLoc().getContainedAutoTypeLoc();
+    CheckConstrainedAuto(AutoT,
+                         Loc ? Loc.getConceptNameLoc() : SourceLocation());
+  }
 
   bool IsMemberSpecialization = false;
   bool IsVariableTemplateSpecialization = false;
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 714409f927a8d..887d1b395d475 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -6308,11 +6308,11 @@ TypeResult Sema::ActOnTypeName(Declarator &D) {
     CheckExtraCXXDefaultArguments(D);
   }
 
-  if (const AutoType *AutoT = T->getAs<AutoType>())
-    CheckConstrainedAuto(
-        AutoT,
-        TInfo->getTypeLoc().getContainedAutoTypeLoc().getConceptNameLoc());
-
+  if (const AutoType *AutoT = T->getAs<AutoType>()) {
+    AutoTypeLoc Loc = TInfo->getTypeLoc().getContainedAutoTypeLoc();
+    CheckConstrainedAuto(AutoT,
+                         Loc ? Loc.getConceptNameLoc() : SourceLocation());
+  }
   return CreateParsedType(T, TInfo);
 }
 
diff --git a/clang/test/SemaCXX/cxx-deprecated.cpp b/clang/test/SemaCXX/cxx-deprecated.cpp
index 81eb07608300d..870930f54af72 100644
--- a/clang/test/SemaCXX/cxx-deprecated.cpp
+++ b/clang/test/SemaCXX/cxx-deprecated.cpp
@@ -36,4 +36,11 @@ template <C T>
 // expected-warning@-1 {{'C' is deprecated}}
 //   expected-note@#C {{'C' has been explicitly marked deprecated here}}
 void f();
+
+template <int>
+auto b() = delete; // #b
+
+decltype(b<0>()) x;
+// expected-error@-1 {{call to deleted function 'b'}}
+//   expected-note@#b {{candidate function [with $0 = 0] has been explicitly deleted}}
 } // namespace cxx20_concept

AaronBallman

Do you have an idea as to why the type location is invalid in the first place?

Endilll · 2024-07-12T12:30:07Z

Do you have an idea as to why the type location is invalid in the first place?

Looking at the new test, my guess is that auto remains undeduced, so it has nowhere to point out to.

cor3ntin

LGTM but I agree with Aaron that tracking exactly where we create an invalid Loc would be interesting.

AaronBallman · 2024-07-12T12:43:15Z

Do you have an idea as to why the type location is invalid in the first place?

Looking at the new test, my guess is that auto remains undeduced, so it has nowhere to point out to.

Even when it's undeduced, we should still have a source location for it because the user wrote auto somewhere.

mizvekov · 2024-07-13T04:12:24Z

clang/lib/Sema/SemaDecl.cpp

+  if (const AutoType *AutoT = R->getAs<AutoType>()) {
+    AutoTypeLoc Loc = TInfo->getTypeLoc().getContainedAutoTypeLoc();


These two lines are analyzing the type in very different ways, so there seems to be further bugs here.

Type::getAs<AutoType> will simply desugar a type until it finds a node with type 'AutoType'.

getContainedAutoTypeLoc() will dig through a certain list of type nodes until it finds an 'AutoType'.
This list includes non-sugar nodes as well.

Let's assume for now we really want R->getAs<AutoType>() in the if condition here, which would not find the AutoType in auto *. This seems wrong, as these can be constrained as well. But we can handle that as a separate issue.

So to hit your bug, you are finding an AutoType through getAs, but not finding it through getContainedAutoTypeLoc().

My first suspicion would be that the type contains a sugar node not handled by GetContainedAutoTypeLocVisitor, in AST/TypeLoc.cpp.

Can you check that?

Oh, looking at the reproducer, I see the problem:

Our variable is getting the type of an undeduced AutoType through that decltype calling a deleted function with auto type.

This is confusing our semantic analysis for the variable, as now we think the variable was declared as auto, as getAs will look through all sugar nodes, 'decltype' included.

Since getContainedAutoTypeLoc() doesn't look through decltype, you get a crash.

But we shouldn't be getting an 'auto' here in the first place.

The correct fix would be to make that decltype have some error type, for error recovery purposes.

Since we don't implement a generic unknown error type, 'int' would still be better.

Thank you for the analysis!

The correct fix would be to make that decltype have some error type, for error recovery purposes.

Can you point me out to the place in our source where this change should be made?

Sure, but let me get back first to the other problem in this area:

I think that assumption I pointed out is wrong, we should not be using getAs<AutoType> here, as that will not dig through ReferenceType and PointerType, and the following example will not be diagnosed: https://godbolt.org/z/jzEaYPq4r

The getContainedAutoTypeLoc is what is a good fit for that, and what we should be using throughout. It will dig through the things needed for finding the AutoType, like pointers, references, and some of the necessary type sugar which can appear in source code when writing a deduced type. It does not dig through the things it doesn't need to, like decltype, because you can't write a deduced type with that.

So something like:

Suggested change

if (const AutoType *AutoT = R->getAs<AutoType>()) {

AutoTypeLoc Loc = TInfo->getTypeLoc().getContainedAutoTypeLoc();

if (AutoTypeLoc TL = TInfo->getTypeLoc().getContainedAutoTypeLoc()) {

const AutoType *AT = TL.getTypePtr();

Should work, and it will not get confused by the error recovery issue, which can be addressed separately.

Nit: We conventionally use TL abbreviation for TypeLoc, and Loc is conventional for SourceLocation.

Regarding the error recovery issue, in SemaOverload.cpp:

In FixOverloadedFunctionReference, for both UnresolvedLookupExpr and UnresolvedMemberExpr, we must avoid building a DeclRefExpr which has a function type returning undeduced auto.

Presently this should only happen for calls to deleted functions returning a deduced type, so should only happen when it's called from FinishOverloadedCallExpr, in the OR_Deleted case.

You can replace just the auto in the return type with int, keeping pointers and references in case of auto * or auto &. That can be done using Sema::SubstAutoType.

Eventually when we implement some kind of ErrorTy, for improving error recovery, we can change all such uses of 'int', using that instead.

I applied your suggestion, leaving error recovery issue for later, and also added tests for pointer and reference types. Thank you again for your excellent analysis.

If that's how it should be fixed today, I'm fine proceeding this way. That said, I find the approach somewhat backwards: morally I'm interested whether there's a constrained auto that might be using a concept, but we implement it asking for location of constrained auto, and get the AST node for constrained auto from it.

But here we are not just asking for the location of constrained auto.

A TypeLoc is not just a SourceLocation, it's a QualType with source locations.

The reason you are starting with a TypeLoc is that you are also interested in the source location.

This is a bit confusing because in a lot of places we pass both a QualType and a TypeLoc, which is mostly redundant. That might induce folks into thinking that a TypeLoc is just source locations on the side, which is not true.

But presently, they are not always redundant though. There are some cases we store both separately, and apply some transformations, like type deduction, only to the QualType, and leave the TypeLoc alone.
I think this is not ideal though, and seems to be mostly due to tech debt.

As an aside, if all you need to do is mark the concepts as used, you might as well do that earlier when the AutoType is formed, for example around the calls to ASTContext::getAutoType in SemaType.cpp / ConvertDeclSpecToType.

This makes more sense now.

A TypeLoc is not just a SourceLocation, it's a QualType with source locations.

I though that QualType with source locations is TypeSourceInfo. If I correctly understand description of TypeLoc correctly, TypeLoc is trailing data of TypeSourceInfo. Is that correct?

As an aside, if all you need to do is mark the concepts as used, you might as well do that earlier when the AutoType is formed, for example around the calls to ASTContext::getAutoType in SemaType.cpp / ConvertDeclSpecToType.

Calling DiagnoseUseOfDecl there seems too early. It triggers on declaration of deprecated concept.

If I correctly understand description of TypeLoc correctly, TypeLoc is trailing data of Type SourceInfo. Is that correct?

Yes, the fact we have both and use both, inconsistently, which is not an ideal situation

Calling DiagnoseUseOfDecl there seems too early. It triggers on declaration of deprecated concept.

This is weird but I'm not sure we need further changes here

Yeah, we don't need to have both TypeSourceInfo and TypeLoc in clang. I am not sure the history of how the situation arose, but we can pretty much consolidate on TypeLoc at some point.

mizvekov

See previous comments, I think this is primarily an error recovery issue on the decltype, we shouldn't let it escape an undeduced auto, as that would confuse further analysis.

mizvekov · 2024-07-17T18:23:06Z

clang/lib/Sema/SemaType.cpp

+  if (const AutoType *AutoT = T->getAs<AutoType>()) {
+    AutoTypeLoc Loc = TInfo->getTypeLoc().getContainedAutoTypeLoc();
+    CheckConstrainedAuto(AutoT,
+                         Loc ? Loc.getConceptNameLoc() : SourceLocation());
+  }


These still have the same issue, but I don't understand why we need this check here.

We obviously can't write stuff like

using X = auto;

So I am missing in what case undeduced auto can appear here, except the weird deleted function error recovery issue.

Ah, is this purely for the error cases?

Ie in my example above, you will still get the error, but is that for suppressing the deprecated warning in case that auto was constrained and happened to be the only use of that concept?

I did this for consistency, and now updated to use the same approach as in the other place.

But my question was, what cases are we handling with this check in ActOnTypeName?
I thought 'auto' couldn't appear here.

Test with constrained auto in trailing return type doesn't pass otherwise (we don't emit anything):

llvm-project/clang/test/CXX/drs/cwg24xx.cpp

Lines 78 to 81 in 2bdcfbe

template <typename T>

auto h() -> C auto {

// expected-warning@-1 {{'C' is deprecated}}

// expected-note@#cwg2428-C {{'C' has been explicitly marked deprecated here}}

cor3ntin

I think this makes sense but please give @mizvekov the day to review before landing

Thank you both for working on this

clang/test/SemaCXX/cxx-deprecated.cpp

cor3ntin · 2024-07-18T07:58:24Z

clang/lib/Sema/SemaDecl.cpp

+  if (const AutoType *AutoT = R->getAs<AutoType>()) {
+    AutoTypeLoc Loc = TInfo->getTypeLoc().getContainedAutoTypeLoc();


If I correctly understand description of TypeLoc correctly, TypeLoc is trailing data of Type SourceInfo. Is that correct?

Yes, the fact we have both and use both, inconsistently, which is not an ideal situation

Calling DiagnoseUseOfDecl there seems too early. It triggers on declaration of deprecated concept.

This is weird but I'm not sure we need further changes here

mizvekov · 2024-07-18T15:24:44Z

LGTM, Thanks!

Summary: There is a gap between `getAs<AutoType>()` and `getConstrainedAutoType()` that the original patch #92295 was not aware of. Fixes #98164 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250794

[clang] Fix crash in concept deprecation

5310764

Apparently we can't assume that `AutoTypeLoc` from `AutoType` is always valid. Fixes llvm#98164

Endilll added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Jul 12, 2024

Endilll requested review from cor3ntin and AaronBallman July 12, 2024 12:13

llvmbot added the clang Clang issues not falling into any other category label Jul 12, 2024

AaronBallman reviewed Jul 12, 2024

View reviewed changes

cor3ntin approved these changes Jul 12, 2024

View reviewed changes

mizvekov reviewed Jul 13, 2024

View reviewed changes

mizvekov requested changes Jul 13, 2024

View reviewed changes

Apply suggestion from @mizvekov

126057f

mizvekov reviewed Jul 17, 2024

View reviewed changes

Fix ActOnTypeName

270b9fc

cor3ntin approved these changes Jul 18, 2024

View reviewed changes

Wrap test case from the issue into a namespace

0a04859

mizvekov approved these changes Jul 18, 2024

View reviewed changes

Endilll merged commit 2bdcfbe into llvm:main Jul 18, 2024
7 checks passed

Endilll deleted the concept-deprecation-crash branch July 18, 2024 15:29

		if (const AutoType *AutoT = R->getAs<AutoType>()) {
		AutoTypeLoc Loc = TInfo->getTypeLoc().getContainedAutoTypeLoc();

	template <typename T>
	auto h() -> C auto {
	// expected-warning@-1 {{'C' is deprecated}}
	// expected-note@#cwg2428-C {{'C' has been explicitly marked deprecated here}}

[clang] Fix crash in concept deprecation #98622

[clang] Fix crash in concept deprecation #98622

Uh oh!

Conversation

Endilll commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 12, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Endilll commented Jul 12, 2024

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Jul 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mizvekov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Endilll Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mizvekov commented Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!

Endilll commented Jul 12, 2024 •

edited

Loading

Endilll Jul 18, 2024 •

edited

Loading