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

[Hugo Beauzée-Luyssen]

On Tue, Feb 11, 2020, at 10:44 AM, Thomas Guillem wrote:
> 
> 
> 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é.
> > > 

I'm not against this since it will catch obvious issues.

Just be sure to fix the «don'o't» typo :)

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr