[OpenMP] [Flang] Resolved Issue llvm#76121: Implemented Check for Unhandled Arguments in __kmpc_fork_call_if #82221
Conversation
Root cause: Segmentation fault is caused by null pointer dereference inside the __kmpc_fork_call_if function at https://github.com/llvm/llvm-project/blob/main/openmp/runtime/src/z_Linux_asm.S#L1186 . __kmpc_fork_call_if is missing case to handle argc=0 . Fix: Added a check inside the __kmp_invoke_microtask function to handle the case when argc is 0.
chandankds
requested review from
kiranchandramohan,
jdoerfert,
shiltian,
harishch4,
NimishMishra and
kiranktp
|
Ping for review @jdoerfert @kiranchandramohan |
|
Looks good to me. Can you add a test? |
| @@ -0,0 +1,27 @@ | |||
| !RUN: %flang %s -o %t -fopenmp && %t | FileCheck %s | |||
There was a problem hiding this comment.
I don't think this test can work here. It needs to be moved into openmp'/runtime/test, but it currently lacks of the infrastructure to handle it in libomp. Check openmp/libomptarget/test/lit.site.cfg.in on how we handle Fortran test cases in libomptarget.
There was a problem hiding this comment.
Thank you for suggestion, I'll look into it.
There was a problem hiding this comment.
Hi @shiltian I've added the test, could you please review.
|
It looks like the replacement of |
Added testcase
chandankds
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
fixed formatting
|
@shiltian Thanks for sharing your insights, I understand we would like to avoid creating dependency between flang and libomp. Gcc fortan compiler does't reproduce this issue. I have added a test case which exposes the issue and validates the fix in c, please review the same. |
|
@shiltian @jdoerfert humble request for review. |
| @@ -0,0 +1,22 @@ | |||
| // RUN: %libomp-compile -Wno-implicit-function-declaration && %t | FileCheck %s | |||
There was a problem hiding this comment.
Why do we need -Wno-implicit-function-declaration?
There was a problem hiding this comment.
I think LLVM has moved to C99 standards recently because of which I was getting error: call to undeclared function '__kmpc_fork_call_if'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] so i've added this flag suppresses this error.
There was a problem hiding this comment.
Then you need to add declaration of __kmpc_fork_call_if
There was a problem hiding this comment.
I had tried to declare void __kmpc_fork_call_if(ident_t *loc, kmp_int32 argc, kmpc_micro microtask, kmp_int32 cond, void *args) but it contains certain types which are not being found.
There was a problem hiding this comment.
You can define those types on your own. See openmp/runtime/test/atomic/kmp_atomic_cas.c for more details. Essentially you will just need ident_t.
There was a problem hiding this comment.
Thank you for sharing your insights, I've defined required types and removed -Wno-implicit-function-declaration
| // Call __kmpc_fork_call_if | ||
| __kmpc_fork_call_if(NULL, 0, microtask, cond, NULL); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
format please, empty line EOF.
openmp/runtime/src/z_Linux_asm.S
Outdated
| @@ -1195,6 +1198,7 @@ KMP_LABEL(kmp_1_exit): | |||
| cmovnsq (%r11), %rdx // p_argv[0] -> %rdx (store 3rd parm to pkfn) | |||
| #endif // KMP_MIC | |||
|
|
|||
| KMP_LABEL(kmp_no_args): | |||
Root cause: Segmentation fault is caused by null pointer dereference inside the __kmpc_fork_call_if function at https://github.com/llvm/llvm-project/blob/main/openmp/runtime/src/z_Linux_asm.S#L1186 . __kmpc_fork_call_if is missing case to handle argc=0 .
Fix: Added a check inside the __kmp_invoke_microtask function to handle the case when argc is 0.