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.
Thanks,
--
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs
------------------------------------------------------------------------------
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
