Page MenuHomePhabricator

[OpenMP] Use `abort` not `error` for fatal runtime exceptions
ClosedPublic

Authored by jdoerfert on Jul 16 2020, 9:55 AM.

Details

Summary

See PR46515 for the rational but generally, we want to *really* abort
not gracefully shut down.

Diff Detail

Unit TestsFailed

TimeTest
5,010 mslinux > libomp.env::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic
1,020 mslinux > libomp.worksharing/for::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

Event Timeline

jdoerfert created this revision.Jul 16 2020, 9:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2020, 9:55 AM
ABataev accepted this revision.Jul 16 2020, 9:56 AM
ABataev added a subscriber: ABataev.

LG

This revision is now accepted and ready to land.Jul 16 2020, 9:56 AM

Are you sure abort gets fprintf flushed?

abort() does not flush stdout, but here we are printing to stderr which is not buffered so there is no need to flush.

grokos accepted this revision.Jul 16 2020, 11:24 AM

abort() does not flush stdout, but here we are printing to stderr which is not buffered so there is no need to flush.

Clear. Then everything is good. Let us have this on both master and 11.x

Add flush

You missed the conversation above. stderr is not buffered and flush is not needed.

Add flush

You missed the conversation above. stderr is not buffered and flush is not needed.

I did, will remove it again..

This revision was automatically updated to reflect the committed changes.

stderr is unbuffered by default. Are we sure this behavior is not changed when the output is redirected into a file, as for example during execution in a batch job? What were the worries about having the fflush there?

stderr is unbuffered by default. Are we sure this behavior is not changed when the output is redirected into a file, as for example during execution in a batch job? What were the worries about having the fflush there?

I went with this: https://reviews.llvm.org/D83963#2156612
If you think flush is needed, feel free to add it, it doesn't cost us anything.

jdenny added a subscriber: jdenny.Jul 27 2020, 8:17 AM

This change broke a number of libomptarget tests. Adding --crash to the %libomptarget-run-fail-* definitions in openmp/libomptarget/test/lit.cfg fixes that for me. Any objections?

Any idea why this change broke some tests? And how does --crash fix the problem?

not expects a non-zero exit not a signal. Adding --crash adjusts the expectation.

I would've just pushed the fix, but I was concerned that maybe others weren't seeing this somehow.

Thanks, it makes sense. Can you submit a patch?

I've pushed it to master as 9b4826d18b5f.