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

[Thomas Guillem]

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.

> 
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20190927/fb4b20c1/attachment.html>