Add support for alternate boost-context backends#30496
Add support for alternate boost-context backends#30496alalazo merged 3 commits intospack:developfrom
Conversation
The fcontext backend is the default high-performance backend. The ucontext backend is needed when using Boost context in conjunction with ASAN. The WinFibers backend...also exists. cf. https://www.boost.org/doc/libs/1_79_0/libs/context/doc/html/context/cc/implementations__fcontext_t__ucontext_t_and_winfiber.html
tldahlgren
left a comment
tldahlgren
left a comment
There was a problem hiding this comment.
LGTM. Deferring merge decision to maintainers/package experts.
tldahlgren
left a comment
tldahlgren
left a comment
There was a problem hiding this comment.
Please correct the style error; otherwise, LGTM.
| values=('fcontext','ucontext','winfib'), | ||
| multi=False, | ||
| description='Use the specified backend for boost-context', | ||
| when='+context') |
There was a problem hiding this comment.
What version of Boost introduced this feature? I'm working to make sure variants are correctly mapped to versions where they are actually available.
There was a problem hiding this comment.
The relevant pages are here:
https://www.boost.org/doc/libs/1_79_0/libs/context/doc/html/context/stack/sanitizers.html and here https://www.boost.org/doc/libs/1_79_0/libs/context/doc/html/context/cc/implementations__fcontext_t__ucontext_t_and_winfiber.html
It's available at least as far back as 1.65, haven't gone further back.
There was a problem hiding this comment.
I'll run my automated tests and check. edit: We don't need to hold this PR for this.
There was a problem hiding this comment.
Incidentally there's an option for supporting valgrind that would be great to get rolled in as well (as a separate PR):
There was a problem hiding this comment.
Oh, very nice! I will try to see about getting at least a reference list of the features/properties.
| context_impl = spec.variants['context-impl'].value | ||
| if '+context' in spec and context_impl in {'ucontext', 'winfib'}: | ||
| options.extend(['context-impl=%s' % context_impl]) | ||
|
|
There was a problem hiding this comment.
Assuming b2 won't raise any fuss if we pass context-impl=fcontext, there's no need to check for it here. That will remove this maintenance hazard in keeping the check here and the values of the variant in synch.
| if '+context' in spec: | |
| context_impl = spec.variants['context-impl'].value | |
| options.append('context-impl=%s' % context_impl) |
There was a problem hiding this comment.
The documentation page I linked above only discusses the possibility of ucontext or winfib as arguments.
There was a problem hiding this comment.
In 1.79.0,
$ ./bootstrap.sh --with-libraries=context
$ ./b2 context-impl=moo
/boost_1_79_0/tools/build/src/build/feature.jam:491: in validate-value-string from module feature
error: "moo" is not a known value of feature <context-impl>
error: legal values: "fcontext" "ucontext" "winfib"
$ ./b2 context-impl=fcontext
...
- context : building
...
The Boost C++ Libraries were successfully built!Let's assume for now that always works. I'll add it to my TODO list to double-check.
There was a problem hiding this comment.
Cool - i just verified fcontext is present as an option at least as far back as 1.65:
https://github.com/boostorg/context/blob/boost-1.65.0/build/Jamfile.v2
I'll go ahead and remove that extra check.
I will note that the special-casing for the defines introduced in 65d7b34 should be kept as BOOST_USE_FCONTEXT is not used anywhere.
There was a problem hiding this comment.
Oh, that's... neat. Particularly since ucontext was removed in POSIX.1-2008. Boost, why did you do that?!
Out of curiosity, which backend are you using?
There was a problem hiding this comment.
fcontext is definitely the sensible default for most use-cases, but ucontext is the only way to cooperate with ASAN if you're doing memory debugging (or was - it seems to have broken at some point recently since ASAN promptly reported a bug in boost itself when I switched)
There was a problem hiding this comment.
Oh sorry, yes, you mentioned that in your commit. It's interesting ASAN is relying on the deprecated/removed interface. I would assume it's really difficult to instrument the fcontext usage.
Whitespace fixes. Also improve the client interface somewhat by setting the appropriate flags in the build environment.
tldahlgren
left a comment
tldahlgren
left a comment
There was a problem hiding this comment.
Removing my blocking review related to style check.
Also adds explanatory comments for the special-casing that remains.
|
Looks like the pipeline is stuck. @spackbot re-run pipeline |
|
I had a problem triggering the pipeline. |
The fcontext backend is the default high-performance backend. The ucontext backend is needed when using Boost context in conjunction with ASAN. The WinFibers backend...also exists. https://www.boost.org/doc/libs/1_79_0/libs/context/doc/html/context/cc/implementations__fcontext_t__ucontext_t_and_winfiber.html
4 participants
The fcontext backend is the default high-performance backend.
The ucontext backend is needed when using Boost context in conjunction with ASAN.
The WinFibers backend...also exists.
cf. https://www.boost.org/doc/libs/1_79_0/libs/context/doc/html/context/cc/implementations__fcontext_t__ucontext_t_and_winfiber.html