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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Ahelenia, (do you prefer наб or Ahelenia? What does наб mean, if I may ask? :)

On 8/24/21 11:50 AM, Michael Kerrisk (man-pages) wrote:
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@xxxxxxxxxxxxxxxxxx>

Thanks for the patch!

Most of it looks good. But as Michael said, it would be better to split this into small patches that change 1 thing.

And apart from what Michael (and Branden) already said, I have one question that has not been treated before it seems:

AFAIK, VLAs are 100% equivalent to alloca() (except for the obvious syntax differences). And considering the VLA syntax is much nicer than alloca(), and is in the standard (IIRC, C99 added VLAs, and C11 declared them optional), what about adding a NOTES subsection that recommends (or at least mentions) VLAs?

And they both share the problem of smashing the stack if you try to allocate an array to big (and none of them has a way to check if it will happen, AFAIK).

Cheers,

Alex

---
  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

Minor wording question:

Do we really need _always_ for emphasis? I think it would be the same without that word.

+.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




--
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/



[Index of Archives]     [Kernel Documentation]     [Netdev]     [Linux Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux