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

[Steve Lhomme]

OK. I think it's better to prevent issues we can catch rather than 
hiding the problem.

Any opinion Thomas ? Hugo ? Francois ?

On 2020-02-11 10:33, Rémi Denis-Courmont wrote:
> 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é.
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>