Fix REALLOC_ZERO_BYTES_FREES comment

Fix REALLOC_ZERO_BYTES_FREES comment

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

While looking into problems with glibc's documentation of 
malloc/free/etc. I noticed that the comment for REALLOC_ZERO_BYTES_FREES 
was out of date: its reference to "the C standard" refers to ISO C11, 
but that compatibility problem has been fixed in C17. I installed the 
attached commentary fix as obvious.

[-- Attachment #2: 0001-Fix-REALLOC_ZERO_BYTES_FREES-comment-to-match-C17.patch --]
[-- Type: text/x-patch, Size: 1376 bytes --]

From dff9e592b8f74e2e7be015cbee1c0fad3ef96d37 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 11 Apr 2021 14:39:20 -0700
Subject: [PATCH] Fix REALLOC_ZERO_BYTES_FREES comment to match C17

* malloc/malloc.c (REALLOC_ZERO_BYTES_FREES):
Update comment to match current C standard.
---
 malloc/malloc.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 6640385282..0cd3ba78ca 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -346,10 +346,13 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
 #define REVEAL_PTR(ptr)  PROTECT_PTR (&ptr, ptr)
 
 /*
-  REALLOC_ZERO_BYTES_FREES should be set if a call to
-  realloc with zero bytes should be the same as a call to free.
-  This is required by the C standard. Otherwise, since this malloc
-  returns a unique pointer for malloc(0), so does realloc(p, 0).
+  REALLOC_ZERO_BYTES_FREES controls the behavior of realloc (p, 0)
+  when p is nonnull.  If nonzero, realloc (p, 0) should free p and
+  return NULL.  Otherwise, realloc (p, 0) should do the equivalent
+  of freeing p and returning what malloc (0) would return.
+
+  ISO C17 says the behavior is implementation-defined here; glibc
+  follows historical practice and defines it to be nonzero.
 */
 
 #ifndef REALLOC_ZERO_BYTES_FREES
-- 
2.27.0

Re: Fix REALLOC_ZERO_BYTES_FREES comment

[Paul Zimmermann]

       Dear Paul,

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 11 Apr 2021 14:42:22 -0700
> 
> While looking into problems with glibc's documentation of 
> malloc/free/etc. I noticed that the comment for REALLOC_ZERO_BYTES_FREES 
> was out of date: its reference to "the C standard" refers to ISO C11, 
> but that compatibility problem has been fixed in C17. I installed the 
> attached commentary fix as obvious.
> 
> [2:text/x-patch Show Save:0001-Fix-REALLOC_ZERO_BYTES_FREES-comment-to-match-C17.patch (1kB)]

you write "If nonzero, ... should free p .... Otherwise ... should do the
equivalent of freeing p". Is there a difference between "free p" and "do the
equivalent of freeing p"?

Also "ISO C17 says the behavior is implementation-defined here": I guess you
mean the "default value of REALLOC_ZERO_BYTES_FREES", since for me the
behavior is what is described above.

Paul

Re: Fix REALLOC_ZERO_BYTES_FREES comment

[Florian Weimer]

* Paul Zimmermann:

>        Dear Paul,
>
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Sun, 11 Apr 2021 14:42:22 -0700
>> 
>> While looking into problems with glibc's documentation of 
>> malloc/free/etc. I noticed that the comment for REALLOC_ZERO_BYTES_FREES 
>> was out of date: its reference to "the C standard" refers to ISO C11, 
>> but that compatibility problem has been fixed in C17. I installed the 
>> attached commentary fix as obvious.
>> 
>> [2:text/x-patch Show Save:0001-Fix-REALLOC_ZERO_BYTES_FREES-comment-to-match-C17.patch (1kB)]
>
> you write "If nonzero, ... should free p .... Otherwise ... should do the
> equivalent of freeing p". Is there a difference between "free p" and "do the
> equivalent of freeing p"?

It's freeing p followed by malloc (0).

> Also "ISO C17 says the behavior is implementation-defined here": I guess you
> mean the "default value of REALLOC_ZERO_BYTES_FREES", since for me the
> behavior is what is described above.

This part I find confusing as well, and also the “If nonzero”
clause—it's probably best to mention the REALLOC_ZERO_BYTES_FREES macro.
And maybe also clarify that the range of permitted C17 behaviors goes
beyond the two choices controlled by REALLOC_ZERO_BYTES_FREES.

Thanks,
Florian

Re: Fix REALLOC_ZERO_BYTES_FREES comment

[Paul Zimmermann]

       Hi Florian,

> > you write "If nonzero, ... should free p .... Otherwise ... should do the
> > equivalent of freeing p". Is there a difference between "free p" and "do the
> > equivalent of freeing p"?
> 
> It's freeing p followed by malloc (0).

sorry I was not clear. The complete sentence proposed by Paul is

"Otherwise, realloc (p, 0) should do the equivalent of freeing p and returning
what malloc (0) would return."

Why not simply the following?

"Otherwise, realloc (p, 0) should free p and return what malloc (0) would."

Am I overlooking something? What puzzles me is why we write "free p" in the
first part and "do the equivalent of freeing p" in the second one.

Paul

Re: Fix REALLOC_ZERO_BYTES_FREES comment

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

On 4/11/21 11:19 PM, Paul Zimmermann wrote:
> Why not simply the following?
> 
> "Otherwise, realloc (p, 0) should free p and return what malloc (0) would."

Thanks, that is better wording. I installed the attached comment patch, 
which also attempts to address the other points you and Florian made.

[-- Attachment #2: 0001-Further-fixes-for-REALLOC_ZERO_BYTES_FREES-comment.patch --]
[-- Type: text/x-patch, Size: 1552 bytes --]

From 9f1bed18f9466ac886addb2f79d8e4c52fb65eb5 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 12 Apr 2021 00:33:15 -0700
Subject: [PATCH] Further fixes for REALLOC_ZERO_BYTES_FREES comment

* malloc/malloc.c (REALLOC_ZERO_BYTES_FREES): Improve comment further.
---
 malloc/malloc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 0cd3ba78ca..e2d7b1b583 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -346,13 +346,14 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
 #define REVEAL_PTR(ptr)  PROTECT_PTR (&ptr, ptr)
 
 /*
-  REALLOC_ZERO_BYTES_FREES controls the behavior of realloc (p, 0)
-  when p is nonnull.  If nonzero, realloc (p, 0) should free p and
-  return NULL.  Otherwise, realloc (p, 0) should do the equivalent
-  of freeing p and returning what malloc (0) would return.
-
-  ISO C17 says the behavior is implementation-defined here; glibc
-  follows historical practice and defines it to be nonzero.
+  The REALLOC_ZERO_BYTES_FREES macro controls the behavior of realloc (p, 0)
+  when p is nonnull.  If the macro is nonzero, the realloc call returns NULL;
+  otherwise, the call returns what malloc (0) would.  In either case,
+  p is freed.  Glibc uses a nonzero REALLOC_ZERO_BYTES_FREES, which
+  implements common historical practice.
+
+  ISO C17 says the realloc call has implementation-defined behavior,
+  and it might not even free p.
 */
 
 #ifndef REALLOC_ZERO_BYTES_FREES
-- 
2.27.0

Re: Fix REALLOC_ZERO_BYTES_FREES comment

[Florian Weimer]

* Paul Eggert:

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 0cd3ba78ca..e2d7b1b583 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -346,13 +346,14 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
>  #define REVEAL_PTR(ptr)  PROTECT_PTR (&ptr, ptr)
>  
>  /*
> -  REALLOC_ZERO_BYTES_FREES controls the behavior of realloc (p, 0)
> -  when p is nonnull.  If nonzero, realloc (p, 0) should free p and
> -  return NULL.  Otherwise, realloc (p, 0) should do the equivalent
> -  of freeing p and returning what malloc (0) would return.
> -
> -  ISO C17 says the behavior is implementation-defined here; glibc
> -  follows historical practice and defines it to be nonzero.
> +  The REALLOC_ZERO_BYTES_FREES macro controls the behavior of realloc (p, 0)
> +  when p is nonnull.  If the macro is nonzero, the realloc call returns NULL;
> +  otherwise, the call returns what malloc (0) would.  In either case,
> +  p is freed.  Glibc uses a nonzero REALLOC_ZERO_BYTES_FREES, which
> +  implements common historical practice.
> +
> +  ISO C17 says the realloc call has implementation-defined behavior,
> +  and it might not even free p.
>  */

I think this is a really good explanation of the situation.  Thanks.

Florian

Re: Fix REALLOC_ZERO_BYTES_FREES comment

[DJ Delorie]

I'll add that, had this been in the *manual*, the original wording would
have been more appropriate.  Otherwise, there's a missed expectation of
what calls an interposer would see.

We can be more lax with internal comments ;-)