Re: [PATCH 2/2] alloca.3: rewrite NOTES

["Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>]

Hello Ahelenia

On 8/23/21 11:01 PM, наб wrote:
> This demistifies the internals and removes outdated information
> and needless glibc guts

Some of this patch seems fine, but it does more than I would like to see
in one patch. Some comments below.

> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---
>  man3/alloca.3 | 66 ++++++++++++++++-----------------------------------
>  1 file changed, 21 insertions(+), 45 deletions(-)
> 
> diff --git a/man3/alloca.3 b/man3/alloca.3
> index 5bceeabe1..133ca6ab3 100644
> --- a/man3/alloca.3
> +++ b/man3/alloca.3
> @@ -84,20 +84,14 @@ T}	Thread safety	MT-Safe
>  .SH CONFORMING TO
>  This function is not in POSIX.1.
>  .PP
> -There is evidence that the
>  .BR alloca ()
> -function appeared in 32V, PWB, PWB.2, 3BSD, and 4BSD.
> -There is a man page for it in 4.3BSD.
> -Linux uses the GNU version.
> +originates from PWB and 32V, and appears in all their derivatives.

The patch subject says "rewrite NOTES", but here you change 
the CONFORMING TO. For the record, I think the change is fine;
there was too much info here that isn't really helpful.
But, I would prefer this change as a separate patch, with
a commit message that notes that the CONFORMING TO is
overly complex, so let's simplify.

>  .SH NOTES
>  The
>  .BR alloca ()
>  function is machine- and compiler-dependent.
> -For certain applications,
> -its use can improve efficiency compared to the use of
> -.BR malloc (3)
> -plus
> -.BR free (3).
> +Because it allocates from the stack, it's always faster than
> +.BR malloc (3)/ free (3).

Okay.

>  In certain cases,
>  it can also simplify memory deallocation in applications that use
>  .BR longjmp (3)
> @@ -125,51 +119,33 @@ Do not attempt to
>  .BR free (3)
>  space allocated by
>  .BR alloca ()!
> -.SS Notes on the GNU version
> -Normally,
> -.BR gcc (1)

(Removing mention of gcc makes sense...)

> -translates calls to
> +.PP
> +By necessity,
> +.BR alloca ()
> +is a compiler built-in, also known as
> +.BR __builtin_alloca ().

I'm not convinced about this change, or what follows. At the
least, it needs some explanation.

> +By default, modern compilers automatically translate plain

What does "plain" mean here? It is not explained.

>  .BR alloca ()
> -with inlined code.
> -This is not done when either the
> +calls, but this is forbidden if

Why lose the piece "with inlined code"?

And why the word "forbidden"? Who forbids it?

>  .IR "\-ansi" ,
>  .IR "\-std=c89" ,
>  .IR "\-std=c99" ,
> -or the
> +or
>  .IR "\-std=c11"

Okay.

> -option is given
> -.BR and
> -the header
> -.I <alloca.h>
> -is not included.
> -Otherwise, (without an \-ansi or \-std=c* option) the glibc version of
> -.I <stdlib.h>
> -includes
> +are specified, in which case
>  .I <alloca.h>
> -and that contains the lines:
> +is required, lest an actual symbol dependency is emitted.

(That last line seems like a useful addition!)

>  .PP
> -.in +4n
> -.EX
> -#ifdef  __GNUC__
> -#define alloca(size)   __builtin_alloca (size)
> -#endif
> -.EE
> -.in
> -.PP
> -with messy consequences if one has a private version of this function.
> -.PP
> -The fact that the code is inlined means that it is impossible
> -to take the address of this function, or to change its behavior
> -by linking with a different library.

Why remove the preceding piece? This should be clarified in the 
commit message.


> -.PP
> -The inlined code often consists of a single instruction adjusting
> -the stack pointer, and does not check for stack overflow.
> -Thus, there is no NULL error return.

Why remove the preceding piece? 

> +The fact that
> +.BR alloca ()
> +is a built-in means it is impossible to take its address
> +or to change its behavior by linking with a different library.
>  .SH BUGS
> -There is no error indication if the stack frame cannot be extended.
> -(However, after a failed allocation, the program is likely to receive a
> +As it's untestable, there is no error indication if the allocation

It's not clear to me that adding "As it's untestable," really helps 
the reader. Why do you think it does? (This should be explained in
the commit message.)

> +would overflow the space available for the stack.
> +(However, the program is likely to receive a
>  .B SIGSEGV
> -signal if it attempts to access the unallocated space.)
> +signal if it attempts to access that space.)
>  .PP
>  On many systems
>  .BR alloca ()

My feelings about this patch:

* There's good stuff here, and stuff that I am less sure of.
* This should be *at least* 2 patches, but possibly 3 or 4.
* We need some meaningful commit messages. Your two line commit
  message is too vague; think of people some years from now
  looking at these changes, and asking: "what was the author's
  motivation for these changes?"

Would you be willing to rework this patch in the light of 
the above please? Breaking it into a few pieces (if you can)
would make it much easier to wave some pieces through and
discuss the other pieces in detail.

Thanks,

Michael

 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/