Add and use memdup(), MEMDUP(), instead of its pattern

[alejandro-colomar]

See also: #1292 (comment)

Cc: @Karlson2k

---

Revisions:

v2

• Remove n parameter in MEMDUP(). I think we won't need it, as it will likely be 1 everywhere we might want to duplicate memory. The use of this API seems to be duplicating a pointer to structure that uses static storage in non-reentrant functions.
• malloc(3) doesn't need the ?: 1 trick. That's just for realloc(3).

$ git rd 
1:  67923ed0 ! 1:  eaf1e24f lib/string/strdup/: memdup(), MEMDUP(): Add APIs
    @@ lib/string/strdup/memdup.h (new)
     +#include "attr.h"
     +
     +
    -+#define MEMDUP(p, n, T)  _Generic(p, T *: (T *) memdup(p, n * sizeof(T)))
    ++#define MEMDUP(p, T)  _Generic(p, T *: (T *) memdup(p, sizeof(T)))
     +
     +
     +ATTR_MALLOC(free)
    @@ lib/string/strdup/memdup.h (new)
     +{
     +  void  *new;
     +
    -+  new = malloc(size ?: 1);
    ++  new = malloc(size);
     +  if (new == NULL)
     +          return NULL;
     +
2:  7bdb4e11 = 2:  62e91ee4 lib/utmp.c: get_current_utmp(): Don't exit(3) from library code
3:  d9925423 ! 3:  5239d2bb lib/utmp.c: get_current_utmp(): Use MEMDUP() instead of its pattern
    @@ lib/utmp.c: get_current_utmp(void)
     -                  memcpy(ret, ut, sizeof (*ret));
     -  }
     +  if (NULL != ut)
    -+          ut = MEMDUP(ut, 1, struct utmpx);
    ++          ut = MEMDUP(ut, struct utmpx);
      
        endutxent();
      

v2b

• Rebase

$ git rd 
1:  eaf1e24f = 1:  894bbbf8 lib/string/strdup/: memdup(), MEMDUP(): Add APIs
2:  62e91ee4 ! 2:  3775e2a3 lib/utmp.c: get_current_utmp(): Don't exit(3) from library code
    @@ lib/utmp.c
      #include "alloc/x/xcalloc.h"
     -#include "alloc/x/xmalloc.h"
      #include "sizeof.h"
    + #include "string/strchr/strnul.h"
      #include "string/strcmp/streq.h"
    - #include "string/strcmp/strprefix.h"
     @@ lib/utmp.c: get_current_utmp(void)
        }
      
3:  5239d2bb ! 3:  e6e4c808 lib/utmp.c: get_current_utmp(): Use MEMDUP() instead of its pattern
    @@ lib/utmp.c
     -#include "alloc/malloc.h"
      #include "alloc/x/xcalloc.h"
      #include "sizeof.h"
    - #include "string/strcmp/streq.h"
    + #include "string/strchr/strnul.h"
    +@@
      #include "string/strcmp/strprefix.h"
      #include "string/strcpy/strncpy.h"
      #include "string/strcpy/strtcpy.h"

v3

• Rebase

$ git range-diff 894bbbf8add6^..gh/memdup shadow/master..memdup 
1:  894bbbf8 = 1:  f8276a62 lib/string/strdup/: memdup(), MEMDUP(): Add APIs
2:  3775e2a3 ! 2:  fb5f9fc9 lib/utmp.c: get_current_utmp(): Don't exit(3) from library code
    @@ lib/utmp.c
      #include "sizeof.h"
      #include "string/strchr/strnul.h"
      #include "string/strcmp/streq.h"
    -@@ lib/utmp.c: get_current_utmp(void)
    -   }
    - 
    +@@ lib/utmp.c: get_current_utmp(pid_t main_pid)
        if (NULL != ut) {
    --          ret = XMALLOC(1, struct utmpx);
    --          memcpy (ret, ut, sizeof (*ret));
    -+          ret = MALLOC(1, struct utmpx);
    -+          if (ret != NULL)
    -+                  memcpy(ret, ut, sizeof (*ret));
    +           struct utmpx  *ut_copy;
    + 
    +-          ut_copy = XMALLOC(1, struct utmpx);
    +-          memcpy(ut_copy, ut, sizeof(*ut));
    ++          ut_copy = MALLOC(1, struct utmpx);
    ++          if (ut_copy != NULL)
    ++                  memcpy(ut_copy, ut, sizeof(*ut));
    +           ut = ut_copy;
        }
      
    -   endutxent();
3:  e6e4c808 ! 3:  903035d4 lib/utmp.c: get_current_utmp(): Use MEMDUP() instead of its pattern
    @@ lib/utmp.c
      #include "string/strdup/xstrdup.h"
      #include "string/strdup/xstrndup.h"
      
    -@@ lib/utmp.c: static /*@null@*/ /*@only@*/struct utmpx *
    - get_current_utmp(void)
    - {
    -   struct utmpx  *ut;
    --  struct utmpx  *ret = NULL;
    - 
    -   setutxent();
    - 
    -@@ lib/utmp.c: get_current_utmp(void)
    +@@ lib/utmp.c: get_current_utmp(pid_t main_pid)
                }
        }
      
     -  if (NULL != ut) {
    --          ret = MALLOC(1, struct utmpx);
    --          if (ret != NULL)
    --                  memcpy(ret, ut, sizeof (*ret));
    +-          struct utmpx  *ut_copy;
    +-
    +-          ut_copy = MALLOC(1, struct utmpx);
    +-          if (ut_copy != NULL)
    +-                  memcpy(ut_copy, ut, sizeof(*ut));
    +-          ut = ut_copy;
     -  }
     +  if (NULL != ut)
     +          ut = MEMDUP(ut, struct utmpx);
      
        endutxent();
      
    --  return ret;
    -+  return ut;
    - }
    - 
    - 

v4

• Rebase

$ git rd 
1:  f8276a62 = 1:  af04fb6d lib/string/strdup/: memdup(), MEMDUP(): Add APIs
2:  fb5f9fc9 ! 2:  9fe1fa3a lib/utmp.c: get_current_utmp(): Don't exit(3) from library code
    @@ lib/utmp.c
     +#include "alloc/malloc.h"
      #include "alloc/x/xcalloc.h"
     -#include "alloc/x/xmalloc.h"
    + #include "attr.h"
      #include "sizeof.h"
      #include "string/strchr/strnul.h"
    - #include "string/strcmp/streq.h"
     @@ lib/utmp.c: get_current_utmp(pid_t main_pid)
        if (NULL != ut) {
                struct utmpx  *ut_copy;
3:  903035d4 ! 3:  ab7bb521 lib/utmp.c: get_current_utmp(): Use MEMDUP() instead of its pattern
    @@ lib/utmp.c
      
     -#include "alloc/malloc.h"
      #include "alloc/x/xcalloc.h"
    + #include "attr.h"
      #include "sizeof.h"
    - #include "string/strchr/strnul.h"
     @@
      #include "string/strcmp/strprefix.h"
      #include "string/strcpy/strncpy.h"
    @@ lib/utmp.c
      #include "string/strdup/xstrndup.h"
      
     @@ lib/utmp.c: get_current_utmp(pid_t main_pid)
    -           }
    -   }
    +   if (NULL == ut)
    +           ut = ut_by_pid ?: ut_by_line;
      
     -  if (NULL != ut) {
     -          struct utmpx  *ut_copy;
    @@ lib/utmp.c: get_current_utmp(pid_t main_pid)
     +  if (NULL != ut)
     +          ut = MEMDUP(ut, struct utmpx);
      
    -   endutxent();
    - 
    +   free(ut_by_line);
    +   free(ut_by_pid);

v4b

• Rebase

$ git rd 
1:  af04fb6d = 1:  a1ebec8c lib/string/strdup/: memdup(), MEMDUP(): Add APIs
2:  9fe1fa3a = 2:  5fc012b4 lib/utmp.c: get_current_utmp(): Don't exit(3) from library code
3:  ab7bb521 = 3:  06c665ad lib/utmp.c: get_current_utmp(): Use MEMDUP() instead of its pattern

[Karlson2k]

I still recommend adding attribute malloc (in form without destructor). When used without -fno-strict-aliasing (like this project) it improves optimisations.

[Karlson2k]

See https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute

Attribute malloc indicates that a function is malloc-like, i.e., that the pointer P returned by the function cannot alias any other pointer valid when the function returns, and moreover no pointers to valid objects occur in any storage addressed by P. In addition, GCC predicts that a function with the attribute returns non-null in most cases.

Independently, the form of the attribute with one or two arguments associates deallocator as a suitable deallocation function for pointers returned from the malloc-like function. ptr-index denotes the positional argument to which when the pointer is passed in calls to deallocator has the effect of deallocating it.

For example, besides stating that the functions return pointers that do not alias any others, the following declarations make fclose a suitable deallocator for pointers returned from all functions except popen, and pclose as the only suitable deallocator for pointers returned from popen. The deallocator functions must be declared before they can be referenced in the attribute.

int fclose (FILE*);
int pclose (FILE*);

__attribute__ ((malloc, malloc (fclose, 1)))
  FILE* fdopen (int, const char*);
__attribute__ ((malloc, malloc (fclose, 1)))
  FILE* fopen (const char*, const char*);
__attribute__ ((malloc, malloc (fclose, 1)))
  FILE* fmemopen(void *, size_t, const char *);
__attribute__ ((malloc, malloc (pclose, 1)))
  FILE* popen (const char*, const char*);
__attribute__ ((malloc, malloc (fclose, 1)))
  FILE* tmpfile (void);

[alejandro-colomar]

I still recommend adding attribute malloc (in form without destructor). When used without -fno-strict-aliasing (like this project) it improves optimisations.

I think by having this function declared inline the compiler should be able to realize about any possible optimizations, since it transparently sees the malloc(3) call.

I don't like using attributes for telling the compiler that it can optimize. That's error-prone. I prefer allowing it to see more of the code, so it can reason by itself.

[Karlson2k]

I think by having this function declared inline the compiler should be able to realize about any possible optimizations, since it transparently sees the malloc(3) call.

Then you do not need current ATTR_MALLOC(free) as compiler may see malloc() call.
It looks inconsistent that function must have two attributes, but uses only one of them.

[alejandro-colomar]

I think by having this function declared inline the compiler should be able to realize about any possible optimizations, since it transparently sees the malloc(3) call.

Then you do not need current ATTR_MALLOC(free) as compiler may see malloc() call. It looks inconsistent that function must have two attributes, but uses only one of them.

The ATTR_MALLOC(free) attribute is not for optimization, but for safety. It allows the compiler to warn when we forget to free(3). There, redundancy increases safety.