This is an archive of the discontinued LLVM Phabricator instance.

Differential D142233

[Clang][OpenMP] Bail out early if `Scope` is nullptr in case of any crash
ClosedPublic

Authored by tianshilei1992 on Jan 20 2023, 9:32 AM.

Details

Reviewers
jdoerfert
ABataev
sandeepkosuri
cchen
jyu2
Commits
rG2ebda47619d7: [Clang][OpenMP] Bail out early if `Scope` is nullptr in case of any crash
Summary

When there is any compile error, clang still tries to compile as many code as
possible, therefore Scope can be nullptr here. However, we didn't check it
beforehand, causing compiler crash.

Fix #59944.

Diff Detail

Event Timeline

tianshilei1992 created this revision.Jan 20 2023, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 9:32 AM
Herald added subscribers: guansong, yaxunl. · View Herald Transcript
tianshilei1992 requested review of this revision.Jan 20 2023, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 9:32 AM
Herald added subscribers: cfe-commits, sstefan1. · View Herald Transcript
ABataev added inline comments.Jan 20 2023, 9:35 AM
clang/lib/Sema/SemaOpenMP.cpp
7262

Maybe better to check it here to avoid too early exit?

if (Scope && Scope->...)
tianshilei1992 added inline comments.Jan 20 2023, 9:36 AM
clang/lib/Sema/SemaOpenMP.cpp
7262

But does it make sense to continue? I'm not that sure.

ABataev added inline comments.Jan 20 2023, 9:37 AM
clang/lib/Sema/SemaOpenMP.cpp
7262

Yes, we can get some extra diagnostics, which may be helpful

tianshilei1992 updated this revision to Diff 490896.Jan 20 2023, 9:41 AM

fix comment

ABataev accepted this revision.Jan 20 2023, 9:47 AM

LG

This revision is now accepted and ready to land.Jan 20 2023, 9:47 AM
Harbormaster completed remote builds in B209022: Diff 490896.Jan 20 2023, 11:33 AM
This revision was landed with ongoing or failed builds.Jan 20 2023, 11:40 AM
Closed by commit rG2ebda47619d7: [Clang][OpenMP] Bail out early if `Scope` is nullptr in case of any crash (authored by tianshilei1992). · Explain Why
This revision was automatically updated to reflect the committed changes.
tianshilei1992 marked an inline comment as done.
tianshilei1992 added a commit: rG2ebda47619d7: [Clang][OpenMP] Bail out early if `Scope` is nullptr in case of any crash.
vitalybuka added a subscriber: vitalybuka.EditedJan 20 2023, 5:27 PM

Fails on this bot https://lab.llvm.org/buildbot/#/builders/5/builds/30884/steps/13/logs/stdio

tianshilei1992 added a comment.Jan 20 2023, 5:52 PM
In D142233#4070572, @vitalybuka wrote:

Fails on this bot https://lab.llvm.org/buildbot/#/builders/5/builds/30884/steps/13/logs/stdio

It's been fixed by https://github.com/llvm/llvm-project/commit/b561c5c149467c98094c87c0e0d07c3b05f17780. Thanks.

Revision Contents

PathSize
clang/
lib/
Sema/
2 lines
test/
OpenMP/
11 lines

Diff 490927

clang/lib/Sema/SemaOpenMP.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 7,253 Lines • ▼ Show 20 LinesExprResult Sema::ActOnOpenMPCall(ExprResult Call, Scope *Scope,
FunctionDecl *CalleeFnDecl = CE->getDirectCallee(); FunctionDecl *CalleeFnDecl = CE->getDirectCallee();
if (!CalleeFnDecl) if (!CalleeFnDecl)
return Call; return Call;
if (LangOpts.OpenMP >= 51 && CalleeFnDecl->getIdentifier() && if (LangOpts.OpenMP >= 51 && CalleeFnDecl->getIdentifier() &&
CalleeFnDecl->getName().startswith_insensitive("omp_")) { CalleeFnDecl->getName().startswith_insensitive("omp_")) {
// checking for any calls inside an Order region // checking for any calls inside an Order region
if (Scope->isOpenMPOrderClauseScope()) if (Scope && Scope->isOpenMPOrderClauseScope())
ABataevUnsubmitted
Done
ReplyInline Actions

Maybe better to check it here to avoid too early exit?

if (Scope && Scope->...)
ABataev: Maybe better to check it here to avoid too early exit? ``` if (Scope && Scope->...) ```
tianshilei1992AuthorUnsubmitted
Done
ReplyInline Actions

But does it make sense to continue? I'm not that sure.

tianshilei1992: But does it make sense to continue? I'm not that sure.
ABataevUnsubmitted
Not Done
ReplyInline Actions

Yes, we can get some extra diagnostics, which may be helpful

ABataev: Yes, we can get some extra diagnostics, which may be helpful
Diag(LParenLoc, diag::err_omp_unexpected_call_to_omp_runtime_api); Diag(LParenLoc, diag::err_omp_unexpected_call_to_omp_runtime_api);
} }
if (!CalleeFnDecl->hasAttr<OMPDeclareVariantAttr>()) if (!CalleeFnDecl->hasAttr<OMPDeclareVariantAttr>())
return Call; return Call;
ASTContext &Context = getASTContext(); ASTContext &Context = getASTContext();
std::function<void(StringRef)> DiagUnknownTrait = [this, std::function<void(StringRef)> DiagUnknownTrait = [this,
▲ Show 20 Lines • Show All 16,508 Lines • Show Last 20 Lines

clang/test/OpenMP/bug59944.c

  • This file was added.
// RUN: %clang_cc1 -fopenmp -fopenmp-version=52 -x c -triple x86_64-apple-darwin10 %s -o - 2>&1 | FileCheck %s --check-prefix=CHECK
extern int omp_get_initial_device();
extern void *omp_get_mapped_ptr(void *, int);
void t() {
omp_get_mapped_ptr(&x, omp_get_initial_device());
}
// CHECK: error: use of undeclared identifier 'x'
// CHECK-NOT: crash