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. :)
> 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.
> 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. :)
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
