[binutils-gdb/binutils-2_43-branch] libctf: clean up hashtab error handling mess

[binutils-gdb/binutils-2_43-branch] libctf: clean up hashtab error handling mess

[Nick Alcock]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e24eb6404a606f6f78e35a3f6f11b57fd2364828

commit e24eb6404a606f6f78e35a3f6f11b57fd2364828
Author: Nick Alcock <nick.alcock@oracle.com>
Date:   Fri Jul 26 21:58:03 2024 +0100

    libctf: clean up hashtab error handling mess
    
    The dict and archive opening code in libctf is somewhat unusual, because
    unlike everything else, it cannot report errors by setting an error on the
    dict, because in case of error there isn't one.  They get passed an error
    integer pointer that is set on error instead.
    
    Inside ctf_bufopen this is implemented by calling ctf_set_open_errno and
    passing it a positive error value.  In turn this means that most things it
    calls (including init_static_types) return zero on success and a *positive*
    ECTF_* or errno value on error.
    
    This trickles down to ctf_dynhash_insert_type, which is used by
    init_static_types to add newly-detected types to the name tables.  This was
    returning the error value it received from a variety of functions without
    alteration.  ctf_dynhash_insert conformed to this contract by returning a
    positive value on error (usually OOM), which is unfortunate for multiple
    reasons:
    
    - ctf_dynset_insert returns a *negative* value
    - ctf_dynhash_insert and ctf_dynset_insert don't take an fp, so the value
      they return is turned into the errno, so it had better be right, callers
      don't just check for != 0 here
    - more or less every single caller of ctf_dyn*_insert in libctf other than
      ctf_dynhash_insert_type (and there are a *lot*, mostly in the
      deduplicator) assumes that ctf_dynhash_insert returns a negative value
      on error, even though it doesn't.  In practice the only possible error is
      OOM, but if OOM does happen we end up with a nonsense error value.
    
    The simplest fix for this seems to be to make ctf_dynhash_insert and
    ctf_dynset_insert conform to the usual interface contract: negative
    values are errors.  This in turn means that ctf_dynhash_insert_type
    needs to change: let's make it consistent too, returning a negative
    value on error, putting the error on the fp in non-negated form.
    
    init_static_types_internal adapts to this by negating the error return from
    ctf_dynhash_insert_type, so the value handed back to ctf_bufopen is still
    positive: the new call site in ctf_track_enumerator does not need to change.
    
    (The existing tests for this reliably detect when I get it wrong.
    I know, because they did.)
    
    libctf/
            * ctf-hash.c (ctf_dynhash_insert): Negate return value.
            (ctf_dynhash_insert_type): Set de-negated error on the dict:
            return negated error.
            * ctf-open.c (init_static_types_internal): Adapt to this change.

Diff:
---
 libctf/ctf-hash.c |  7 +++++--
 libctf/ctf-open.c | 20 +++++++++++---------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/libctf/ctf-hash.c b/libctf/ctf-hash.c
index 7f291e3ac27..cd7473419cb 100644
--- a/libctf/ctf-hash.c
+++ b/libctf/ctf-hash.c
@@ -256,7 +256,7 @@ ctf_dynhash_insert (ctf_dynhash_t *hp, void *key, void *value)
 			     key_free, value_free);
 
   if (!slot)
-    return errno;
+    return -errno;
 
   /* Keep track of the owner, so that the del function can get at the key_free
      and value_free functions.  Only do this if one of those functions is set:
@@ -786,7 +786,7 @@ ctf_dynhash_insert_type (ctf_dict_t *fp, ctf_dynhash_t *hp, uint32_t type,
     return EINVAL;
 
   if ((str = ctf_strptr_validate (fp, name)) == NULL)
-    return ctf_errno (fp);
+    return ctf_errno (fp) * -1;
 
   if (str[0] == '\0')
     return 0;		   /* Just ignore empty strings on behalf of caller.  */
@@ -795,6 +795,9 @@ ctf_dynhash_insert_type (ctf_dict_t *fp, ctf_dynhash_t *hp, uint32_t type,
 				 (void *) (ptrdiff_t) type)) == 0)
     return 0;
 
+  /* ctf_dynhash_insert returns a negative error value: negate it for
+     ctf_set_errno.  */
+  ctf_set_errno (fp, err * -1);
   return err;
 }
 
diff --git a/libctf/ctf-open.c b/libctf/ctf-open.c
index 3f624987f90..942a53a8f2c 100644
--- a/libctf/ctf-open.c
+++ b/libctf/ctf-open.c
@@ -681,6 +681,8 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
    the latest supported representation in the process, if needed, and if this
    recension of libctf supports upgrading.
 
+   Returns zero on success and a *positive* ECTF_* or errno value on error.
+
    This is a wrapper to simplify memory allocation on error in the _internal
    function that does all the actual work.  */
 
@@ -886,7 +888,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
 					       LCTF_INDEX_TO_TYPE (fp, id, child),
 					       tp->ctt_name);
 		if (err != 0)
-		  return err;
+		  return err * -1;
 	      }
 	    break;
 	  }
@@ -905,7 +907,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
 					 LCTF_INDEX_TO_TYPE (fp, id, child),
 					 tp->ctt_name);
 	  if (err != 0)
-	    return err;
+	    return err * -1;
 	  break;
 
 	case CTF_K_STRUCT:
@@ -920,7 +922,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
 					 tp->ctt_name);
 
 	  if (err != 0)
-	    return err;
+	    return err * -1;
 
 	  break;
 
@@ -936,7 +938,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
 					 tp->ctt_name);
 
 	  if (err != 0)
-	    return err;
+	    return err * -1;
 	  break;
 
 	case CTF_K_ENUM:
@@ -949,14 +951,14 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
 					   tp->ctt_name);
 
 	    if (err != 0)
-	      return err;
+	      return err * -1;
 
 	    /* Remember all enums for later rescanning.  */
 
 	    err = ctf_dynset_insert (all_enums, (void *) (ptrdiff_t)
 				     LCTF_INDEX_TO_TYPE (fp, id, child));
 	    if (err != 0)
-	      return err;
+	      return err * -1;
 	    break;
 	  }
 
@@ -968,7 +970,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
 					 LCTF_INDEX_TO_TYPE (fp, id, child),
 					 tp->ctt_name);
 	  if (err != 0)
-	    return err;
+	    return err * -1;
 	  break;
 
 	case CTF_K_FORWARD:
@@ -985,7 +987,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
 		err = ctf_dynhash_insert_type (fp, h, LCTF_INDEX_TO_TYPE (fp, id, child),
 					       tp->ctt_name);
 		if (err != 0)
-		  return err;
+		  return err * -1;
 	      }
 	    break;
 	  }
@@ -1010,7 +1012,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
 					 LCTF_INDEX_TO_TYPE (fp, id, child),
 					 tp->ctt_name);
 	  if (err != 0)
-	    return err;
+	    return err * -1;
 	  break;
 	default:
 	  ctf_err_warn (fp, 0, ECTF_CORRUPT,