[OpenMP] Introduce new file wrapper class for runtime

[jlpeyton]

Summary

Introduce new kmp_safe_file_t class with RAII semantics for file open/close. It is essentially a wrapper around the C-style FILE* object. This also unifies the way we error report if a file can't be opened.

[jdoerfert]

Really nice cleanup. My only request is more documentation for new code.

openmp/runtime/src/kmp.h

4075

Please include doxygen documentation, at least for non trivial methods and the class. Maybe use "RAII" in the class name to indicate the functionality and make it clear there is no "close" needed at use sites.

[jlpeyton]

Updated with documentation. Class is now kmp_safe_raii_file_t

[jdoerfert]

Thanks!

LGTM, update the commit message wrt. class name.

openmp/runtime/src/kmp.h

 int __kmp_execute_tasks_oncore(kmp_info_t *thread, kmp_int32 gtid,
                                kmp_flag_oncore *flag, int final_spin,
                                int *thread_finished,
 #if USE_ITT_BUILD
                                void *itt_sync_obj,
 #endif /* USE_ITT_BUILD */
                                kmp_int32 is_constrained);
 
+/// This class safely opens and closes a C-style FILE* object using RAII
+/// semantics. There are also methods which allow using stdout or stderr as
+/// the underlying FILE* object. With the implicit conversion operator to
+/// FILE*, an object with this type can be used in any function which takes
+/// a FILE* object e.g., fprintf().
+/// No close method is needed at use sites.
+class kmp_safe_raii_file_t {
+  FILE *f;
+
+  void close() {
+    if (f && f != stdout && f != stderr) {
+      fclose(f);
+      f = nullptr;
+    }
+  }
+
+public:
+  kmp_safe_raii_file_t() : f(nullptr) {}
+  kmp_safe_raii_file_t(const char *filename, const char *mode,
+                       const char *env_var = nullptr)
+      : f(nullptr) {
+    open(filename, mode, env_var);
+  }
+  ~kmp_safe_raii_file_t() { close(); }
+
+  /// Open filename using mode. This is automatically closed in the destructor.
+  /// The env_var parameter indicates the environment variable the filename
+  /// came from if != nullptr.
+  void open(const char *filename, const char *mode,
+            const char *env_var = nullptr) {
+    KMP_ASSERT(!f);
+    f = fopen(filename, mode);
+    if (!f) {
+      int code = errno;
+      if (env_var) {
+        __kmp_fatal(KMP_MSG(CantOpenFileForReading, filename), KMP_ERR(code),
+                    KMP_HNT(CheckEnvVar, env_var, filename), __kmp_msg_null);
+      } else {
+        __kmp_fatal(KMP_MSG(CantOpenFileForReading, filename), KMP_ERR(code),
+                    __kmp_msg_null);
+      }
+    }
+  }
+  /// Set the FILE* object to stdout and output there
+  /// No open call should happen before this call.
+  void set_stdout() {
+    KMP_ASSERT(!f);
+    f = stdout;
+  }
+  /// Set the FILE* object to stderr and output there
+  /// No open call should happen before this call.
+  void set_stderr() {
+    KMP_ASSERT(!f);
+    f = stderr;
+  }
+  operator bool() { return bool(f); }
+  operator FILE *() { return f; }
+};
+
 #endif /* KMP_H */

openmp/runtime/src/kmp_affinity.cpp

if (depth < 0) {
         if (msg_id != kmp_i18n_null) {
           KMP_INFORM(AffStrParseFilename, "KMP_AFFINITY",
                      __kmp_i18n_catgets(msg_id), "/proc/cpuinfo");
         } else {
           KMP_INFORM(AffParseFilename, "KMP_AFFINITY", "/proc/cpuinfo");
         }
       }
 
-      FILE *f = fopen("/proc/cpuinfo", "r");
-      if (f == NULL) {
-        msg_id = kmp_i18n_str_CantOpenCpuinfo;
-      } else {
-        file_name = "/proc/cpuinfo";
-        depth =
-            __kmp_affinity_create_cpuinfo_map(&address2os, &line, &msg_id, f);
-        fclose(f);
-        if (depth == 0) {
-          KMP_EXIT_AFF_NONE;
-        }
-      }
-    }
+      kmp_safe_raii_file_t f("/proc/cpuinfo", "r");
+      depth = __kmp_affinity_create_cpuinfo_map(&address2os, &line, &msg_id, f);
 
 #endif /* KMP_OS_LINUX */
 
 #if KMP_GROUP_AFFINITY
 
     if ((depth < 0) && (__kmp_num_proc_groups > 1)) {
       if (__kmp_affinity_verbose) {
         KMP_INFORM(AffWindowsProcGroupMap, "KMP_AFFINITY");
if (depth < 0) {
       KMP_FATAL(MsgExiting, __kmp_i18n_catgets(msg_id));
     }
   }
 
 #endif /* KMP_ARCH_X86 || KMP_ARCH_X86_64 */
 
   else if (__kmp_affinity_top_method == affinity_top_method_cpuinfo) {
     const char *filename;
+    const char *env_var = nullptr;
     if (__kmp_cpuinfo_file != NULL) {
       filename = __kmp_cpuinfo_file;
+      env_var = "KMP_CPUINFO_FILE";
     } else {
       filename = "/proc/cpuinfo";
     }
 
     if (__kmp_affinity_verbose) {
       KMP_INFORM(AffParseFilename, "KMP_AFFINITY", filename);
     }
 
-    FILE *f = fopen(filename, "r");
-    if (f == NULL) {
-      int code = errno;
-      if (__kmp_cpuinfo_file != NULL) {
-        __kmp_fatal(KMP_MSG(CantOpenFileForReading, filename), KMP_ERR(code),
-                    KMP_HNT(NameComesFrom_CPUINFO_FILE), __kmp_msg_null);
-      } else {
-        __kmp_fatal(KMP_MSG(CantOpenFileForReading, filename), KMP_ERR(code),
-                    __kmp_msg_null);
-      }
-    }
+    kmp_safe_raii_file_t f(filename, "r", env_var);
     int line = 0;
     depth = __kmp_affinity_create_cpuinfo_map(&address2os, &line, &msg_id, f);
-    fclose(f);
     if (depth < 0) {
       KMP_ASSERT(msg_id != kmp_i18n_null);
       if (line > 0) {
         KMP_FATAL(FileLineMsgExiting, filename, line,
                   __kmp_i18n_catgets(msg_id));
       } else {
         KMP_FATAL(FileMsgExiting, filename, __kmp_i18n_catgets(msg_id));
       }

openmp/runtime/src/kmp_lock.cpp

static void __kmp_accumulate_speculative_stats(kmp_adaptive_lock_info_t *lck) {
 
   __kmp_release_bootstrap_lock(&chain_lock);
 }
 
 static float percent(kmp_uint32 count, kmp_uint32 total) {
   return (total == 0) ? 0.0 : (100.0 * count) / total;
 }
 
-static FILE *__kmp_open_stats_file() {
-  if (strcmp(__kmp_speculative_statsfile, "-") == 0)
-    return stdout;
-
-  size_t buffLen = KMP_STRLEN(__kmp_speculative_statsfile) + 20;
-  char buffer[buffLen];
-  KMP_SNPRINTF(&buffer[0], buffLen, __kmp_speculative_statsfile,
-               (kmp_int32)getpid());
-  FILE *result = fopen(&buffer[0], "w");
-
-  // Maybe we should issue a warning here...
-  return result ? result : stdout;
-}
-
 void __kmp_print_speculative_stats() {
   kmp_adaptive_lock_statistics_t total = destroyedStats;
   kmp_adaptive_lock_info_t *lck;
 
   for (lck = liveLocks.stats.next; lck != &liveLocks; lck = lck->stats.next) {
     __kmp_add_stats(&total, lck);
   }
   kmp_adaptive_lock_statistics_t *t = &total;
   kmp_uint32 totalSections =
       t->nonSpeculativeAcquires + t->successfulSpeculations;
   kmp_uint32 totalSpeculations = t->successfulSpeculations +
                                  t->hardFailedSpeculations +
                                  t->softFailedSpeculations;
   if (totalSections <= 0)
     return;
 
-  FILE *statsFile = __kmp_open_stats_file();
+  kmp_safe_raii_file_t statsFile;
+  if (strcmp(__kmp_speculative_statsfile, "-") == 0) {
+    statsFile.set_stdout();
+  } else {
+    size_t buffLen = KMP_STRLEN(__kmp_speculative_statsfile) + 20;
+    char buffer[buffLen];
+    KMP_SNPRINTF(&buffer[0], buffLen, __kmp_speculative_statsfile,
+                 (kmp_int32)getpid());
+    statsFile.open(buffer, "w");
+  }
 
   fprintf(statsFile, "Speculative lock statistics (all approximate!)\n");
   fprintf(statsFile, " Lock parameters: \n"
                      "   max_soft_retries               : %10d\n"
                      "   max_badness                    : %10d\n",
           __kmp_adaptive_backoff_params.max_soft_retries,
           __kmp_adaptive_backoff_params.max_badness);
   fprintf(statsFile, " Non-speculative acquire attempts : %10d\n",
fprintf(statsFile, " Successes                        : %10d (%5.1f%%)\n",
           t->successfulSpeculations,
           percent(t->successfulSpeculations, totalSpeculations));
   fprintf(statsFile, " Soft failures                    : %10d (%5.1f%%)\n",
           t->softFailedSpeculations,
           percent(t->softFailedSpeculations, totalSpeculations));
   fprintf(statsFile, " Hard failures                    : %10d (%5.1f%%)\n",
           t->hardFailedSpeculations,
           percent(t->hardFailedSpeculations, totalSpeculations));
-
-  if (statsFile != stdout)
-    fclose(statsFile);
 }
 
 #define KMP_INC_STAT(lck, stat) (lck->lk.adaptive.stats.stat++)
 #else
 #define KMP_INC_STAT(lck, stat)
 
 #endif // KMP_DEBUG_ADAPTIVE_LOCKS
 

openmp/runtime/src/kmp_stats.cpp

for (it = __kmp_stats_list->begin(); it != __kmp_stats_list->end(); it++) {
     ptr->getPartitionedTimers()->windup();
     ptr->endLife();
   }
 }
 
 void kmp_stats_output_module::printPloticusFile() {
   int i;
   int size = __kmp_stats_list->size();
-  FILE *plotOut = fopen(plotFileName, "w+");
-
+  kmp_safe_raii_file_t plotOut(plotFileName, "w+");
   fprintf(plotOut, "#proc page\n"
                    "   pagesize: 15 10\n"
                    "   scale: 1.0\n\n");
 
   fprintf(plotOut, "#proc getdata\n"
                    "   file: %s\n\n",
           eventsFileName);
 
if (timeStat::logEvent((timer_e)i)) {
                        "   details: rgb(%1.1f,%1.1f,%1.1f)\n\n",
               timeStat::name((timer_e)i), c.r, c.g, c.b);
     }
   }
 
   fprintf(plotOut, "#proc legend\n"
                    "   format: down\n"
                    "   location: max max\n\n");
-  fclose(plotOut);
   return;
 }
 
 static void outputEnvVariable(FILE *statsOut, char const *name) {
   char const *value = getenv(name);
   fprintf(statsOut, "# %s = %s\n", name, value ? value : "*unspecified*");
 }
 
void kmp_stats_output_module::outputStats(const char *heading) {
   // constructors so will take time to create.
   windupExplicitTimers();
 
   statistic allStats[TIMER_LAST];
   statistic totalStats[TIMER_LAST]; /* Synthesized, cross threads versions of
                                        normal timer stats */
   statistic allCounters[COUNTER_LAST];
 
-  FILE *statsOut =
-      !outputFileName.empty() ? fopen(outputFileName.c_str(), "a+") : stderr;
-  if (!statsOut)
-    statsOut = stderr;
+  kmp_safe_raii_file_t statsOut;
+  if (!outputFileName.empty()) {
+    statsOut.open(outputFileName.c_str(), "a+");
+  } else {
+    statsOut.set_stderr();
+  }
 
-  FILE *eventsOut;
+  kmp_safe_raii_file_t eventsOut;
   if (eventPrintingEnabled()) {
-    eventsOut = fopen(eventsFileName, "w+");
+    eventsOut.open(eventsFileName, "w+");
   }
 
   printHeaderInfo(statsOut);
   fprintf(statsOut, "%s\n", heading);
   // Accumulate across threads.
   kmp_stats_list::iterator it;
   for (it = __kmp_stats_list->begin(); it != __kmp_stats_list->end(); it++) {
     int t = (*it)->getGtid();
for (counter_e c = counter_e(0); c < COUNTER_LAST; c = counter_e(c + 1)) {
       if (counter::masterOnly(c) && t != 0)
         continue;
       allCounters[c].addSample((*it)->getCounter(c)->getValue());
     }
   }
 
   if (eventPrintingEnabled()) {
     printPloticusFile();
-    fclose(eventsOut);
   }
 
   fprintf(statsOut, "Aggregate for all threads\n");
   printTimerStats(statsOut, &allStats[0], &totalStats[0]);
   fprintf(statsOut, "\n");
   printCounterStats(statsOut, &allCounters[0]);
-
-  if (statsOut != stderr)
-    fclose(statsOut);
 }
 
 /* *************  exported C functions ************** */
 
 // no name mangling for these functions, we want the c files to be able to get
 // at these functions
 extern "C" {