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. And do we even need it to be volatile?
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.
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
