Re: [boost] [context review]

Subject: Re: [boost] [context review]
From: Giovanni Piero Deretta (gpderetta_at_[hidden])
Date: 2011-03-25 07:31:58

On Fri, Mar 25, 2011 at 7:34 AM, Oliver Kowalke <oliver.kowalke_at_[hidden]> wrote:
>
>> A stack can only be safely deallocated if either has been completely
>> unwound or if it doesn't own any resources (note that is not enough to
>> just destroy any stack allocated object). Context should either: a)
>> provide an interface to unwind the stack (by automatically trowing a
>> special exception for example), or b) detect whether the stack has not
>> been unwound and assert in debug mode in the destructor.
>
> Because boost.context should be a thin wrapper/building block the higher abstractions are responsible. If the execution context was yielded in the middle of its exec path it would require to jump back throw an exception (whic hmay consume a lot of CPU cycles).
> Is this realy required? Couldn't it be reasonable for some implementations only to deallocate the stack?
>

It is very rare (but of course possible) that you want to deallocate
the stack without unwinding it. As I said, I think that the current
context class is at the wrong abstraction level: it should either not
provide RAII at all (I.e. a plain C interface) or a complete safe
implementation.

>
>> Context is neither copyable nor movable; I can understand the reason
>> for making it non-copyable, but I see no reason why it shouldn't be
>> movable. Without either capability it is hard to make context
>> interoperate with existing code without forcing to dynamic allocation.
>
> the reason not to support move-semantics relies on the support of ucontext.
> ucontext_t can not be copied (as required by move-semantics) because the struct ucontext_t is is implementation defined and may vary from release to release. Many man pages of several OS definetly warn for copying ucontext_t (http://nixdoc.net/man-pages/HP-UX/man2/makecontext.2.html).
>

Hum, I seem to remember reading of such a limitations, but a quick
search didn't return anything. Also I couldn't find anything relevant
on the Single Unix Specification. Anyways, you can still allocate the
ucontext dynamically; it will be more expensive, but ucontext is
already slow enough that it won't matter. Or you could allocate it on
the provided stack. I see no reason to cripple the interface because
of limitations of a specific implementation.

>
>> Context is templated on both the context implementation and the stack.
>>  The first problem is that the concept requirements for the Stack
>> template arguments are not stated.
>
> I describe the requirements in the docu (implicit interface, move-semantics). I'm not sure which additional descriptions are required.
>

Can you point me to these requirements? I see the definition of the
protected_stack interface, but not of the concept requirements for
StackT.

Maybe I'm looking at an old version of the docs. I'm looking at this:
http://ok73.ok.funpic.de/boost/libs/context/doc/html/context/context_management.html

>
>> Second, the stack type should not
>> be part of the context type: the primary reason for specifying
>> different stacks is for option of having the guard page: enabling this
>> should not change the type of Context and it should possible to
>> specify it at runtime.
>
> But enabling a guard page with mprotect() requires that the memory was allocated with mmap() othwerwise the behaviour is unspecified.
>

How that prevents the stack being from being specifiable at runtime?
What I'm asking for is for the class not to be templated on the stack
type but only the constructor.

>
>> A good abstraction layer around the low
>> level context, among other things, should support clean unwind of the
>> stack (or detecting an not-unwound stack), generic functors (and not
>> just function pointers) for type safety and custom deleters.
>
> I think this is discussible: I think that detecting an unwound stack would be a good addition. But Ive some concerns regarding to the other points:
> automatic stack unwinding, generic functors.
> Thsi should handled/implemented by this low-layer lib. For instance generic functors would require an additional allocation and some higher abstractions might not use/want such generic functors but would have to pay for it.
>

Why generic functions would require an additional allocation? You can
trivially build them on top of the existing interface. Again, it is
fine for the C interface to only support pointers, but a RAII wrapper
should be complete.

>
>> On the other hand the detail fcontext class seems a good, if maybe not
>> complete, portable implementation of a low level ucontext-like API. I
>> would be in favor of a simpler version of context which consists
>> solely on the fcontext class (with a guarantee that this type is a
>> POD), plus the make_context and swap_context functions. A user will
>> have lower expectations from such a low level API and at the same time
>> will give more freedom for other people to implement on top of it. A
>> safe context class would just be one of the client of the API, other
>> library authors could either use Context of the underlying fcontex
>> directly depending on wether they need the speed or safety.
>
> I was already requested to provide a C-lib regarding to fcontext.
>

Great. This should be the low level interface I'm talking about: leave
all the stack handling to the user and only provide a context
switching implementation. On the other hand, the context class (built
on top of this low level interface), should provide a much higher
degree of safety.

>
>> I think this basic API could still be tweaked [but my vote won't be
>> conditional on that] to add extra functionality:
>>
>>  * Instead of supporting a 'fc_link' a-la ucontext, have the callback
>> function return a new context (it is void now). 'fc_link' is useful
>> only on some limited cases (usually when you have an external
>> scheduler managing your contextes)
>
> this is what boost.tasklet does
>

Are you saying that boost tasklet provides a scheduler or that it
support for 'next continuation' returning functions?

>
>> Most of the 'throw' statements in the context class should actually be
>> asserts.
>
> was my first thought too - but what happend if the user compiles it with NDEBUG?
>

UB of course, as usual when removing assert and
invariants/preconditions are broken.
A quick grep finds 5 uses of throw, all in context.hpp:

  - context constructor from function and chaining constructor:

      if ( ! stack_) throw invalid_stack();

    Just require stack to be valid as a precondition (and assert on
debug builds). The user should be able to track the validity of its
stacks anyway.

      if( ! base) throw bad_alloc();

   it is not obvious at all that stack_.address() (used to initialize
base) is allocating memory; In fact I see no reason it should, nor do
the protected_stack_{posix,windows} implementations. Just require that
stack_.address() returns a non null pointer.

 - jump_to

   Same as above: having a valid stack should be a precondition for
calling this function. Note that here it is particularly important.
This function is performance critical and should be as simple as
possible.

>
>> Make_fcontext requires a call to get_fcontex first, a-la ucontext:
>> there is no reason for this, just merge get_context with make_context
>> (and drop the unused set_fcontext).
>
> this was required for the Windows fcontext implementation
>

Can you elaborate on that? It is not obvious to me why the two
functions cannot be merged together on this platform...

>
>> The posix ucontext wapper and the windows fiber wrapper should be made
>> API compatible with fcontext.
>
> could you explain it please - I'm uncertain what do you mean.
>

Ok, just forget about this. What I mean is that the library should
provide a C api. Something like this:

  struct mycontext_t; // POD, memcpy-able.

  typedef mycontext_t fn_t(intptr_t);

  extern "C" intptr_t swap_mycontext( mycontext_t * ofc, mycontext_t
const* nfc, intptr_t arg);

  //  as swap_mycontext but with function injection.
  extern "C" intptr_t swap_mycontext2( mycontext_t * ofc, mycontext_t
const* nfc, intptr_t arg, intptr_t (*chain)(intptr_t));

  extern "C" intptr_t make_mycontext( mycontext_t * fc, fn_t * f, intptr_t p);

  // Destroy any other memory allocated by fc. Does not deallocate the
stack. If stack is not unwound, the user better know
  // what he is doing
  extern "C" intptr_t destroy_mycontext( mycontext_t * fc);

Protected_stack should be provided as is (plus a C interface if you
really feel is useful), as it is good enough right now.

>
>> Bot the i386 and amd64 asm implementation save and restore the fpu
>> control word (the amd64 both x87 and SSE2 control words).
>
> The SYSV AMD64 ABI requires that the SSE2 control and status word
> and the x87 control word have to preserved across function calls.
>

You are right. For correctness you need to preserve them. It would be
nice to have an option to remove these save/restore when the user does
not manipulate the control word or it manipulates it program wise.

>
>> BTW, the current code is already inconsistent because it is saving the
>> SSE2 control world on amd64 but not on i386.
>
> I didn't read in the SYSV I386 ABI document about the fact that it requires to preserve SSE2 stuff.
>

I think this is still the official SYSV i386 abi document:

  http://www.sco.com/developers/devspecs/abi386-4.pdf

but it doesn't mention SSE at all, so it can be complete. Do you have
any other reference?

>
>> For posix systems lacking ucontext, fcontext can be implemented using
>> the sigaltstack trick (this can actually be faster than ucontext if
>> the non signal restoring _longjmp is used). For reference see:
>> http://www.gnu.org/software/pth/rse-pmt.ps
>
> I'll take a look at it - but AFAIK setjmp/longjmp are not required to preserve the stackframe. Maybe it will not work on all platforms.
>

It is a very dirty hack which is not guaranteed to work anywhere, but
in practice is much more portable than ucontext_t.

-- 
gpd