This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fix crash when handling nested immediate invocations
ClosedPublic

Authored by Fznamznon on Mar 16 2023, 9:34 AM.

Details

Summary

Before this patch it was expected that if there was several immediate
invocations they all belong to the same expression evaluation context.
During parsing of non local variable initializer a new evaluation context is
pushed, so code like this

namespace scope {
struct channel {
    consteval channel(const char* name) noexcept { }
};
consteval const char* make_channel_name(const char* name) { return name;}

channel rsx_log(make_channel_name("rsx_log"));
}

produced a nested immediate invocation whose subexpressions are attached
to different expression evaluation contexts. The constructor call
belongs to TU context and make_channel_name call to context of
variable initializer.

This patch removes this assumption and adds tracking of previously
failed immediate invocations, so it is possible when handling an
immediate invocation th check that its subexpressions from possibly another
evaluation context contains errors and not produce duplicate
diagnostics.

Fixes https://github.com/llvm/llvm-project/issues/58207

Diff Detail

Event Timeline

Fznamznon created this revision.Mar 16 2023, 9:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 9:34 AM
Fznamznon requested review of this revision.Mar 16 2023, 9:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 9:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thank you for the fix.

clang/lib/Sema/SemaExpr.cpp
17914

It is not obvious to me why this is correct, can you please explain.

Fznamznon added inline comments.Mar 16 2023, 12:36 PM
clang/lib/Sema/SemaExpr.cpp
17914

The visitor which this function is a part of traverses all subexpressions of a given ImmediateInvocationCandidate, it is saved to CurrentII member. Each ImmediateIvocationCandidate is a pair of integer and a pointer to ConstantExpr. The non-zero integer indicates that we should not try to evaluate particular immediate invocation. So, this patch, once found out that current expression contains a subexpression from another evaluation context whose evaluation failed before, marks that we shouldn't evaluate it to avoid double diagnosing. Otherwise, if some subexpression was found in the same context that current ImmediateInvocationCandidate belongs to, mark that we should not evaluate it instead, again to avoid double diagnosing. Hope that helps.

Sorry for the late review.
I think this makes sense generally, i just have one question

clang/lib/Sema/SemaExpr.cpp
17996

Shouln't we clear FailedImmediateInvocations at the end of a full expression to avoid going there if there was an error in another expression

Fznamznon added inline comments.Mar 23 2023, 9:47 AM
clang/lib/Sema/SemaExpr.cpp
17996

The idea sounds reasonable, but I'm not sure it can work with current algorithm of handling immediate invocations. Immediate invocations are handled when expression evaluation context is popped, so we need to remember previously failed immediate invocations until then, even though they could happen in two different full expressions.

consteval foo() { ... }
void bar() {
  int a = foo(/* there might be a failed immediate invocation attached to initializer context */); // this is a full-expression
  int b = foo(); // this is another full-expression
} // This is where immediate invocations that appear in function context are processed

I think this LGTM, but I'm holding off on signing off until @shafik is satisfied with the part he was asking about. You should add a release note for the fix, too.

clang/lib/Sema/SemaExpr.cpp
17908–17918

Minor grammar nits.

17990–17996
17996
Fznamznon updated this revision to Diff 508974.Mar 28 2023, 5:52 AM

Rebase, add release note, fix grammar

Fznamznon updated this revision to Diff 509039.Mar 28 2023, 8:55 AM

Fix formatting

shafik accepted this revision.Mar 30 2023, 4:06 PM

LGTM

This revision is now accepted and ready to land.Mar 30 2023, 4:06 PM

@aaron.ballman , @cor3ntin are you ok with the patch?