Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang] Fix seg fault CodeGenAction::executeAction() #78269

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

ergawy
Copy link
Member

@ergawy ergawy commented Jan 16, 2024

If generateLLVMIR() fails, we still continue using the module we failed to generate which causes a seg fault if LLVM code-gen failed for some reason or another. This commit fixes this issue.

@llvmbot llvmbot added flang:driver flang Flang issues not falling into any other category labels Jan 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 16, 2024

@llvm/pr-subscribers-flang-driver

Author: Kareem Ergawy (ergawy)

Changes

If generateLLVMIR() fails, we still continue using the module we failed to generate which causes a seg fault if LLVM code-gen failed for some reason or another. This commit fixes this issue.


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

1 Files Affected:

  • (modified) flang/lib/Frontend/FrontendActions.cpp (+5)
diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index 74e3992d5ab62ba..65c4df7388f97b2 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -1202,6 +1202,11 @@ void CodeGenAction::executeAction() {
   if (!llvmModule)
     generateLLVMIR();
 
+  // If generating the LLVM module failed, abort! No need for further error
+  // reporting since generateLLVMIR() does this already.
+  if (!llvmModule)
+    return;
+
   // Set the triple based on the targetmachine (this comes compiler invocation
   // and the command-line target option if specified, or the default if not
   // given on the command-line).

@ergawy
Copy link
Member Author

ergawy commented Jan 16, 2024

Note that I came across this issue while testing:

flang-new -c -fopenmp target.f90

For this input:

subroutine omp_target_enter_nowait
   integer :: a(1024)
   !$omp target enter data map(to: a) nowait
end subroutine omp_target_enter_nowait

Now, I get this error (without the seg fault):

error: loc("/home/kaergawy/playground/tmp/target.f90":3:10): LLVM Translation failed for operation: omp.target_enter_data
error: failed to create the LLVM module

nowait seems to be the culprit. Removing it resolves the error.

I am currently looking into this issue and will hopefully open a separate PR.

Edit: this happens because nowait is not supported in CodeGen yet. I forgot about this. Will open a PR with this commit for slightly better error reporting.

Copy link
Contributor

@banach-space banach-space left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

It would be good to add a diagnostic and a test - perhaps trying compiling a "broken" MLIR file?

@ergawy
Copy link
Member Author

ergawy commented Jan 16, 2024

Thanks!

It would be good to add a diagnostic and a test - perhaps trying compiling a "broken" MLIR file?

Thanks for the reply @banach-space. I can try to add a unit test (I see there is none for the flang CodeGenAction class so far. I think this will be better self-contained and easier to understand what is going on than a lit test. WDYT?

@banach-space
Copy link
Contributor

Thanks!
It would be good to add a diagnostic and a test - perhaps trying compiling a "broken" MLIR file?

Thanks for the reply @banach-space. I can try to add a unit test (I see there is none for the flang CodeGenAction class so far. I think this will be better self-contained and easier to understand what is going on than a lit test. WDYT?

+1 :)

@ergawy
Copy link
Member Author

ergawy commented Jan 17, 2024

Thanks!

It would be good to add a diagnostic and a test - perhaps trying compiling a "broken" MLIR file?

Done!

If `generateLLVMIR()` fails, we still continue using the module we
failed to generate which causes a seg fault if LLVM code-gen failed for
some reason or another. This commit fixes this issue.
@ergawy
Copy link
Member Author

ergawy commented Jan 18, 2024

Ping! :)

Copy link
Contributor

@banach-space banach-space left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clever - love this approach :)

LGTM, thank you!

@ergawy ergawy merged commit 99cae9a into llvm:main Jan 18, 2024
3 of 4 checks passed
ergawy added a commit to ergawy/llvm-project that referenced this pull request Jan 19, 2024
ergawy added a commit that referenced this pull request Jan 19, 2024
…" (#78667)

This reverts commit 99cae9a.

Temporarily until I reproduce and fix a linker issue:
```
FAILED: tools/flang/unittests/Frontend/FlangFrontendTests
...
/usr/bin/ld: tools/flang/unittests/Frontend/CMakeFiles/FlangFrontendTests.dir/CodeGenActionTest.cpp.o: undefined reference to symbol '_ZN4llvm11LLVMContextC1Ev'
/usr/bin/ld: /work1/omp-nightly/build/git/trunk18.0/build/llvm-project/lib/libLLVMCore.so.18git: error adding symbols: DSO missing from command line
```
ergawy added a commit to ergawy/llvm-project that referenced this pull request Jan 19, 2024
llvm#78269)" (llvm#78667)"

This reverts commit 4fc7506.

Re-applies PR llvm#78269 and adds LLVM and MLIR dependencies that were
missed in the PR. The missing libs were: `LLVMCore` & `MLIRIR`.
ergawy added a commit that referenced this pull request Jan 19, 2024
If `generateLLVMIR()` fails, we still continue using the module we
failed to generate which causes a seg fault if LLVM code-gen failed for
some reason or another. This commit fixes this issue.

Re-applies PR #78269 and adds LLVM and MLIR dependencies that were
missed in the PR. The missing libs were: `LLVMCore` & `MLIRIR`.

This reverts commit 4fc7506.
ergawy added a commit that referenced this pull request Jan 19, 2024
Provides some context for failing to generate LLVM IR for `target
enter|exit|update` directives when `nowait` is provided. This is
directly helpful for flang users since they would get this error message
if they tried to use `nowait`. Before that we had a very generic
message.

This is a follow-up to #78269,
please only review the latest commit (the one with the same commit
message as the PR title).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:driver flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants