[OpenMP][libomptarget] rework of fatal error reporting

[AlexEichenberger]

Summary

Removed the function that used a lock and varargs
Used the same mechanism as for debug messages

[gtbercea]

LG unless other reviewers have objections.

[Hahnfeld]

I don't understand this: In the last review you said that we need the complicated FatalMessage and now you are reverting that?!?

[AlexEichenberger]

• format

[AlexEichenberger]

In D51226#1212685, @Hahnfeld wrote:

I don't understand this: In the last review you said that we need the complicated FatalMessage and now you are reverting that?!?

I am reimplementing this because it needs an include that may be fragile on some platforms (D51219)

using the #def, I can avoid getting a lock as the whole printing is done in one fprintf

[Hahnfeld]

In D51226#1212699, @AlexEichenberger wrote:

In D51226#1212685, @Hahnfeld wrote:

I don't understand this: In the last review you said that we need the complicated FatalMessage and now you are reverting that?!?

I am reimplementing this because it needs an include that may be fragile on some platforms (D51219)

Can you please elaborate on "fragile"? It's in the standard, we can just use that include!

using the #def, I can avoid getting a lock as the whole printing is done in one fprintf

(I suggested this back in https://reviews.llvm.org/D50522#1199469)

[RaviNarayanaswamy]

libomptarget/src/interface.cpp

58

What scenario would result in this.

[AlexEichenberger]

Alexey was not happy about adding an include, and we already used the define approach for the debug messages. So we reuse the same functionality that we already used for debug.
Agreed that Johnas suggested it, and given the problem with the include, Alexey urged me to change the code to what we have here.

If you prefer the old code with the added include, we can go back to that. Just trying to make you guys happy.

[Hahnfeld]

@ABataev what's wrong with cstdarg?

In the end I don't have a strong opinion here (just a general dislike of too many nasty macros), but in the end it's hard to review a change without any justifaction.

libomptarget/src/private.h

53

If anything please use the do { ... } while (0) logic, that's actually a good thing for macros: https://stackoverflow.com/a/1067238

[AlexEichenberger]

libomptarget/src/interface.cpp

58

Ravi,

its probably unlikely, just wanted it here to cover all cases.

[RaviNarayanaswamy]

libomptarget/src/interface.cpp

59

The reason I asked is because you have different way of reporting for tgt_disabled and tgt_default.

[ABataev]

In D51226#1212784, @Hahnfeld wrote:

@ABataev what's wrong with cstdarg?

In the end I don't have a strong opinion here (just a general dislike of too many nasty macros), but in the end it's hard to review a change without any justifaction.

Just wanted to exclude some extra dependencies from the code. If you're ok with it, I'm fine too.

[AlexEichenberger]

libomptarget/src/interface.cpp

59

ok, will make default the same way as disabled

[AlexEichenberger]

• put fatal message for default tgt test

[Hahnfeld]

As long as we don't need to change back to the current way of doing things next week, I think I'm also fine with the macro if you think it's better. Please address the inline comment about do { ... } while (0) before committing.

libomptarget/src/private.h

53

Ping!

[AlexEichenberger]

• added do while(1) for macros

[Hahnfeld]

No, that's completely different from what I asked.

[Hahnfeld]

libomptarget/src/private.h

53

do { ... } while (0) wasn't a typo, please see the link.

• Please remove the outer compound statement ({ ... }).
• Change while (1) to while (0). The idea is not to make that an infinite loop (which it is not in this case because of the exit()) but to require a semicolon after "invoking" the macro.
• Hence remove the final ;, that needs to be at the call site.

[AlexEichenberger]

• Merge branch 'unpatched-master' into unpatched-master-target-offload-envvar
• while (0) edit

libomptarget/src/private.h

53

changed

[Hahnfeld]

LGTM. One very minor comment inline, I leave it up to you whether you want to make that change - it's more important that we unbreak the build.

libomptarget/src/private.h

51

You could probably put _str directly into the string as in FATAL_MESSAGE, but I don't think this makes a difference.

[Hahnfeld]

libomptarget/src/interface.cpp

23

Fun fact: We include stdarg.h here. I'll remove this as it shouldn't be needed anymore.

[Hahnfeld]

libomptarget/src/interface.cpp

23

Done in r341563.

libomptarget/src/interface.cpp

 #include <omptarget.h>
 
 #include "device.h"
 #include "private.h"
 #include "rtl.h"
 
 #include <cassert>
 #include <cstdlib>
 #include <stdarg.h>
 #include <mutex>
 
 // Store target policy (disabled, mandatory, default)
 kmp_target_offload_kind_t TargetOffloadPolicy = tgt_default;
 std::mutex TargetOffloadMtx;
 
 ////////////////////////////////////////////////////////////////////////////////
 /// manage the success or failure of a target constuct
static int IsOffloadDisabled() {
   if (TargetOffloadPolicy == tgt_default) HandleDefaultTargetOffload();
   return TargetOffloadPolicy == tgt_disabled;
 }
 
 static void HandleTargetOutcome(bool success) {
   switch (TargetOffloadPolicy) {
     case tgt_disabled:
       if (success) {
-        FatalMessage(1, "expected no offloading while offloading is disabled");
+        FATAL_MESSAGE0(1, "expected no offloading while offloading is disabled");
       }
       break;
     case tgt_default:
-      DP("Should never reach this test with target offload set to default\n");
-      assert(false);
+        FATAL_MESSAGE0(1, "default offloading policy must switched to " 
+            "mandatory or disabled");
       break;
     case tgt_mandatory:
       if (!success) {
-        FatalMessage(1, "failure of target construct while offloading is mandatory");
+        FATAL_MESSAGE0(1, "failure of target construct while offloading is mandatory");
       }
       break;
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 /// adds a target shared library to the target execution image
 EXTERN void __tgt_register_lib(__tgt_bin_desc *desc) {

libomptarget/src/omptarget.cpp

 #include <cassert>
 #include <vector>
 #include <mutex>
 
 #ifdef OMPTARGET_DEBUG
 int DebugLevel = 0;
 #endif // OMPTARGET_DEBUG
 
-////////////////////////////////////////////////////////////////////////////////
-/// support for fatal messages
-
-// mutex
-std::mutex LibomptargetPrintMtx;
-
-void FatalMessage(const int errorNum, const char *fmt, ...) {
-  va_list args;
-  va_start(args, fmt);
-  LibomptargetPrintMtx.lock();
-  fprintf(stderr, "Libomptarget error %d:", errorNum);
-  vfprintf(stderr, fmt, args);
-  fprintf(stderr, "\n");
-  LibomptargetPrintMtx.unlock();
-  va_end(args);
-  exit(1);
-}
 
 
 /* All begin addresses for partially mapped structs must be 8-aligned in order
  * to ensure proper alignment of members. E.g.
  *
  * struct S {
  *   int a;   // 4-aligned
  *   int b;   // 4-aligned

libomptarget/src/private.h

 enum kmp_target_offload_kind {
   tgt_disabled = 0,
   tgt_default = 1,
   tgt_mandatory = 2
 };
 typedef enum kmp_target_offload_kind kmp_target_offload_kind_t;
 extern kmp_target_offload_kind_t TargetOffloadPolicy;
 
+////////////////////////////////////////////////////////////////////////////////
+// implemtation for fatal messages
+////////////////////////////////////////////////////////////////////////////////
+
+#define FATAL_MESSAGE0(_num, _str)                                    \
+  do {                                                                \
+    fprintf(stderr, "Libomptarget fatal error %d: %s\n", _num, _str); \
+    exit(1);                                                          \
+  } while (0)
+
+#define FATAL_MESSAGE(_num, _str, ...)                              \
+  do {                                                              \
+    fprintf(stderr, "Libomptarget fatal error %d:" _str "\n", _num, \
+            __VA_ARGS__);                                           \
+    exit(1);                                                        \
+  } while (0)
+
 // Implemented in libomp, they are called from within __tgt_* functions.
 #ifdef __cplusplus
 extern "C" {
 #endif
 // functions that extract info from libomp; keep in sync
 int omp_get_default_device(void) __attribute__((weak));
 int32_t __kmpc_omp_taskwait(void *loc_ref, int32_t gtid) __attribute__((weak));
 int __kmpc_get_target_offload(void) __attribute__((weak));
 #ifdef __cplusplus
 }
 #endif
 
-void FatalMessage(const int errorNum, const char *fmt, ...);
-
 #ifdef OMPTARGET_DEBUG
 extern int DebugLevel;
 
 #define DP(...) \
   do { \
     if (DebugLevel > 0) { \
       DEBUGP("Libomptarget", __VA_ARGS__); \
     } \
   } while (false)
 #else // OMPTARGET_DEBUG
 #define DP(...) {}
 #endif // OMPTARGET_DEBUG
 
 #endif