Download Raw Diff

Details

Reviewers

erichkeane
aaron.ballman

Commits

rGe417f02b5159: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

Summary

PR #61326

fix clang crash when fold expression contains a delayed typos correction.

code snippet in ActOnCXXFoldExpr

if (!LHS || !RHS) {
  Expr *Pack = LHS ? LHS : RHS;
  assert(Pack && "fold expression with neither LHS nor RHS");
  DiscardOperands();
  if (!Pack->containsUnexpandedParameterPack())
    return Diag(EllipsisLoc, diag::err_pack_expansion_without_parameter_packs)
           << Pack->getSourceRange();
}

DiscardOperands will be triggered when LHS/RHS is delayed typo correction expression.
It will output and clean all diagnose but still return a valid expression. (in else branch)
valid expression will be handled in caller function. When caller wants to output the diagnose, the diagnose in delayed typo correction expression has been consumed in ActOnCXXFoldExpr. It causes clang crash.

Diff Detail

Event Timeline

HerrCai0907 created this revision.Mar 12 2023, 8:01 PM

Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 8:01 PM

HerrCai0907 requested review of this revision.Mar 12 2023, 8:01 PM

Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 8:01 PM

Herald added a subscriber: cfe-commits. · View Herald Transcript

Harbormaster completed remote builds in B218926: Diff 504499.Mar 12 2023, 8:43 PM

This needs a release note. Also, the patch message doesn't do a good job explaining what is going on here.

Also, I'm not sure this is the right answer. The purpose of CorrectDelayedTyposInExpr is, in part, to make sure we emit the diagnostics of all child expressions. It seems this patch would make us miss other issues in the Foo<T>(Unknown) if they were to exist, which will make our compiler experience worse, particularly if it is the reason it seems that we don't see an expansion here. I cannot debug right now, but I suspect the fix to this issue is really in TransformTypoExpr.

The idea is that:

If function return an invalid ExprResult, this error should be handled in the callee
If function return valid Expr, the potential error should be handled in the caller, in this case, is ActOnExprStmt. If callee handles error, when caller find some error and want to output error message, the error in this expression has been processed and cause crash.

It seems this patch would make us miss other issues in the Foo<T>(Unknown) if they were to exist,

I try this code and it will not miss other issues

c++
template <typename... T>
void foo(T &&...Params) {
  Unknown;
  foo<T>(Unknown);
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
}

build-demo/a.cpp:3:3: error: use of undeclared identifier 'Unknown'
  Unknown;
  ^
build-demo/a.cpp:4:3: error: expression contains unexpanded parameter pack 'T'
  foo<T>(Unknown);
  ^   ~
build-demo/a.cpp:5:35: error: expression is not assignable
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
                                ~~^
build-demo/a.cpp:5:12: error: use of undeclared identifier 'Unknown'
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
           ^
build-demo/a.cpp:5:26: warning: division by zero is undefined [-Wdivision-by-zero]
  ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
                         ^ ~
build-demo/a.cpp:4:10: error: use of undeclared identifier 'Unknown'
  foo<T>(Unknown);
         ^
1 warning and 5 errors generated.

In D145892#4189603, @HerrCai0907 wrote:
The idea is that:

If function return an invalid ExprResult, this error should be handled in the callee

If function return valid Expr, the potential error should be handled in the caller, in this case, is ActOnExprStmt. If callee handles error, when caller find some error and want to output error message, the error in this expression has been processed and cause crash.

It seems this patch would make us miss other issues in the Foo<T>(Unknown) if they were to exist,

I try this code and it will not miss other issues
c++
template <typename... T>
void foo(T &&...Params) {
 Unknown;
 foo<T>(Unknown);
 ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
}
build-demo/a.cpp:3:3: error: use of undeclared identifier 'Unknown'
 Unknown;
 ^
build-demo/a.cpp:4:3: error: expression contains unexpanded parameter pack 'T'
 foo<T>(Unknown);
 ^ ~
build-demo/a.cpp:5:35: error: expression is not assignable
 ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
 ~~^
build-demo/a.cpp:5:12: error: use of undeclared identifier 'Unknown'
 ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
 ^
build-demo/a.cpp:5:26: warning: division by zero is undefined [-Wdivision-by-zero]
 ((foo<T>(Unknown + (10 / 0) + 10++)), ...);
 ^ ~
build-demo/a.cpp:4:10: error: use of undeclared identifier 'Unknown'
 foo<T>(Unknown);
 ^
1 warning and 5 errors generated.

Hmm... What about multiple packs, where the inner pack expansion would consume the ..., but not this one? Add a few more tests for the above and ones like this as well to confirm.

I cannot find an example that throw error: expression contains unexpanded parameter pack 'T' in fold expression because code like (foo<T>(10 + (static_cast(1))) + ...); can be unpack both T and U in same ... operator

But this code can be diagnosed correctly

template <typename... U> struct A {
  template <typename... T> void foo(T &&...Params) {
    (foo<T>(1 + (static_cast<U>(1))) + ...); // ok

    // error: expression contains unexpanded parameter pack 'T'
    foo<T>((... + static_cast<U>(1)));

    (foo<T>((... + static_cast<U>(1))) + ...); // ok
  }
};

Should I add this case into test?

HerrCai0907 edited the summary of this revision. (Show Details)Mar 13 2023, 4:59 PM

In D145892#4191220, @HerrCai0907 wrote:
I cannot find an example that throw error: expression contains unexpanded parameter pack 'T' in fold expression because code like (foo<T>(10 + (static_cast(1))) + ...); can be unpack both T and U in same ... operator

But this code can be diagnosed correctly
template <typename... U> struct A {
 template <typename... T> void foo(T &&...Params) {
 (foo<T>(1 + (static_cast(1))) + ...); // ok

 // error: expression contains unexpanded parameter pack 'T'
 foo<T>((... + static_cast(1)));

 (foo<T>((... + static_cast(1))) + ...); // ok
 }
};
Should I add this case into test?

I spent some time on this and I can't come up with any counter examples either. Please add this to the test, add a release note, then this LGTM.

add more testcase

Should I add release note in clang/docs/ReleaseNotes.rst or any other way?

In D145892#4193085, @HerrCai0907 wrote:

Should I add release note in clang/docs/ReleaseNotes.rst or any other way?

Yes, that is the correct place.

add release note

erichkeane added inline comments.Mar 14 2023, 8:11 AM

clang/docs/ReleaseNotes.rst
173	This should go in the `Bug Fixes In This Version` section, right below this. Also, try to elaborate and make it more like the format of the others around there.