This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] atomic compare fail : Parser & AST support
ClosedPublic

Authored by koops on Apr 6 2022, 10:22 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
koops updated this revision to Diff 431315.May 23 2022, 2:37 AM

Fixing a minor error : clang formatting of variable names to avoid build errors.

koops updated this revision to Diff 431385.May 23 2022, 8:24 AM

Clang formatting for variables in ParseOpenMP.cpp

koops updated this revision to Diff 431391.May 23 2022, 9:00 AM
tianshilei1992 accepted this revision.May 23 2022, 2:44 PM

LGTM

clang/lib/Parse/ParseOpenMP.cpp
3839–3841
This revision is now accepted and ready to land.May 23 2022, 2:44 PM
This revision was landed with ongoing or failed builds.May 24 2022, 9:57 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 9:57 PM
kda added a subscriber: kda.May 25 2022, 8:19 AM

This has broken sanitizer buildbot:
https://lab.llvm.org/buildbot/#/builders/5/builds/24074

Relevant log snippet:

FAIL: Clang :: OpenMP/atomic_messages.cpp (9696 of 66080)
******************** TEST 'Clang :: OpenMP/atomic_messages.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/15.0.0/include -nostdsysteminc -verify=expected,omp45 -fopenmp -fopenmp-version=45 -ferror-limit 150 /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/OpenMP/atomic_messages.cpp -Wuninitialized
: 'RUN: at line 2';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/15.0.0/include -nostdsysteminc -verify=expected,omp50 -fopenmp -ferror-limit 150 /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/OpenMP/atomic_messages.cpp -Wuninitialized
: 'RUN: at line 3';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/15.0.0/include -nostdsysteminc -DOMP51 -verify=expected,omp50,omp51 -fopenmp -fopenmp-version=51 -ferror-limit 150 /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/OpenMP/atomic_messages.cpp -Wuninitialized
: 'RUN: at line 5';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/15.0.0/include -nostdsysteminc -verify=expected,omp45 -fopenmp-simd -fopenmp-version=45 -ferror-limit 150 /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/OpenMP/atomic_messages.cpp -Wuninitialized
: 'RUN: at line 6';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/15.0.0/include -nostdsysteminc -verify=expected,omp50 -fopenmp-simd -ferror-limit 150 /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/OpenMP/atomic_messages.cpp -Wuninitialized
: 'RUN: at line 7';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/15.0.0/include -nostdsysteminc -DOMP51 -verify=expected,omp50,omp51 -fopenmp-simd -fopenmp-version=51 -ferror-limit 150 /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/OpenMP/atomic_messages.cpp -Wuninitialized
--
Exit Code: 1
Command Output (stderr):
--
==75693==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x561f8672eb0e in (anonymous namespace)::OpenMPAtomicFailChecker::checkSubClause(llvm::ArrayRef<clang::OMPClause*>, clang::SourceLocation*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOpenMP.cpp:12060:11
    #1 0x561f866ef4bc in clang::Sema::ActOnOpenMPAtomicDirective(llvm::ArrayRef<clang::OMPClause*>, clang::Stmt*, clang::SourceLocation, clang::SourceLocation) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOpenMP.cpp:12634:22
    #2 0x561f866c0775 in clang::Sema::ActOnOpenMPExecutableDirective(llvm::omp::Directive, clang::DeclarationNameInfo const&, llvm::omp::Directive, llvm::ArrayRef<clang::OMPClause*>, clang::Stmt*, clang::SourceLocation, clang::SourceLocation) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOpenMP.cpp:6168:11
    #3 0x561f856da291 in clang::Parser::ParseOpenMPDeclarativeOrExecutableDirective(clang::Parser::ParsedStmtContext, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:2922:25
    #4 0x561f8553b46f in clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*, clang::ParsedAttributes&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:413:12
    #5 0x561f85537f27 in clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:113:20
    #6 0x561f85551be1 in clang::Parser::ParseCompoundStatementBody(bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:1111:11
    #7 0x561f85554cfd in clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:2379:21
    #8 0x561f854cfa8e in clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:1407:10
    #9 0x561f855bbc3e in clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::SourceLocation*, clang::Parser::ForRangeInit*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:2097:27
    #10 0x561f854cb272 in clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:1158:10
    #11 0x561f854ca0b6 in clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:1172:12
    #12 0x561f854c754a in clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsingDeclSpec*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:998:12
    #13 0x561f854c0c9a in clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:727:12
    #14 0x561f854ae252 in clang::ParseAST(clang::Sema&, bool, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseAST.cpp:162:20
    #15 0x561f8203140d in clang::FrontendAction::Execute() /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1032:8
    #16 0x561f81e8da47 in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:1036:33
    #17 0x561f82278528 in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:25
    #18 0x561f79792507 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/cc1_main.cpp:248:15
    #19 0x561f7978b484 in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/driver.cpp:317:12
    #20 0x561f7978a434 in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/driver.cpp:388:12
    #21 0x7fada460f09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) (BuildId: eb6a5dd378d22b1e695984462a799cd4c81cdc22)
    #22 0x561f796fa6d9 in _start (/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang-15+0x480d6d9)
SUMMARY: MemorySanitizer: use-of-uninitialized-value /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOpenMP.cpp:12060:11 in (anonymous namespace)::OpenMPAtomicFailChecker::checkSubClause(llvm::ArrayRef<clang::OMPClause*>, clang::SourceLocation*)
Exiting
--
********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
********************
Failed Tests (1):
  Clang :: OpenMP/atomic_messages.cpp

Hello Kevin,

I am not sure why it is indicating an uninitialized variable at that point. The code at " llvm-project/clang/lib/Sema/SemaOpenMP.cpp:12060 " is as below:

for (const OMPClause *C : Clauses) {
  if (C->getClauseKind() == OMPC_fail) {
    NoOfFails++;
    const OMPFailClause *FC = static_cast<const OMPFailClause *>(C);
    const OMPClause *MemOrderC = FC->const_getMemoryOrderClause();
    /* Clauses contains OMPC_fail and the subclause */
    if (MemOrderC) {

Here " if (MemOrderC) { " is the 12060 line.

Regards,
--Sunil

I am not sure why it is indicating an uninitialized variable at that point. The code at " llvm-project/clang/lib/Sema/SemaOpenMP.cpp:12060 " is as below:

FWIW, it's outright crashing for me on Windows.

FAIL: Clang :: OpenMP/atomic_messages.cpp (255 of 15035)
******************** TEST 'Clang :: OpenMP/atomic_messages.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -verify=expected,omp45 -fopenmp -fopenmp-version=45 -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 2';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -verify=expected,omp50 -fopenmp -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 3';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -DOMP51 -verify=expected,omp50,omp51 -fopenmp -fopenmp-version=51 -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 5';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -verify=expected,omp45 -fopenmp-simd -fopenmp-version=45 -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 6';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -verify=expected,omp50 -fopenmp-simd -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 7';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -DOMP51 -verify=expected,omp50,omp51 -fopenmp-simd -fopenmp-version=51 -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
--
Exit Code: 3221225477

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include" "-nostdsysteminc" "-verify=expected,omp45" "-fopenmp" "-fopenmp-version=45" "-ferror-limit" "150" "F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp" "-Wuninitialized"
$ ":" "RUN: at line 2"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include" "-nostdsysteminc" "-verify=expected,omp50" "-fopenmp" "-ferror-limit" "150" "F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp" "-Wuninitialized"
$ ":" "RUN: at line 3"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include" "-nostdsysteminc" "-DOMP51" "-verify=expected,omp50,omp51" "-fopenmp" "-fopenmp-version=51" "-ferror-limit" "150" "F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp" "-Wuninitialized"
# command stderr:
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: f:\\source\\llvm-project\\llvm\\out\\build\\x64-debug\\bin\\clang.exe -cc1 -internal-isystem f:\\source\\llvm-project\\llvm\\out\\build\\x64-debug\\lib\\clang\\15.0.0\\include -nostdsysteminc -DOMP51 -verify=expected,omp50,omp51 -fopenmp -fopenmp-version=51 -ferror-limit 150 F:\\source\\llvm-project\\clang\\test\\OpenMP\\atomic_messages.cpp -Wuninitialized
1.      F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp:968:1: at annotation token
2.      F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp:930:13: parsing function body 'mixed'
3.      F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp:930:13: in compound statement ('{}')
 #0 0x00007ff61281a32b clang::OMPClause::getClauseKind(void) const F:\source\llvm-project\clang\include\clang\AST\OpenMPClause.h:83:0
 #1 0x00007ff617bcf805 `anonymous namespace'::OpenMPAtomicFailChecker::checkSubClause F:\source\llvm-project\clang\lib\Sema\SemaOpenMP.cpp:12061:0
 #2 0x00007ff617b5d38d clang::Sema::ActOnOpenMPAtomicDirective(class llvm::ArrayRef<class clang::OMPClause *>, class clang::Stmt *, class clang::SourceLocation, class clang::SourceLocation) F:\source\llvm-project\clang\lib\Sema\SemaOpenMP.cpp:12634:0
 #3 0x00007ff617b50901 clang::Sema::ActOnOpenMPExecutableDirective(enum llvm::omp::Directive, struct clang::DeclarationNameInfo const &, enum llvm::omp::Directive, class llvm::ArrayRef<class clang::OMPClause *>, class clang::Stmt *, class clang::SourceLocation, class clang::SourceLocation) F:\source\llvm-project\clang\lib\Sema\SemaOpenMP.cpp:6168:0
 #4 0x00007ff616c2fba8 clang::Parser::ParseOpenMPDeclarativeOrExecutableDirective(enum clang::Parser::ParsedStmtContext, bool) F:\source\llvm-project\clang\lib\Parse\ParseOpenMP.cpp:2932:0
 #5 0x00007ff616be46b2 clang::Parser::ParseStatementOrDeclarationAfterAttributes(class llvm::SmallVector<class clang::Stmt *, 32> &, enum clang::Parser::ParsedStmtContext, class clang::SourceLocation *, class clang::ParsedAttributes &) F:\source\llvm-project\clang\lib\Parse\ParseStmt.cpp:413:0
 #6 0x00007ff616be3342 clang::Parser::ParseStatementOrDeclaration(class llvm::SmallVector<class clang::Stmt *, 32> &, enum clang::Parser::ParsedStmtContext, class clang::SourceLocation *) F:\source\llvm-project\clang\lib\Parse\ParseStmt.cpp:115:0
 #7 0x00007ff616be6cc9 clang::Parser::ParseCompoundStatementBody(bool) F:\source\llvm-project\clang\lib\Parse\ParseStmt.cpp:1111:0
 #8 0x00007ff616bedb92 clang::Parser::ParseFunctionStatementBody(class clang::Decl *, class clang::Parser::ParseScope &) F:\source\llvm-project\clang\lib\Parse\ParseStmt.cpp:2382:0
 #9 0x00007ff616aef1ec clang::Parser::ParseFunctionDefinition(class clang::ParsingDeclarator &, struct clang::Parser::ParsedTemplateInfo const &, class clang::Parser::LateParsedAttrList *) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:1407:0
#10 0x00007ff616b3778c clang::Parser::ParseDeclGroup(class clang::ParsingDeclSpec &, enum clang::DeclaratorContext, class clang::SourceLocation *, struct clang::Parser::ForRangeInit *) F:\source\llvm-project\clang\lib\Parse\ParseDecl.cpp:2097:0
#11 0x00007ff616aede14 clang::Parser::ParseDeclOrFunctionDefInternal(class clang::ParsedAttributes &, class clang::ParsingDeclSpec &, enum clang::AccessSpecifier) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:1158:0
#12 0x00007ff616aed6c2 clang::Parser::ParseDeclarationOrFunctionDefinition(class clang::ParsedAttributes &, class clang::ParsingDeclSpec *, enum clang::AccessSpecifier) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:1172:0
#13 0x00007ff616aed0c8 clang::Parser::ParseExternalDeclaration(class clang::ParsedAttributes &, class clang::ParsingDeclSpec *) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:998:0
#14 0x00007ff616ae7799 clang::Parser::ParseTopLevelDecl(class clang::OpaquePtr<class clang::DeclGroupRef> &, enum clang::Sema::ModuleImportState &) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:727:0
#15 0x00007ff616ae3900 clang::ParseAST(class clang::Sema &, bool, bool) F:\source\llvm-project\clang\lib\Parse\ParseAST.cpp:162:0
#16 0x00007ff613805dd7 clang::ASTFrontendAction::ExecuteAction(void) F:\source\llvm-project\clang\lib\Frontend\FrontendAction.cpp:1141:0
#17 0x00007ff61380578e clang::FrontendAction::Execute(void) F:\source\llvm-project\clang\lib\Frontend\FrontendAction.cpp:1036:0
#18 0x00007ff61378a66a clang::CompilerInstance::ExecuteAction(class clang::FrontendAction &) F:\source\llvm-project\clang\lib\Frontend\CompilerInstance.cpp:1036:0
#19 0x00007ff613a002b8 clang::ExecuteCompilerInvocation(class clang::CompilerInstance *) F:\source\llvm-project\clang\lib\FrontendTool\ExecuteCompilerInvocation.cpp:266:0
#20 0x00007ff60f72e4c8 cc1_main(class llvm::ArrayRef<char const *>, char const *, void *) F:\source\llvm-project\clang\tools\driver\cc1_main.cpp:248:0
#21 0x00007ff60f719135 ExecuteCC1Tool F:\source\llvm-project\clang\tools\driver\driver.cpp:317:0
#22 0x00007ff60f7199ca main F:\source\llvm-project\clang\tools\driver\driver.cpp:388:0
#23 0x00007ff61997b2b9 invoke_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:79:0
#24 0x00007ff61997b15e __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288:0
#25 0x00007ff61997b01e __scrt_common_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:331:0
#26 0x00007ff61997b34e mainCRTStartup D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp:17:0
#27 0x00007ffec5937034 (C:\WINDOWS\System32\KERNEL32.DLL+0x17034)
#28 0x00007ffec6502651 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x52651)

error: command failed with exit status: 3221225477

--

I made some comments on things I saw when looking through the review but didn't spot a smoking gun. I think the issue is possibly the fact that this patch uses static_cast instead of the usual casting infrastructure, but I'm not positive.

Please revert the changes while investigating how to resolve the crashes though.

clang/include/clang/AST/ASTNodeTraverser.h
217–218 ↗(On Diff #431880)

This is an anti-pattern, it should be:

if (const auto *OMPC = dyn_cast<OMPFailClause>(C))
  Visit(OMPC);
clang/lib/Parse/ParseOpenMP.cpp
3823
3829–3831

The coding style has us dropping braces here.

3834

Formatting (the whole patch should be run through clang-format: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting)

clang/lib/Sema/SemaOpenMP.cpp
12663–12665

This looks like another anti-pattern; why is this not:

if (const auto *FC = dyn_cast<OMPFailClause>(C)) {
  ...
13184–13185

Why are you assigning to ErrorLoc and then immediately overwriting it?

13186

Formatting is off here.

Please revert the changes while investigating how to resolve the crashes though.

I just noticed that you needed someone to commit on your behalf for this, so I went ahead and did the revert myself in 69da3b6aead2e7a18a2578aad661d6d36b8d30cf.

Please revert the changes while investigating how to resolve the crashes though.

I just noticed that you needed someone to commit on your behalf for this, so I went ahead and did the revert myself in 69da3b6aead2e7a18a2578aad661d6d36b8d30cf.

There was a follow-up I also needed to revert in 9368bf9023eee0dc6fcfa007e157fe30e1540fcc.

koops added a comment.Jun 8 2022, 8:51 PM
This comment was removed by koops.
koops reopened this revision.Jun 9 2022, 9:00 AM
This revision is now accepted and ready to land.Jun 9 2022, 9:00 AM
koops requested review of this revision.Jun 9 2022, 9:00 AM
koops updated this revision to Diff 435831.Jun 10 2022, 2:27 AM

Changes suggested by aaron.ballman to avoid failures on windows.

Precommit CI on Linux seems to be failing, but I can't quite tell whether the failures are related or not. I suspect they're unrelated, but they're with libomp and other OpenMP tests so it's not fully clear. Can you investigate those?

I tried your patch locally on Windows and it seems to still crash:

FAIL: Clang :: OpenMP/atomic_messages.cpp (1412 of 15074)
******************** TEST 'Clang :: OpenMP/atomic_messages.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -verify=expected,omp45 -fopenmp -fopenmp-version=45 -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 2';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -verify=expected,omp50 -fopenmp -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 3';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -DOMP51 -verify=expected,omp50,omp51 -fopenmp -fopenmp-version=51 -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 5';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -verify=expected,omp45 -fopenmp-simd -fopenmp-version=45 -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 6';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -verify=expected,omp50 -fopenmp-simd -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
: 'RUN: at line 7';   f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe -cc1 -internal-isystem f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include -nostdsysteminc -DOMP51 -verify=expected,omp50,omp51 -fopenmp-simd -fopenmp-version=51 -ferror-limit 150 F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp -Wuninitialized
--
Exit Code: 3221225477

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include" "-nostdsysteminc" "-verify=expected,omp45" "-fopenmp" "-fopenmp-version=45" "-ferror-limit" "150" "F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp" "-Wuninitialized"
$ ":" "RUN: at line 2"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include" "-nostdsysteminc" "-verify=expected,omp50" "-fopenmp" "-ferror-limit" "150" "F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp" "-Wuninitialized"
$ ":" "RUN: at line 3"
$ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "f:\source\llvm-project\llvm\out\build\x64-debug\lib\clang\15.0.0\include" "-nostdsysteminc" "-DOMP51" "-verify=expected,omp50,omp51" "-fopenmp" "-fopenmp-version=51" "-ferror-limit" "150" "F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp" "-Wuninitialized"
# command stderr:
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: f:\\source\\llvm-project\\llvm\\out\\build\\x64-debug\\bin\\clang.exe -cc1 -internal-isystem f:\\source\\llvm-project\\llvm\\out\\build\\x64-debug\\lib\\clang\\15.0.0\\include -nostdsysteminc -DOMP51 -verify=expected,omp50,omp51 -fopenmp -fopenmp-version=51 -ferror-limit 150 F:\\source\\llvm-project\\clang\\test\\OpenMP\\atomic_messages.cpp -Wuninitialized
1.      F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp:968:1: at annotation token
2.      F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp:930:13: parsing function body 'mixed'
3.      F:\source\llvm-project\clang\test\OpenMP\atomic_messages.cpp:930:13: in compound statement ('{}')
 #0 0x00007ff6c208caab clang::OMPClause::getClauseKind(void) const F:\source\llvm-project\clang\include\clang\AST\OpenMPClause.h:83:0
 #1 0x00007ff6c7492153 `anonymous namespace'::OpenMPAtomicFailChecker::checkSubClause F:\source\llvm-project\clang\lib\Sema\SemaOpenMP.cpp:12069:0
 #2 0x00007ff6c741fc3d clang::Sema::ActOnOpenMPAtomicDirective(class llvm::ArrayRef<class clang::OMPClause *>, class clang::Stmt *, class clang::SourceLocation, class clang::SourceLocation) F:\source\llvm-project\clang\lib\Sema\SemaOpenMP.cpp:12652:0
 #3 0x00007ff6c74130f1 clang::Sema::ActOnOpenMPExecutableDirective(enum llvm::omp::Directive, struct clang::DeclarationNameInfo const &, enum llvm::omp::Directive, class llvm::ArrayRef<class clang::OMPClause *>, class clang::Stmt *, class clang::SourceLocation, class clang::SourceLocation) F:\source\llvm-project\clang\lib\Sema\SemaOpenMP.cpp:6172:0
 #4 0x00007ff6c64e6d78 clang::Parser::ParseOpenMPDeclarativeOrExecutableDirective(enum clang::Parser::ParsedStmtContext, bool) F:\source\llvm-project\clang\lib\Parse\ParseOpenMP.cpp:2932:0
 #5 0x00007ff6c649b8e2 clang::Parser::ParseStatementOrDeclarationAfterAttributes(class llvm::SmallVector<class clang::Stmt *, 32> &, enum clang::Parser::ParsedStmtContext, class clang::SourceLocation *, class clang::ParsedAttributes &) F:\source\llvm-project\clang\lib\Parse\ParseStmt.cpp:413:0
 #6 0x00007ff6c649a572 clang::Parser::ParseStatementOrDeclaration(class llvm::SmallVector<class clang::Stmt *, 32> &, enum clang::Parser::ParsedStmtContext, class clang::SourceLocation *) F:\source\llvm-project\clang\lib\Parse\ParseStmt.cpp:115:0
 #7 0x00007ff6c649def9 clang::Parser::ParseCompoundStatementBody(bool) F:\source\llvm-project\clang\lib\Parse\ParseStmt.cpp:1111:0
 #8 0x00007ff6c64a4dc2 clang::Parser::ParseFunctionStatementBody(class clang::Decl *, class clang::Parser::ParseScope &) F:\source\llvm-project\clang\lib\Parse\ParseStmt.cpp:2382:0
 #9 0x00007ff6c63a642c clang::Parser::ParseFunctionDefinition(class clang::ParsingDeclarator &, struct clang::Parser::ParsedTemplateInfo const &, class clang::Parser::LateParsedAttrList *) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:1407:0
#10 0x00007ff6c63ee9ac clang::Parser::ParseDeclGroup(class clang::ParsingDeclSpec &, enum clang::DeclaratorContext, class clang::SourceLocation *, struct clang::Parser::ForRangeInit *) F:\source\llvm-project\clang\lib\Parse\ParseDecl.cpp:2098:0
#11 0x00007ff6c63a5054 clang::Parser::ParseDeclOrFunctionDefInternal(class clang::ParsedAttributes &, class clang::ParsingDeclSpec &, enum clang::AccessSpecifier) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:1158:0
#12 0x00007ff6c63a4902 clang::Parser::ParseDeclarationOrFunctionDefinition(class clang::ParsedAttributes &, class clang::ParsingDeclSpec *, enum clang::AccessSpecifier) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:1172:0
#13 0x00007ff6c63a4308 clang::Parser::ParseExternalDeclaration(class clang::ParsedAttributes &, class clang::ParsingDeclSpec *) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:998:0
#14 0x00007ff6c639e9d9 clang::Parser::ParseTopLevelDecl(class clang::OpaquePtr<class clang::DeclGroupRef> &, enum clang::Sema::ModuleImportState &) F:\source\llvm-project\clang\lib\Parse\Parser.cpp:727:0
#15 0x00007ff6c639ab40 clang::ParseAST(class clang::Sema &, bool, bool) F:\source\llvm-project\clang\lib\Parse\ParseAST.cpp:162:0
#16 0x00007ff6c308ae97 clang::ASTFrontendAction::ExecuteAction(void) F:\source\llvm-project\clang\lib\Frontend\FrontendAction.cpp:1137:0
#17 0x00007ff6c308a84e clang::FrontendAction::Execute(void) F:\source\llvm-project\clang\lib\Frontend\FrontendAction.cpp:1032:0
#18 0x00007ff6c300f59a clang::CompilerInstance::ExecuteAction(class clang::FrontendAction &) F:\source\llvm-project\clang\lib\Frontend\CompilerInstance.cpp:1033:0
#19 0x00007ff6c32856d8 clang::ExecuteCompilerInvocation(class clang::CompilerInstance *) F:\source\llvm-project\clang\lib\FrontendTool\ExecuteCompilerInvocation.cpp:266:0
#20 0x00007ff6bef3da48 cc1_main(class llvm::ArrayRef<char const *>, char const *, void *) F:\source\llvm-project\clang\tools\driver\cc1_main.cpp:248:0
#21 0x00007ff6bef26e65 ExecuteCC1Tool F:\source\llvm-project\clang\tools\driver\driver.cpp:317:0
#22 0x00007ff6bef276fa clang_main(int, char **) F:\source\llvm-project\clang\tools\driver\driver.cpp:388:0
#23 0x00007ff6bef6859c main F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\tools\driver\clang-driver.cpp:11:0
#24 0x00007ff6c92600b9 invoke_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:79:0
#25 0x00007ff6c925ff5e __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288:0
#26 0x00007ff6c925fe1e __scrt_common_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:331:0
#27 0x00007ff6c926014e mainCRTStartup D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp:17:0
#28 0x00007ff826487034 (C:\WINDOWS\System32\KERNEL32.DLL+0x17034)
#29 0x00007ff828362651 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x52651)

error: command failed with exit status: 3221225477

--

********************
koops updated this revision to Diff 548103.Aug 8 2023, 1:37 AM
koops updated this revision to Diff 548494.Aug 9 2023, 12:38 AM
koops added a reviewer: thakis.
This comment was removed by koops.
koops updated this revision to Diff 548534.EditedAug 9 2023, 2:46 AM

Pulling in latest changes.
The patch uploaded 2 hours back was a wrong one (meant for another feature) and has been unrolled back with this patch.

koops updated this revision to Diff 548886.Aug 9 2023, 11:51 PM
This comment was removed by koops.
ABataev added inline comments.
clang/include/clang/AST/OpenMPClause.h
2320–2321

Why need tail-allocation here for constant number of attributes? They can be represented simply as data members

2322

OMPClause *MemoryOrderClause = nullptr;

2361

Remove this

2367

Remove this

clang/lib/Basic/OpenMPKinds.cpp
451

Not recommended to use defaul switch, use all enums explicitly

clang/lib/Parse/ParseOpenMP.cpp
3309

Looks like this is the wrong place for this clause, better to parse like OMPC_default clause

3808

auto *FailClause = cast<OMPFailClause>(Clause)

clang/lib/Sema/SemaOpenMP.cpp
12638

Needs class description

12639–12640

Why protected?

12645–12646

Should be private member with the default initializer

12665
const auto *FC = dyn_cast<OMPFailClause>(C)
if (!FC)
  continue;

to reduce structural complexity

12681
  1. default: cases are not repcommended
  2. formatting
clang/lib/Serialization/ASTReader.cpp
10685–10693

This is strange you need this

10694–10695

do not use default:

clang/lib/Serialization/ASTWriter.cpp
6626–6627

Remove this

koops updated this revision to Diff 557749.Oct 18 2023, 3:09 AM

Removed the tail-allocation.
Other changes suggested in the previous review.

ABataev added inline comments.Oct 18 2023, 3:23 AM
clang/include/clang/AST/ASTNodeTraverser.h
217–220 ↗(On Diff #557749)

Why do you need special logic here?

clang/include/clang/AST/OpenMPClause.h
2320

That's not the best decision, better to make this kind of clause a base class for these new class.

2373

Formatting

2429

No need for const_ prefix in the name

koops updated this revision to Diff 557808.Oct 20 2023, 3:28 AM
  1. In class OMPFailClause, the was a duplication in the storage of parameter to the fail clause because the parameter was stored as FailParameterKind and MemoryOrderClause (FailMemoryOrderClause). There was a possibility of these two being out of sync along with confusion to the reader of the code. Hence storing FailMemoryOrderClause only. The FailParameterKind (of type ClauseKind) is now obtained from the FailMemoryOrderClause when needed.
  2. In Visit(const OMPClause *C) there is a check for if (const auto *OMPC = dyn_cast<OMPFailClause>(C)). This is mainly done to visit the parameter of the FailClause which is a memory Order Clause.
koops updated this revision to Diff 557820.Oct 20 2023, 11:10 PM

Correcting a git-clang-format error.

koops updated this revision to Diff 557920.Oct 27 2023, 11:47 AM

Removing the printing of parameter of FailClause. This needed a special if statement in Visit(const OMPClause *C) , which is a generalized Visit for Clauses.

ABataev added inline comments.Oct 28 2023, 7:53 AM
clang/include/clang/AST/OpenMPClause.h
2525

I don't like the idea of a reference to another clause here, it may lead to many issues with serialization/deserialization, use-after-free etc. Better to keep a flag (kind of memory order clause?) that there is associated clause here and then just find it in the list of clauses, where required.

koops updated this revision to Diff 557933.Oct 30 2023, 1:53 AM

Replacing the storing of "OMPClause *FailMemoryOrderClause" with "OpenMPClauseKind FailParameter" in class OMPFailClause.

ABataev added inline comments.Oct 30 2023, 7:20 AM
clang/include/clang/AST/OpenMPClause.h
2543

Remove this empty line

2611–2617

make this function private

clang/include/clang/Basic/DiagnosticSemaKinds.td
10971

Unused error message?

10972

I thin k this must be handled by parse..clause function already

koops added inline comments.Oct 30 2023, 9:35 AM
clang/include/clang/AST/OpenMPClause.h
2611–2617

OMPFailClause::initFailClause() is called from ParseOpenMPFailClause().

clang/include/clang/Basic/DiagnosticSemaKinds.td
10972

This is not being handled by the ParseOpenMPFailClause() or any other Parse function. It is done only in the SemaOpenMP.cpp.

ABataev added inline comments.Oct 30 2023, 9:44 AM
clang/lib/Parse/ParseOpenMP.cpp
3328–3332

If there is a second instance of this clause, it should be reported here

3831

That's a bad place, it should be done in Sema.

koops updated this revision to Diff 557992.Nov 2 2023, 10:42 AM

Moving OMC_fail to use ParseOpenMPSimpleClause instead of ParseOpenMPClause.
Other changes suggested by previous review.

ABataev added inline comments.Nov 2 2023, 11:21 AM
clang/include/clang/AST/OpenMPClause.h
2561

Unused?

2590–2597

You don't need all these function, this clause is a simple one, it does not require special create function, just new (...) OMPFailClause works in all cases

clang/lib/Sema/SemaOpenMP.cpp
12639

You already has all the info for the fail clause, why do you need to scan through the list of all available clauses to find this fail clause?

12644

Use С++ style comments.

12693

You can perform all required checks for the fail clause here

17704

Just new (Context) OMPFailClause(StartLoc, EndLoc);

clang/lib/Serialization/ASTReader.cpp
10280

Just С = new (Context) OMPFailClause();

koops updated this revision to Diff 558007.Nov 4 2023, 2:52 AM
  1. Removing the Create methods from OMPFailClause class.
  2. checkFailClauseParameters removed and the checking is now done in ActOnOpenMPAtomicDirective() itself.
ABataev added inline comments.Nov 7 2023, 5:45 AM
clang/lib/Parse/ParseOpenMP.cpp
3838–3839
clang/lib/Sema/SemaOpenMP.cpp
12691
12696–12700
12700–12702

No need for braces here

koops updated this revision to Diff 558039.Nov 7 2023, 10:25 AM
ABataev added inline comments.Nov 7 2023, 10:27 AM
clang/lib/Sema/SemaOpenMP.cpp
12694–12695

Remove extra parens

12696–12698

Remove extra parens, LLVM does not use this style

koops updated this revision to Diff 558040.Nov 7 2023, 11:08 AM

Removing extra brackets.

ABataev accepted this revision.Nov 7 2023, 11:16 AM

LG with a nit

clang/lib/Sema/SemaOpenMP.cpp
12693

Same, remove parens

This revision is now accepted and ready to land.Nov 7 2023, 11:16 AM
koops updated this revision to Diff 558041.Nov 7 2023, 12:14 PM
This revision was landed with ongoing or failed builds.Nov 7 2023, 2:58 PM
This revision was automatically updated to reflect the committed changes.
uabelho added inline comments.
clang/lib/Basic/OpenMPKinds.cpp
444

Hi @koops ,
clang complains on this patch with

../../clang/lib/Basic/OpenMPKinds.cpp:444:13: error: 97 enumeration values not handled in switch: 'OMPC_adjust_args', 'OMPC_affinity', 'OMPC_align'... [-Werror,-Wswitch]
    switch (CK) {
            ^~
1 error generated.
qiucf added a subscriber: qiucf.Nov 8 2023, 12:01 AM

Hi, it seems rG086b65340cca2648a2a91a0a47d28c7d9bafd1e5 causes build failure:

llvm-project/clang/lib/Basic/OpenMPKinds.cpp:444:13: error: 97 enumeration values not handled in switch: 'OMPC_adjust_args', 'OMPC_affinity', 'OMPC_align'... [-Werror,-Wswitch]
    switch (CK) {
            ^~
1 error generated.
koops added a comment.Nov 8 2023, 12:08 AM

Only on ppc64 this error is being seen because it is expecting a default in the switch statement:
OpenMPClauseKind CK = ...
switch(CK)

This is similar to what has been added in OMPClauseWithPreInit::get() {
switch(C->getClauseKind()) {
case ...
....
default:
}
I am about to make this change.

koops reopened this revision.Nov 8 2023, 12:19 AM
This revision is now accepted and ready to land.Nov 8 2023, 12:19 AM
koops updated this revision to Diff 558051.Nov 8 2023, 12:24 AM

To avoid build error in ppc64 adding a "default" to switch statement. This is similar to the way it is currently handled in

 OMPClauseWithPreInit::get() {
switch(C->getClauseKind()) {
case ...
....
default:
}
koops requested review of this revision.Nov 8 2023, 12:28 AM

To avoid build error in ppc64 adding a "default" to switch statement.

I don't think it has anything particular to do with ppc64.

I see the warning/error when i compile with clang 15 on a RHEL7 machine.

If you compile without -Werror you just get a warning.

Btw, a bit confusing (to me at least) to reopen this review since it's already landed.

clang/lib/Basic/OpenMPKinds.cpp
443

Adding "default:" here silences the warning.
But looks like @ABataev commented on something similar earlier and said that's not recommended?

koops added inline comments.Nov 8 2023, 3:11 AM
clang/lib/Basic/OpenMPKinds.cpp
443

I agree but, I am

443

Alexey was referring to

the -Wswitch warning won’t fire when new elements are added to that enumeration. To help avoid adding these kinds of defaults, Clang has the warning -Wcovered-switch-default which is off by default but turned on when building LLVM with a version of Clang that supports the warning

in the standards document. In this particular case this has been taken care of by using the macro

#define OPENMP_ATOMIC_FAIL_MODIFIER(Name)
case OMPC_##Name:

which covers all cases added to the enum.

koops updated this revision to Diff 558056.EditedNov 8 2023, 7:42 AM

After the reverting of changes, putting the default in the switch case as discussed in the comments earlier.

After the reverting of changes, putting the default in the switch case as discussed in the comments earlier.

No, do not put the default: switch, instead you need explicitly list all other enumeric values, like

case 1:
case 2:
  break;
clang/include/clang/AST/OpenMPClause.h
2555

Same here, remove default: and explicitly list all possible values

koops added a comment.EditedNov 8 2023, 7:57 AM

In clang/lib/AST/OpenMPClause.cpp,

OMPClauseWithPreInit::get(const OMPClause *C) {
switch(C->getClauseKind()) {
case OMPC_schedule:
....
default:
    break;
}

It is not possible to list down all possible `OpenMPClauseKind`` types in the switch. Hence they have used a default even in other methods. I am following the same logic.

In clang/lib/AST/OpenMPClause.cpp,

OMPClauseWithPreInit::get(const OMPClause *C) {
switch(C->getClauseKind()) {
case OMPC_schedule:
....
default:
    break;
}

It is not possible to list down all possible `OpenMPClauseKind`` types in the switch. Hence they have used a default even in other methods. I am following the same logic.

Why not possible?

koops updated this revision to Diff 558076.Nov 10 2023, 9:05 PM

Removing default from switch statements.

ABataev added inline comments.Nov 13 2023, 3:52 AM
clang/include/clang/AST/OpenMPClause.h
2554

I think all these cases are unexpected and must be terminated with llvm_unreachable

koops updated this revision to Diff 558112.Nov 16 2023, 8:57 AM
  1. Added a check for the fail parameter before the instance of OMPFailClause is created in ActOnOpenMPFailClause. An error in fail parameter like #pragma omp atomic compare fail(capture) was creating a wrong instance of OMPFailClause with OMPC_unknown as parameter.
  2. In g++ 12.3 and spec, fail(acq_rel) and fail(release) are not permitted. Removed them from being supported as parameters to the fail clause.
  3. Checking the parameters to fail clause into a single routine clang::checkFailParameter.
ABataev added inline comments.Nov 16 2023, 12:53 PM
clang/include/clang/AST/OpenMPClause.h
2516

No way for extern functions. Declare in include/clang/Basic/OpenMPKinds.h

2544–2545
clang/lib/Sema/SemaOpenMP.cpp
17707–17714

Move it to lib/Basic/OpenMPKinds.cpp

17721
koops updated this revision to Diff 558120.Nov 17 2023, 1:19 AM

Moving the declaration and definition of checkFailClauseParameter to include/clang/Basic/OpenMPKinds.h & lib/Basic/OpenMPKinds.cpp respectively.
2 other minor changes suggested by Alexey.

This revision is now accepted and ready to land.Nov 17 2023, 3:32 AM

FWIW, this appears to have broken some build bots: https://lab.llvm.org/buildbot/#/builders/121/builds/36635 -- can you revert and investigate (or fix-forward quickly)?

koops updated this revision to Diff 558146.Nov 22 2023, 8:19 AM

Adding libFrontendOpenMP.so as a dependent library when building libclangBasic.so. Shared libraries were not built and tested by default, hence build and failed when checking on ppc64le.

ABataev added inline comments.Nov 22 2023, 8:30 AM
clang/lib/Basic/CMakeLists.txt
4

What requires this new dependency?

koops requested review of this revision.Nov 22 2023, 8:51 AM
koops added inline comments.
clang/lib/Basic/CMakeLists.txt
4

When cmake uses -DBUILD_SHARED_LIBS=1 shared libraries are built instead of archive libraries in lib/ directory.
Then, I was getting this error:
Error in linking libclangBasic.so.18git because getOpenMPClauseName() is not defined.

While linking the .o files to form libFrontendOpenMP.so, OMP.cpp.o is also included which has the function definition of getOpenMPClauseName(). Hence there was a need to have libFrontendOpenMP.so in the link line of libclangBasic.so.18git.

koops added inline comments.Nov 22 2023, 8:54 AM
clang/lib/Basic/CMakeLists.txt
4

The error Error in linking libclangBasic.so.18git because getOpenMPClauseName() is not defined. was seen on ppc64le and can be reproduced on other platforms using -DBUILD_SHARED_LIBS=1 in the cmake command line.

This revision is now accepted and ready to land.Nov 22 2023, 9:00 AM
koops updated this revision to Diff 558158.Nov 23 2023, 12:24 AM

No changes from my side. pre-commit builds failed while applying patch. Changes in flang/lib/Semantics/check-omp-structure.cpp in the master caused a conflict. This patch upload is to resolve the conflict.

koops updated this revision to Diff 558165.Nov 23 2023, 11:29 PM
koops added a comment.Nov 24 2023, 6:41 AM

Can someone please commit this change to the master branch?

koops closed this revision.Dec 14 2023, 10:31 AM