On 18/12/12 14:17, Ulisses Furquim wrote:
> Hi,
>
> On Tue, Dec 18, 2012 at 12:07 PM, Ulisses Furquim
> <ulis...@profusion.mobi> wrote:
>> Hi,
>>
>> On Tue, Dec 18, 2012 at 11:33 AM, Stefan Schmidt <s.schm...@samsung.com>
>> wrote:
>>> Hello.
>>>
>>> On 18/12/12 13:09, Ulisses Furquim wrote:
>>>> Hi,
>>>>
>>>> On Tue, Dec 18, 2012 at 7:49 AM, Stefan Schmidt <s.schm...@samsung.com>
>>>> wrote:
>>>>> Hello.
>>>>>
>>>>> On 17/12/12 10:07, Tom Hacohen wrote:
>>>>>> Well. The code creates a lock per class and uses a global lock to handle
>>>>>> the lock per class creation. It's needed because classes are actually
>>>>>> created on the fly. Just reading through the code and seeing where it
>>>>>> returns and when it's released and looks that it's just fine.
>>>>>>
>>>>>> I would recommend you to use a tool that actually shows the path of
>>>>>> execution leading to the proclaimed error.
>>>>>
>>>>> It actually does that. :) Here is an example trace:
>>>>>
>>>>> ecore_anim.c:634: __builtin_expect( ( !!_my_class), 1) is false
>>>>> ecore_anim.c:634: lk_init<2 is true
>>>>> ecore_anim.c:634: !lk_init is false
>>>>> ecore_anim.c:634: lk_init<2 is false
>>>>> ecore_anim.c:634: Variable '_my_lock.mutex' is locked.
>>>>> ecore_anim.c:634: Variable '_my_lock.mutex' was locked.
>>>>>
>>>>> ecore_anim.c at line 634 contains the macro:
>>>>> EO_DEFINE_CLASS(ecore_animator_class_get, &class_desc, EO_BASE_CLASS,
>>>>> NULL)
>>>>>
>>>>> If we now look at the macro again with the trace in mind:
>>>>> #define EO_DEFINE_CLASS(class_get_func_name, class_desc, parent_class,
>>>>> ...) \
>>>>> EAPI const Eo_Class * \
>>>>> class_get_func_name(void) \
>>>>> { \
>>>>> const Eo_Class *_tmp_parent_class; \
>>>>> static volatile char lk_init = 0; \
>>>>> static Eina_Lock _my_lock; \
>>>>> static const Eo_Class * volatile _my_class = NULL; \
>>>>> if (EINA_LIKELY(!!_my_class)) return _my_class; \
>>>>> \
>>>>> eina_lock_take(&_eo_class_creation_lock); \
>>>>> if (!lk_init) \
>>>>> eina_lock_new(&_my_lock); \
>>>>> if (lk_init < 2) eina_lock_take(&_my_lock); \
>>>>> if (!lk_init) \
>>>>> lk_init = 1; \
>>>>> else \
>>>>> { \
>>>>> if (lk_init < 2) eina_lock_release(&_my_lock); \
>>>>> eina_lock_release(&_eo_class_creation_lock); \
>>>>> return _my_class; \
>>>>> } \
>>>>> eina_lock_release(&_eo_class_creation_lock); \
>>>>> _tmp_parent_class = parent_class; \
>>>>> _my_class = eo_class_new(class_desc, _tmp_parent_class,
>>>>> __VA_ARGS__); \
>>>>> eina_lock_release(&_my_lock); \
>>>>> \
>>>>> eina_lock_take(&_eo_class_creation_lock); \
>>>>> eina_lock_free(&_my_lock); \
>>>>> lk_init = 2; \
>>>>> eina_lock_release(&_eo_class_creation_lock); \
>>>>> return _my_class; \
>>>>> }
>>>>>
>>>>> One can the that _my_lock will be taken with lk_init < 2 true but not
>>>>> released with lk_init < 2 false. And as lk_init has the volatile keyword
>>>>> it can be changed without our knowing. So the analyser seem to have a
>>>>> valid case here as it can't understand what will happen to lk_init.
>>>>
>>>> Well, if lk_init < 2 is false then _my_class should be set and we
>>>> return it with "if (EINA_LIKELY(!!_my_class)) return _my_class;" and
>>>> that's it. And volatile in this case just tells the compiler to fetch
>>>> the operand from memory every time which disables some optimizations
>>>> like leaving it in a register.
>>>
>>> Well, from the traceback one can see that the analyser checks the case
>>> where lk_init<2 is true first and after that assumes it is false so it
>>> would not hit the path you think it will.
>>>
>>> I _think_ that it does that because it assumes lk_init can change behind
>>> its back in any case and thus tests all cases for a volatile var. It
>>> just don't trust the content at all to stay the same. Do we change it
>>> behind its back is the question we need to answer. :)
>>
>> lk_init is just changed under _eo_class_creation_lock, so I don't
>> think it'll change under us and the volatile modifier will make the
>> compiler generate code to load/store from/to memory every time.
>>
>>>> And do we even need it to be volatile?
>>>
>>> If we don't change it we should not need it. But maybe I miss some
>>> concepts of EO here.
>>
>> Well, I don't think there's any EO concept really involved but the EO
>> authors could enlighten us. ;-)
>>
>>>> It might be the case we don't need this modifier as it's protected by
>>>> _eo_class_creation_lock.
>>>>
>>>>> The real question now is if we can guarantee that lk_init will be <2
>>>>> also in the unlock case. I have no idea what tricks eo is playing here
>>>>> with the volatile variable so that is where you guys come into place. :)
>>>>
>>>> I do think we have this guarantee because like I said lk_init is
>>>> protected by _eo_class_creation_lock. I do think we have a false
>>>> positive and the code looks "right" regarding taking and releasing
>>>> locks. However, why do we need 2 locks? Can't we just use
>>>> _eo_class_creation_lock? It seems lk_init and _my_lock just exist to
>>>> do this "2 stage" class creation where you drop
>>>> _eo_class_creation_lock when you "don't need it anymore". Right?
>>>> Please tell me if I'm missing anything.
>>>
>>> First of all thanks for taking the time looking over this. I tend to
>>> agree that it is a false positive. But given that this affects 105
>>> places and might give us troubles later I wanted to be sure before
>>> tagging it false positive and ignoring it. I just don't like the idea of
>>> doing that and it later turns out to have been a valid problem. :)
>>
>> I agree and thanks for reporting. I still think we might simplify
>> things by just using one lock, let's see.
>
> Well, TAsn just told me we need to drop the global lock because other
> classes might be created on the fly and thus we would deadlock. That
> is really confusing nonetheless and that is why there's this 2-stage
> class creation.
So in summary we are stuck with what we have but at least we don't need
to worry about the reports as they are under the global lock anyway.
regards
Stefan Schmidt
------------------------------------------------------------------------------
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
