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

[Thomas Guillem]

On Tue, Feb 11, 2020, at 10:41, Steve Lhomme wrote:
> OK. I think it's better to prevent issues we can catch rather than 
> hiding the problem.
> 
> Any opinion Thomas ? Hugo ? Francois ?

Even if not perfect, I prefer to have this patch merged.

> 
> 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
> > 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel