[vlc-devel] [PATCH] threads: forbid the use of vlc_cleanup_push/pop in portable C++ code

[Rémi Denis-Courmont]

Hi,

Strictly speaking, there is a problem if one or more frames are not C. It's still possible that:
- cleanup handlers are pushed from C code called from C++ code -> we cannot prevent that,
- cleanup handlers are pushed by inline header code that is cancelable in C, but not in C++. That's why cleanup handlers are defined to no-ops in C++ atm and this patch breaks that (theoretical) scenario.

I'm not against the patch because it will catch the most obvious mistakes, at little cosy, but in all honesty, it's not entirely correct.

Le 11 février 2020 10:51:31 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>On 2020-02-11 9:45, Rémi Denis-Courmont wrote:
>> Hi,
>> 
>> I considered doing that, but it potentially wrecks common C/C++ code
>(in 
>> headers), if the code is cancellable in C. Same problem with 
>> vlc_cancel(), which is not strictly wrong in C++.
>
>If the code is cancelable in C, vlc_cancel() can still be called from C
>
>(and in fact even from C++).
>
>If the thread is created from C++ and then calls C code which push/pops
>
>a cleanup routine, it should not impact the C++ part of the code. After
>
>the vlc_testcancel(), the C code will normally go up the stack and end 
>up in the C++ part.
>
>If the thread is created in C++, the C++ part cannot push/pop a cleanup
>
>routine as we can't guarantee it will work (in portable code). So IMO
>it 
>makes sense to forbid that part only.
>
>> But if you prefer that way, I can live with it.
>> 
>> Le 11 février 2020 09:45:01 GMT+02:00, Steve Lhomme
><robux4 at ycbcr.xyz> a 
>> écrit :
>> 
>>     It may work for some C++ code only used on Linux. But if the
>module is compiled
>>     on other platforms it will assert.
>>    
>------------------------------------------------------------------------
>>       include/vlc_threads.h | 10 ++++------
>>       1 file changed, 4 insertions(+), 6 deletions(-)
>> 
>>     diff --git a/include/vlc_threads.h b/include/vlc_threads.h
>>     index 0e27ea803fa..5a75f4ad1f8 100644
>>     --- a/include/vlc_threads.h
>>     +++ b/include/vlc_threads.h
>>     @@ -1084,12 +1084,10 @@ struct vlc_cleanup_t
>>               vlc_control_cancel (NULL); \
>>           } while (0)
>>       # else
>>     -/* Those macros do not work in C++. However common C/C++ helpers
>may call them
>>     - * anyway - this is fine if the code is never cancelled in C++
>case.
>>     - * So define the macros to do nothing.
>>     - */
>>     -#  define vlc_cleanup_push(routine, arg) do { (routine, arg)
>>     -#  define vlc_cleanup_pop() } while (0)
>>     +#  define vlc_cleanup_push(routine, arg) \
>>     +    static_assert(false, "don't use vlc_cleanup_push in portable
>C++ code")
>>     +#  define vlc_cleanup_pop() \
>>     +    static_assert(false, "don'o't use vlc_cleanup_pop in
>portable C++ code")
>>       # endif
>>       
>>       #endif /* !LIBVLC_USE_PTHREAD_CLEANUP */
>> 
>> 
>> -- 
>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>excuser 
>> ma brièveté.
>> 
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>> 
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200211/8cb073d1/attachment.html>