Pause recording errors instead of clearing the error stack

An internal capability that's similar to the H5E_BEGIN_TRY / H5E_END_TRY macros in H5Epublic.h, but more efficient since we can avoid pushing errors on the stack entirely (and those macros use public API routines). This capability (and other techniques) can be used to remove use of H5E_clear_stack() and H5E_BEGIN_TRY / H5E_END_TRY within library routines. We want to remove H5E_clear_stack() because it can trigger calls to the H5I interface from within the H5E code, which creates a great deal of complexity for threadsafe code. And we want to remove H5E_BEGIN_TRY / H5E_END_TRY's because they make public API calls from within the library code. Also some other minor tidying in routines related to removing the use of H5E_clear_stack() and H5E_BEGIN_TRY / H5E_END_TRY from H5Fint.c Signed-off-by: Quincey Koziol <[email protected]>
qkoziol · May 10, 2024 · 1a20767 · 1a20767
1 parent f17ca56
commit 1a20767
Show file tree

Hide file tree

Showing 9 changed files with 287 additions and 190 deletions.
diff --git a/bin/format_source b/bin/format_source
@@ -25,10 +25,12 @@ find . \( -type d -path ./config -prune -and -not -path ./config \) \
         -name H5LTanalyze.c \
         -or -name H5LTparse.c \
         -or -name H5LTparse.h \
-        -or -name H5Epubgen.h \
+        -or -name H5Edefin.h \
         -or -name H5Einit.h \
+        -or -name H5Emajdef.h \
+        -or -name H5Emindef.h \
+        -or -name H5Epubgen.h \
         -or -name H5Eterm.h \
-        -or -name H5Edefin.h \
         -or -name H5version.h \
         -or -name H5overflow.h \
         \) \) \

diff --git a/src/H5E.c b/src/H5E.c
@@ -532,8 +532,11 @@ H5Epush2(hid_t err_stack, const char *file, const char *func, unsigned line, hid
     /* Don't clear the error stack! :-) */
     FUNC_ENTER_API_NOCLEAR(FAIL)
 
-    if (err_stack == H5E_DEFAULT)
-        estack = NULL;
+    /* Check for 'default' error stack */
+    if (err_stack == H5E_DEFAULT) {
+        if (NULL == (estack = H5E__get_my_stack()))
+            HGOTO_ERROR(H5E_ERROR, H5E_CANTGET, FAIL, "can't get the default error stack");
+    }
     else {
         /* Only clear the error stack if it's not the default stack */
         H5E_clear_stack();
@@ -543,35 +546,33 @@ H5Epush2(hid_t err_stack, const char *file, const char *func, unsigned line, hid
             HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a error stack ID");
     } /* end else */
 
-    /* Note that the variable-argument parsing for the format is identical in
-     *      the H5E_printf_stack() routine - correct errors and make changes in both
-     *      places. -QAK
-     */
-
-    /* Format the description */
-    va_start(ap, fmt);
-    va_started = true;
-
-    /* Duplicate string information */
-    if (NULL == (tmp_file = strdup(file)))
-        HGOTO_ERROR(H5E_ERROR, H5E_CANTALLOC, FAIL, "can't duplicate file string");
-    if (NULL == (tmp_func = strdup(func)))
-        HGOTO_ERROR(H5E_ERROR, H5E_CANTALLOC, FAIL, "can't duplicate function string");
-
-    /* Increment refcount on non-library IDs */
-    if (cls_id != H5E_ERR_CLS_g)
-        if (H5I_inc_ref(cls_id, false) < 0)
-            HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment class ID");
-    if (maj_id < H5E_first_maj_id_g || maj_id > H5E_last_maj_id_g)
-        if (H5I_inc_ref(maj_id, false) < 0)
-            HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment major error ID");
-    if (min_id < H5E_first_min_id_g || min_id > H5E_last_min_id_g)
-        if (H5I_inc_ref(min_id, false) < 0)
-            HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment minor error ID");
-
-    /* Push the error on the stack */
-    if (H5E__push_stack(estack, true, tmp_file, tmp_func, line, cls_id, maj_id, min_id, fmt, &ap) < 0)
-        HGOTO_ERROR(H5E_ERROR, H5E_CANTSET, FAIL, "can't push error on stack");
+    /* Check if error reporting is paused for this stack */
+    if (!estack->paused) {
+        /* Start the variable-argument parsing */
+        va_start(ap, fmt);
+        va_started = true;
+
+        /* Duplicate string information */
+        if (NULL == (tmp_file = strdup(file)))
+            HGOTO_ERROR(H5E_ERROR, H5E_CANTALLOC, FAIL, "can't duplicate file string");
+        if (NULL == (tmp_func = strdup(func)))
+            HGOTO_ERROR(H5E_ERROR, H5E_CANTALLOC, FAIL, "can't duplicate function string");
+
+        /* Increment refcount on non-library IDs */
+        if (cls_id != H5E_ERR_CLS_g)
+            if (H5I_inc_ref(cls_id, false) < 0)
+                HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment class ID");
+        if (maj_id < H5E_first_maj_id_g || maj_id > H5E_last_maj_id_g)
+            if (H5I_inc_ref(maj_id, false) < 0)
+                HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment major error ID");
+        if (min_id < H5E_first_min_id_g || min_id > H5E_last_min_id_g)
+            if (H5I_inc_ref(min_id, false) < 0)
+                HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment minor error ID");
+
+        /* Push the error on the stack */
+        if (H5E__push_stack(estack, true, tmp_file, tmp_func, line, cls_id, maj_id, min_id, fmt, &ap) < 0)
+            HGOTO_ERROR(H5E_ERROR, H5E_CANTSET, FAIL, "can't push error on stack");
+    }
 
 done:
     if (va_started)

diff --git a/src/H5Edeprec.c b/src/H5Edeprec.c
@@ -182,30 +182,38 @@ H5Eget_minor(H5E_minor_t min)
 herr_t
 H5Epush1(const char *file, const char *func, unsigned line, H5E_major_t maj, H5E_minor_t min, const char *str)
 {
+    H5E_stack_t *estack;             /* Pointer to error stack to modify */
     const char *tmp_file;            /* Copy of the file name */
     const char *tmp_func;            /* Copy of the function name */
     herr_t      ret_value = SUCCEED; /* Return value */
 
     /* Don't clear the error stack! :-) */
     FUNC_ENTER_API_NOCLEAR(FAIL)
 
-    /* Duplicate string information */
-    if (NULL == (tmp_file = strdup(file)))
-        HGOTO_ERROR(H5E_ERROR, H5E_CANTALLOC, FAIL, "can't duplicate file string");
-    if (NULL == (tmp_func = strdup(func)))
-        HGOTO_ERROR(H5E_ERROR, H5E_CANTALLOC, FAIL, "can't duplicate function string");
-
-    /* Increment refcount on non-library IDs */
-    if (maj < H5E_first_maj_id_g || maj > H5E_last_maj_id_g)
-        if (H5I_inc_ref(maj, false) < 0)
-            HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment major error ID");
-    if (min < H5E_first_min_id_g || min > H5E_last_min_id_g)
-        if (H5I_inc_ref(min, false) < 0)
-            HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment minor error ID");
-
-    /* Push the error on the default error stack */
-    if (H5E__push_stack(NULL, true, tmp_file, tmp_func, line, H5E_ERR_CLS_g, maj, min, str, NULL) < 0)
-        HGOTO_ERROR(H5E_ERROR, H5E_CANTSET, FAIL, "can't push error on stack");
+    /* Get the 'default' error stack */
+    if (NULL == (estack = H5E__get_my_stack()))
+        HGOTO_ERROR(H5E_ERROR, H5E_CANTGET, FAIL, "can't get the default error stack");
+
+    /* Check if error reporting is paused for this stack */
+    if (!estack->paused) {
+        /* Duplicate string information */
+        if (NULL == (tmp_file = strdup(file)))
+            HGOTO_ERROR(H5E_ERROR, H5E_CANTALLOC, FAIL, "can't duplicate file string");
+        if (NULL == (tmp_func = strdup(func)))
+            HGOTO_ERROR(H5E_ERROR, H5E_CANTALLOC, FAIL, "can't duplicate function string");
+
+        /* Increment refcount on non-library IDs */
+        if (maj < H5E_first_maj_id_g || maj > H5E_last_maj_id_g)
+            if (H5I_inc_ref(maj, false) < 0)
+                HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment major error ID");
+        if (min < H5E_first_min_id_g || min > H5E_last_min_id_g)
+            if (H5I_inc_ref(min, false) < 0)
+                HGOTO_ERROR(H5E_ERROR, H5E_CANTINC, FAIL, "can't increment minor error ID");
+
+        /* Push the error on the default error stack */
+        if (H5E__push_stack(estack, true, tmp_file, tmp_func, line, H5E_ERR_CLS_g, maj, min, str, NULL) < 0)
+            HGOTO_ERROR(H5E_ERROR, H5E_CANTSET, FAIL, "can't push error on stack");
+    }
 
 done:
     FUNC_LEAVE_API(ret_value)
@@ -259,6 +267,7 @@ H5Eprint1(FILE *stream)
     /* Don't clear the error stack! :-) */
     FUNC_ENTER_API_NOCLEAR(FAIL)
 
+    /* Get the 'default' error stack */
     if (NULL == (estack = H5E__get_my_stack()))
         HGOTO_ERROR(H5E_ERROR, H5E_CANTGET, FAIL, "can't get current error stack");
 

diff --git a/src/H5Eint.c b/src/H5Eint.c
@@ -188,7 +188,6 @@ H5E_init(void)
         HGOTO_ERROR(H5E_ID, H5E_CANTINIT, FAIL, "unable to initialize ID group");
 
 #ifndef H5_HAVE_THREADSAFE
-    H5E_stack_g[0].nused = 0;
     H5E__set_default_auto(H5E_stack_g);
 #endif /* H5_HAVE_THREADSAFE */
 
@@ -317,7 +316,6 @@ H5E__get_stack(void)
         assert(estack);
 
         /* Set the thread-specific info */
-        estack->nused = 0;
         H5E__set_default_auto(estack);
 
         /* (It's not necessary to release this in this API, it is
@@ -821,8 +819,7 @@ H5E__append_stack(H5E_stack_t *dst_stack, const H5E_stack_t *src_stack)
 /*--------------------------------------------------------------------------
  * Function:    H5E__set_default_auto
  *
- * Purpose:     Initialize "automatic" error stack reporting info to library
- *              default
+ * Purpose:     Initialize error stack to library default
  *
  * Return:      SUCCEED/FAIL
  *
@@ -833,6 +830,8 @@ H5E__set_default_auto(H5E_stack_t *stk)
 {
     FUNC_ENTER_PACKAGE_NOERR
 
+    stk->nused = 0;
+
 #ifndef H5_NO_DEPRECATED_SYMBOLS
 #ifdef H5_USE_16_API_DEFAULT
     stk->auto_op.vers = 1;
@@ -848,6 +847,7 @@ H5E__set_default_auto(H5E_stack_t *stk)
 #endif /* H5_NO_DEPRECATED_SYMBOLS */
 
     stk->auto_data = NULL;
+    stk->paused = 0;
 
     FUNC_LEAVE_NOAPI_VOID
 } /* end H5E__set_default_auto() */
@@ -1383,6 +1383,7 @@ herr_t
 H5E_printf_stack(const char *file, const char *func, unsigned line, hid_t maj_id, hid_t min_id,
                  const char *fmt, ...)
 {
+    H5E_stack_t *estack;          /* Pointer to error stack to modify */
     va_list ap;                   /* Varargs info */
     bool    va_started = false;   /* Whether the variable argument list is open */
     herr_t  ret_value  = SUCCEED; /* Return value */
@@ -1401,18 +1402,20 @@ H5E_printf_stack(const char *file, const char *func, unsigned line, hid_t maj_id
     assert(min_id >= H5E_first_min_id_g && min_id <= H5E_last_min_id_g);
     assert(fmt);
 
-    /* Note that the variable-argument parsing for the format is identical in
-     *      the H5Epush2() routine - correct errors and make changes in both
-     *      places. -QAK
-     */
+    /* Get the 'default' error stack */
+    if (NULL == (estack = H5E__get_my_stack()))
+        HGOTO_DONE(FAIL);
 
-    /* Start the variable-argument parsing */
-    va_start(ap, fmt);
-    va_started = true;
+    /* Check if error reporting is paused for this stack */
+    if (!estack->paused) {
+        /* Start the variable-argument parsing */
+        va_start(ap, fmt);
+        va_started = true;
 
-    /* Push the error on the stack */
-    if (H5E__push_stack(NULL, false, file, func, line, H5E_ERR_CLS_g, maj_id, min_id, fmt, &ap) < 0)
-        HGOTO_DONE(FAIL);
+        /* Push the error on the stack */
+        if (H5E__push_stack(estack, false, file, func, line, H5E_ERR_CLS_g, maj_id, min_id, fmt, &ap) < 0)
+            HGOTO_DONE(FAIL);
+    }
 
 done:
     if (va_started)
@@ -1458,11 +1461,6 @@ H5E__push_stack(H5E_stack_t *estack, bool app_entry, const char *file, const cha
     assert(maj_id > 0);
     assert(min_id > 0);
 
-    /* Check for 'default' error stack */
-    if (estack == NULL)
-        if (NULL == (estack = H5E__get_my_stack()))
-            HGOTO_DONE(FAIL);
-
     /*
      * Push the error if there's room.  Otherwise just forget it.
      */
@@ -1688,8 +1686,9 @@ H5E_clear_stack(void)
         HGOTO_ERROR(H5E_ERROR, H5E_CANTGET, FAIL, "can't get current error stack");
 
     /* Empty the error stack */
-    if (H5E__clear_entries(estack, estack->nused) < 0)
-        HGOTO_ERROR(H5E_ERROR, H5E_CANTSET, FAIL, "can't clear error stack");
+    if (estack->nused)
+        if (H5E__clear_entries(estack, estack->nused) < 0)
+            HGOTO_ERROR(H5E_ERROR, H5E_CANTSET, FAIL, "can't clear error stack");
 
 done:
     FUNC_LEAVE_NOAPI(ret_value)
@@ -1791,3 +1790,79 @@ H5E_dump_api_stack(void)
 
     FUNC_LEAVE_NOAPI(ret_value)
 } /* end H5E_dump_api_stack() */
+
+/*-------------------------------------------------------------------------
+ * Function:    H5E_pause_stack
+ *
+ * Purpose:     Pause pushing errors on the default error stack.
+ *
+ *              Generally used via the H5E_PAUSE_ERRORS / H5E_RESUME_ERRORS
+ *              macros.
+ *
+ *              When one needs to temporarily disable recording errors while
+ *              trying something that's likely or expected to fail.  The code
+ *              to try can be nested between these macros like:
+ *
+ *              H5E_PAUSE_ERRORS {
+ *                  ...stuff here that's likely to fail...
+ *              } H5E_RESUME_ERRORS
+ *
+ *              Warning: don't break, return, or longjmp() from the block of
+ *              code or the error reporting won't be properly restored!
+ *
+ * Return:      none
+ *
+ *-------------------------------------------------------------------------
+ */
+void
+H5E_pause_stack(void)
+{
+    H5E_stack_t *estack    = H5E__get_my_stack();
+
+    FUNC_ENTER_NOAPI_NOERR
+
+    assert(estack);
+
+    /* Increment pause counter */
+    estack->paused++;
+
+    FUNC_LEAVE_NOAPI_VOID
+} /* end H5E_pause_stack() */
+
+/*-------------------------------------------------------------------------
+ * Function:    H5E_resume_stack
+ *
+ * Purpose:     Resume pushing errors on the default error stack.
+ *
+ *              Generally used via the H5E_PAUSE_ERRORS / H5E_RESUME_ERRORS
+ *              macros.
+ *
+ *              When one needs to temporarily disable recording errors while
+ *              trying something that's likely or expected to fail.  The code
+ *              to try can be nested between these macros like:
+ *
+ *              H5E_PAUSE_ERRORS {
+ *                  ...stuff here that's likely to fail...
+ *              } H5E_RESUME_ERRORS
+ *
+ *              Warning: don't break, return, or longjmp() from the block of
+ *              code or the error reporting won't be properly restored!
+ *
+ * Return:      none
+ *
+ *-------------------------------------------------------------------------
+ */
+void
+H5E_resume_stack(void)
+{
+    H5E_stack_t *estack = H5E__get_my_stack();
+
+    FUNC_ENTER_NOAPI_NOERR
+
+    assert(estack);
+
+    /* Decrement pause counter */
+    estack->paused--;
+
+    FUNC_LEAVE_NOAPI_VOID
+} /* end H5E_resume_stack() */
diff --git a/src/H5Epkg.h b/src/H5Epkg.h
@@ -114,6 +114,7 @@ typedef struct H5E_stack_t {
     H5E_entry_t   entries[H5E_MAX_ENTRIES]; /* Array of error entries */
     H5E_auto_op_t auto_op;                  /* Operator for 'automatic' error reporting */
     void         *auto_data;                /* Callback data for 'automatic error reporting */
+    unsigned      paused;                   /* Whether error reporting is paused (>0) for this stack */
 } H5E_stack_t;
 
 /*****************************/

diff --git a/src/H5Eprivate.h b/src/H5Eprivate.h
@@ -21,6 +21,23 @@
 /* Private headers needed by this file */
 #include "H5private.h"
 
+/*
+ * When one needs to temporarily disable recording errors while trying
+ * something that's likely or expected to fail.  The code to try can be nested
+ * between these macros like:
+ *
+ *     H5E_PAUSE_ERRORS {
+ *        ...stuff here that's likely to fail...
+ *      } H5E_RESUME_ERRORS
+ *
+ * Warning: don't break, return, or longjmp() from the block of code or
+ *        the error reporting won't be properly restored!
+ *
+ */
+#define H5E_PAUSE_ERRORS H5E_pause_stack();
+
+#define H5E_RESUME_ERRORS H5E_resume_stack();
+
 /*
  * HERROR macro, used to facilitate error reporting between a FUNC_ENTER()
  * and a FUNC_LEAVE() within a function body.  The arguments are the major
@@ -186,5 +203,7 @@ H5_DLL herr_t H5E_printf_stack(const char *file, const char *func, unsigned line
                                hid_t min_idx, const char *fmt, ...) H5_ATTR_FORMAT(printf, 6, 7);
 H5_DLL herr_t H5E_clear_stack(void);
 H5_DLL herr_t H5E_dump_api_stack(void);
+H5_DLL void H5E_pause_stack(void);
+H5_DLL void H5E_resume_stack(void);
 
 #endif /* H5Eprivate_H */
diff --git a/src/H5Epublic.h b/src/H5Epublic.h
@@ -77,11 +77,12 @@ H5_DLLVAR hid_t H5E_ERR_CLS_g;
  * trying something that's likely or expected to fail.  The code to try can
  * be nested between calls to H5Eget_auto() and H5Eset_auto(), but it's
  * easier just to use this macro like:
+ *
  *     H5E_BEGIN_TRY {
  *        ...stuff here that's likely to fail...
  *      } H5E_END_TRY
  *
- * Warning: don't break, return, or longjmp() from the body of the loop or
+ * Warning: don't break, return, or longjmp() from the block of code or
  *        the error reporting won't be properly restored!
  *
  * These two macros still use the old API functions for backward compatibility