Hi! On Tue, Aug 24, 2021 at 11:50:57AM +0200, Michael Kerrisk (man-pages) wrote: > On 8/23/21 11:01 PM, наб wrote: > > Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@xxxxxxxxxxxxxxxxxx> > > --- > > 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. Will do. > > -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. How'd you mean? I think I'm missing what there's to be explained in this particular hunk. > > +By default, modern compilers automatically translate plain > What does "plain" mean here? It is not explained. How about "By default, modern compilers automatically translate all uses of alloca() into the built-in, but this is forbidden if..."? > > .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"? Because... it isn't inlined code (indeed, code at all)? It's an intrinsic. If you don't include alloca.h, aliasing alloca() to __builtin_alloca() is also 100% magic. PWB and 32V do actually implement this as a libc function and raze the stack frame (PWB even pre-emptively crashes, as a feature, if it OOMed), but those days are long gone. > And why the word "forbidden"? Who forbids it? The standards that are outlined in the following seven words. > > .IR "\-ansi" , > > .IR "\-std=c89" , > > .IR "\-std=c99" , > > -or the > > +or > > .IR "\-std=c11" > Okay. > > .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. The first bit is glibc guts, and has no place in documentation of alloca(3) as an interface ‒ this is noted. The latter paragraph just moved down. > > -.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? Because there /is/ no code, inlined or otherwise. The stack pointer or lack thereof is beside this point, and the rest of this paragraph lives in the hunk below. > > +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.) I kept it from the hunk you outlined above, I can drop it, but it is an important difference from most regular allocators, which are able to detect an OOM condition. By definition, you have stack until you hit an unmapped or non-R/W-mapped page. How about something like "Due to the nature of the stack, there is ..."? > 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. The CONFORMING TO split is trivial, but I hardly see a good cleave line for anything beside it. If we can arrive at a consensus on some of the bits you found problematic in this version, I'll split it two-fold and resend with the new wording and commit message, then we can cleave it further if you'd like? Best, наб
Attachment:
signature.asc
Description: PGP signature
