Use [[gnu::access(none)]] on free(3)

Message ID 20240126183847.12939-2-alx@kernel.org
State Not applicable
Headers
Series Use [[gnu::access(none)]] on free(3) |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed

Commit Message

Alejandro Colomar Jan. 26, 2024, 6:39 p.m. UTC
  The lifetime of the object expires right at the boundary of the call to
free(3), and the function doesn't access (neither read nor write) it, as
far as the abstract machine is concerned.

Cc: Arsen Arsenović <arsen@gentoo.org>
Cc: Andreas Schwab <schwab@linux-m68k.org>
Cc: Xi Ruoyao <xry111@xry111.site>
Cc: Russ Allbery <eagle@eyrie.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 stdlib/stdlib.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

Comments

Alejandro Colomar Jan. 26, 2024, 6:41 p.m. UTC | #1
On Fri, Jan 26, 2024 at 07:39:06PM +0100, Alejandro Colomar wrote:
> The lifetime of the object expires right at the boundary of the call to
> free(3), and the function doesn't access (neither read nor write) it, as
> far as the abstract machine is concerned.
> 
> Cc: Arsen Arsenović <arsen@gentoo.org>
> Cc: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Xi Ruoyao <xry111@xry111.site>
> Cc: Russ Allbery <eagle@eyrie.org>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>

Please do not apply.  Xi pointed out that GCC uses some magic that would
break with the attribute.

> ---
>  stdlib/stdlib.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
> index 414c49d731..8dfd373bf5 100644
> --- a/stdlib/stdlib.h
> +++ b/stdlib/stdlib.h
> @@ -684,7 +684,8 @@ extern void *realloc (void *__ptr, size_t __size)
>       __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2));
>  
>  /* Free a block allocated by `malloc', `realloc' or `calloc'.  */
> -extern void free (void *__ptr) __THROW;
> +extern void free (void *__ptr)
> +     __THROW __attr_access ((__none__, 1));
>  
>  #ifdef __USE_MISC
>  /* Re-allocate the previously allocated block in PTR, making the new
> -- 
> 2.43.0
>
  
Paul Eggert Jan. 26, 2024, 9:23 p.m. UTC | #2
On 2024-01-26 10:41, Alejandro Colomar wrote:
> Please do not apply.  Xi pointed out that GCC uses some magic that would
> break with the attribute.

Plus, I'm not seeing the point of either this patch or the earlier one. 
'free' is built into the language, and GCC therefore can deduce anything 
about 'free' that an attribute would supply. (If glibc made promises 
about 'free' that extend the C standard, that might justify adding an 
attribute, but this does not appear to be the case here.)

If there's some problem with GCC not deducing enough about 'free' in the 
areas discussed on this thread, that can and should be fixed in GCC.

PS. I didn't see the following mentioned so I'll add it here. Putting 
'const void *' into the signature of 'free' would incorrectly reject the 
conforming C code like this:

   void (*f) (void *) = free;
  
Alejandro Colomar Jan. 26, 2024, 11:19 p.m. UTC | #3
Hi Paul,

On Fri, Jan 26, 2024 at 01:23:58PM -0800, Paul Eggert wrote:
> PS. I didn't see the following mentioned so I'll add it here. Putting 'const
> void *' into the signature of 'free' would incorrectly reject the conforming
> C code like this:
> 
>   void (*f) (void *) = free;

I mentioned it.  And yeah, it's a big caveat.  :)

Have a lovely night!
Alex
  
Cristian Rodríguez Jan. 27, 2024, 1:21 p.m. UTC | #4
On Fri, Jan 26, 2024 at 6:24 PM Paul Eggert <eggert@cs.ucla.edu> wrote:

> On 2024-01-26 10:41, Alejandro Colomar wrote:
> > Please do not apply.  Xi pointed out that GCC uses some magic that would
> > break with the attribute.
>
> Plus, I'm not seeing the point of either this patch or the earlier one.
> 'free' is built into the language, and GCC therefore can deduce anything
> about 'free' that an attribute would supply. (If glibc made promises
> about 'free' that extend the C standard, that might justify adding an
> attribute, but this does not appear to be the case here.)
>
>
As there is __builtin_malloc I guess there could be a __builtin_free which
knows all these details..how useful would that be is a different story.
  
Gabriel Ravier Feb. 13, 2024, 3:19 p.m. UTC | #5
On 1/27/24 13:21, Cristian Rodríguez wrote:
>
>
> On Fri, Jan 26, 2024 at 6:24 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
>     On 2024-01-26 10:41, Alejandro Colomar wrote:
>     > Please do not apply.  Xi pointed out that GCC uses some magic
>     that would
>     > break with the attribute.
>
>     Plus, I'm not seeing the point of either this patch or the earlier
>     one.
>     'free' is built into the language, and GCC therefore can deduce
>     anything
>     about 'free' that an attribute would supply. (If glibc made promises
>     about 'free' that extend the C standard, that might justify adding an
>     attribute, but this does not appear to be the case here.)
>
>
> As there is __builtin_malloc I guess there could be a __builtin_free 
> which knows all these details..how useful would that be is a different 
> story.

 From what I can see, __builtin_free already exists.
  
Alejandro Colomar Feb. 13, 2024, 3:28 p.m. UTC | #6
On Tue, Feb 13, 2024 at 03:19:57PM +0000, Gabriel Ravier wrote:
> On 1/27/24 13:21, Cristian Rodríguez wrote:
> > As there is __builtin_malloc I guess there could be a __builtin_free
> > which knows all these details..how useful would that be is a different
> > story.
> 
> From what I can see, __builtin_free already exists.

Christian probably meant __attribute__((free())).

Have a lovely day!
Alex
  

Patch

diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
index 414c49d731..8dfd373bf5 100644
--- a/stdlib/stdlib.h
+++ b/stdlib/stdlib.h
@@ -684,7 +684,8 @@  extern void *realloc (void *__ptr, size_t __size)
      __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2));
 
 /* Free a block allocated by `malloc', `realloc' or `calloc'.  */
-extern void free (void *__ptr) __THROW;
+extern void free (void *__ptr)
+     __THROW __attr_access ((__none__, 1));
 
 #ifdef __USE_MISC
 /* Re-allocate the previously allocated block in PTR, making the new