[vlc-devel] [PATCH] lib: assert instead of abort

[Alexandre Janniaux]

Hi,

I'm fine with vlc_assert_unreachable() for my part, if we can ensure
we get correct calltrace in case of misuse (without vlc-ios patch for
example ?). I'm not sure it will optimize out a lot of thing but we
never know.

Regards,
--
Alexandre Janniaux
Videolabs

On Fri, Sep 27, 2019 at 05:33:58PM +0200, Thomas Guillem wrote:
>
> On Fri, Sep 27, 2019, at 17:28, Steve Lhomme wrote:
> > On 2019-09-27 16:41, Thomas Guillem wrote:
> > >
> > >
> > > On Fri, Sep 27, 2019, at 16:26, Rémi Denis-Courmont wrote:
> > >> Hi,
> > >>
> > >> I am on holidays without laptop, so I cannot check, but I believe the answer to your question is history. I mean, vlc_assert_unreachable() should be used there but it probably did not exist when the code was written.
> > >
> > > I don't like vlc_assert_unreachable(). I would prefer that this helper abort() instead or doing a __builtin_unreachable().
> > > Indeed,it is very hard to debug a code that reached this statement, everything is UB after that. I prefer to kill the process instead of making the app totally UB.
> >
> > When compiled in libvlc, vlc_assert_unreachable() asserts. So I think it
> > would work like an assert just fine.
> >
> > On the other hand, unreachable doesn't seem to signal the right issue in
> > code called by a libvlc user.
>
> I mean vlc_assert_unreachable() if asserts are disabled (in release) is very painful to debug and cause a lot of UB. I prefer to abort() in that case.
>
> >
> > >>
> > >> Le 20 septembre 2019 15:49:50 GMT+03:00, Romain Vimont <rom1v at videolabs.io> a écrit :
> > >>> On Fri, Sep 20, 2019 at 03:13:08PM +0300, Rémi Denis-Courmont wrote:
> > >>>> I completely disagree. When event registration/deregistration fails, you're almost guaranteed a deadlock or some use after free. Much better to abort asap.
> > >>>
> > >>> If an assertion fails, then the program has (probably) undefined
> > >>> behavior.
> > >>>
> > >>> But why using assert() everywhere but abort() here? There are a lot of
> > >>> places where a failing assert() would almost guarantee a deadlock or
> > >>> some use after free.
> > >>>
> > >>>> Le 20 septembre 2019 12:21:37 GMT+03:00, Thomas Guillem <thomas at gllm.fr> a écrit :
> > >>>>> Because you should not abort in production. Developers will still be
> > >>>>> notified
> > >>>>> that they are miss-using the API with the assert.
> > >>>>>
> > >>>>> PS: this abort was happening on iOS via VLCKit, the API miss-use had
> > >>>>> been fixed
> > >>>>> here: https://code.videolan.org/videolan/VLCKit/merge_requests/19 lib/event.c | 2 +-
> > >>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/lib/event.c b/lib/event.c
> > >>>>> index 28d3d0c232..4a57029b89 100644
> > >>>>> --- a/lib/event.c
> > >>>>> +++ b/lib/event.c
> > >>>>> @@ -179,5 +179,5 @@ void libvlc_event_detach(libvlc_event_manager_t
> > >>>>> *em, libvlc_event_type_t type,
> > >>>>>                return;
> > >>>>>            }
> > >>>>>       }
> > >>>>> -    abort();
> > >>>>> +    assert(!"event not found or already detached");
> > >>>>>   }
> > >>>>> --
> > >>>>> 2.20.1vlc-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
> > >>
> > >> --
> > >> 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
> > >
> > _______________________________________________
> > 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