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

[Thomas Guillem]

Anyway, this patch is wrong since I miss the mutex_unlock.

By the way, the libvlc_event_detach documentation lack a comment about the necessity that arguments must correspond to a previous libvlc_event_attach.

As said by Alexandre, using an event_id like playlist/player would be better in that case.

On Mon, Sep 30, 2019, at 08:00, Steve Lhomme wrote:
> On 2019-09-27 17:33, 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.
> 
> In the rare case a libvlc app would have issues only in the release 
> mode, then yes it's trickier to debug. One can always force asserts in a 
> release build too.
> 
> >>
> >>>>
> >>>> 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
> > 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel