luajit wants to check the upper 17 bits of pointers and assume they
are all 0 if on a 64bit architecture. it will panic and barf if they
are used. we have been using them in our eoid's for a long time.
my take on this is that this is unportable. assuming how many bits are
or are not valid in an opaque pointer (void *) that is simply passed
to you and you cannot dereference or use (no size information even for
what amount of data it points to etc.), is just wrong. it's not
portable and it's trying to be too smart and creating such issues. my
take is that luajit needs a fix for this in the longer term. but for
now let's have a 47 bit mode and go with that. it does mean i have to
drop our generation counter to 10 bits on 64bit (from 27 bits) which
increases likelihood of eoid re-use being falsely detected as valid
(before on 64bit it was 1 in 130 million or so chance, with this 47
bit change it's 1 in 1000. but to be fair on 32bit it's 7 bits for gen
count so 1 in 127 ... so still more than 10x "safer" than on 32bit...
but still...). the relevant check in luajit is:
(((uint64_t)(p) >> 47) ? (lj_err_msg(L, LJ_ERR_BADLU), NULL) : (p))
it ONLY does this on 64bit. on 32bit pointers are not checked for
validity at all.
as an aside, armv8.2 seemingly will bring 52bit addresses so luajit is
going to fall over flat on a newer rev of armv8. it may be x86 also
uses more bits. last i knew it was 48bits so the 47 bit check luajit
does i think is even wrong for x86-64. to be detailed i read:
amd64 arch == 48 bits (so luajit is wrong). even better In addition,
the AMD specification requires that the most significant 16 bits of
any virtual address, bits 48 through 63, must be copies of bit 47 (in
a manner akin to sign extension). so if the upper bit of 48 is set
THEN all the 16 upper bits must be 1... breaking luajit, even if it
were 47bit and this rule applied. I read the architecture allows for
up to 52bits of actual addresses so architecture-wise this is even
wrong...
So I smell a core bug here in luajit. Certainly in the number of bits
it insists must be 0 (the upper 17 bits where on amd64/x86-64 it
should be the upper 16 bits... and even then these may NOT be 0 if bit
47 (the upper bit of the lower 48 is 1).... so the whole check is
invalid... :(
at least the above is at a theoretical level. i believe that the
addresses divide the 48 bits into 2 chunks (thus 47)... but at the
PHYSICAL level with no mmu and virtual memory. arm64 has this:
https://www.kernel.org/doc/Documentation/arm64/memory.txt
note in all cases the 2nd chunk of memory has at leats some upper bits
of physical addresses beign 1 ... which makes luajit invalid tyo use
without virtual memory remapping these away from high bits.
@fix
It would be pretty stupid to create an object that dies before it even
gets created. Auto-unref is for parts and should probably not be used
anywhere else :)
the freeq can't allow things to access the content of the pointer
because the content canbe written over by canary handling qhen
submitted to the freeq. clear it inline then just submit the final
pointer to the freeq.
@fix
Before screaming in horror (C++...) here's why we may need this:
Efl.Part.part API returns an object that is by definition valid for a
single function call only. Enforcing this in practice is actually quite
hard as all implementation functions must manually take care of the
life-cycle. This is a lot of code in many places and a lot of
opportunities to forget to properly handle that life-cycle. Also, this
means any invalid function call on a part will leak an object.
This API absolutely must remain either "internal" or "beta" and
definitely not become abused by applications. On top of that such an API
can cause great trouble for bindings like C++. As a consequence, only
specially crafted APIs like efl_part() should return an object marked as
auto_unref.
Alternatively this API could be defined in Eo.h or some other
Eo_Internal.h. I placed it in efl_object.eo because it's much more
convenient :) (read: I'm lazy)
****
Performance notes:
Tested with clang & gcc (with -O2), I had a look at the output of perf
top, in particular the asm view. I used eo_bench in a loop. My
conclusions are:
- EINA_LIKELY/UNLIKELY actually works. The jump statement varies
according to the expectation. I highly doubt all those ugly goto in
eo.c / Eo.h are even useful.
- The impact of auto_unref on a call_resolve is so small it doesn't even
appear in the trace. It is significant inside call_end, though
(obviously, that function is just a few lines long). That function
accounts for ~1% to ~4% of all CPU time. The impact of auto_unref in
call_end is ~4% of the function time. This means ~0.16% of all CPU
time (worst measured case). _efl_object_op_api_id_get simply doesn't
show up because of caching, so the extra check there is negligible.
PS: I also tested EINA_LIKELY/UNLIKELY by compiling with -O2 and looking
at the output with objdump. The flag is well respected, and the jump
instructions are what you would expect (no jump for LIKELY and jump for
UNLIKELY). Conclusion: The goto's in eo.c only make the code harder to
read...
Simply pass in the strbuf and don't expect the callee to own it. This
makes things simpler and safer (it'll crash only if the callee frees
said strbuf, and shouldn't leak). efl_ebug_name is new in the upcoming
release, EFL 1.21.
Realised this after talking with Amitesh. Thanks.
See 999dbd9764
And c4769ff898
there can be cases where the call to such a function is never going to
be directed to the actual implementation. In those cases the FallackCall
will be called.
This allows deleting an object by simply calling efl_unref() on it, even
if there is a parent. Normally the parent is in charge, and you can
request deletion by calling efl_del() or efl_parent_set(NULL).
But in some rare cases, you want to give ownership of an object (@owned)
and still give a parent to that object. efl_unref() should be used (and
would be used by bindings when going out of scope or on garbage
collection), which would then print an error message. This API allows
the specific behaviour.
@feature
export EO_LIFECYCLE_BACKTRACE=0
This means that the lifecycle can be tracked but we don't care about the
backtraces. Related to some recent eo_debug patches.
If an object still has a parent inside the del intercept, we shouldn't
reset the "parent_sunk" flag. This would indeed break logic as
parent_sunk == false implies that a parent was never set, which means
the parent must be null. Right now this case isn't used but it can be
imagined with caches of evas objects (they should remain parented to
their evas).
@fix
efl_data_scope_safe_get() is often used to assert that an object is of a
certain type, but assuming it may be NULL or not of the required type.
This means that encountering an invalid type is an error handled by the
caller (we return NULL), and shouldn't print out extra ERR() logs.
This fixes issues with E run under eo_debug. E's code was safe as it's
using evas_object_smart_data_get() and verifies whether the returned
value is NULL (which is expected for a rectangle, for instance).
@fix
This is done via the syntax (not documented yet):
EO_LIFECYCLE_DEBUG=N:<class names>
Where N is:
1. new/free
2. new/free/ref/unref
3. new/free/ref/unref/reuse
@feature
It's not enough to check the init generation count, if the cache data is
not reset properly.
This patch adds safety in two ways:
1. Actually reset the op call cache when we detect a new generation
2. Verify that we don't return NULL function pointer
The 1st point is absolutely necessary, 2nd is optional (extra cost of
one if()).
@fix
It's a complex struct but defined in EO as a simple struct. ABI-wise
it's equivalent to Eina_Rectangle. Some macros that use Eina_Rectangle
also work on Eina_Rect out of the box, most of the code dealing with
x,y,w,h will require no modifications either.
But Eina_Rect provides direct access to a size or position 2d component,
as well as the usual x,y,w,h. The field "rect" is provided as a
convenience for code dealing with both Eina_Rectangle and Eina_Rect. We
may or may not require it.
Note: Size2D could use unsigned values but I have spotted a few places
in the code that actually use -1 to indicate invalid size (as opposed to
0x0).
@feature
This commit adds the EO support for the new future infra.
From now on there's no need to efl_future_link()/efl_future_unlink()
object and futures since the new API already handles that internally.
There is a problem with the previous version. The object can still be
alive due to the use of manual_free in evas. So you wouldn't be able
for example to remove a callback from an object that hasn't been
destroyed yet. If that callback is triggered by the destruction
of the object, you would end up with an unexpected and impossible to
prevent effect of access after free on a callback that you had removed.
Not sure if that still solve the original problem that the code was
trying to prevent in Ecore_Evas.
This makes sure the object is actually still alive and kicking before
returning any data. Otherwise the "safe" word is a bit of an abuse...
Ref T5869
@fix
In some case, detected during eo test suite, the vtable does fail to
be fully assigned, but it is still being assigned as the new vtable.
Of course when later destroying it, it has already been freed. Leading
to a double free.
This will include the following information, by default:
- class name
- whether the class is an override
- eo id (pointer)
- refcount
- name if one was set (Efl.Object property)
This also supports classes, which is why it's an EAPI in eo.c
and not only a method of Efl.Object
This can be overriden by subclasses using the empty method
Efl.Object.debug_name_override.get
If the function is overriden, then the returned string is used
as is and so it is left to the subclass to include all the
necessary information (as above). This can easily be achieved
by calling efl_debug_name_get(efl_super()) and then concatenating
the strings.
Think of this function as something like Java's toString(), but
only for debugging (i.e. a string class should not just return
its string value).
@feature
This allows two things:
- adding new override functions on an object that already has
overrides
- resetting a specific function (or list of functions) to the
parent class implementation by passing NULL as implementation
Fixes T5580
@feature
Explicit lock / unlock of the shared mutex mixed with implicit
lock / unlock when accessing the internal Eo_Object data lead
to uncaught issues such as these.
This was found by trying to run E with gfx filters under eo_debug.
eo_function(NULL) always leads to a no-operation. A this
point it is basically considered the normal operation
with EO to just silently ignore NULL errors.
If the API function "eo_function" belongs to a class that
has not been loaded yet (eg. no object of this type has
been created yet), then the op associated to the API func
may not be defined yet.
Fixes T5715
This focus on the domain and ID bits is very confusing. Let's
keep it at the end of the message, and also try to guess whether
the object may have been deleted or simply doesn't belong to the
current thread.
This is a safe version of efl_data_scope_get, meaning that it will
return NULL if the object is not of the required type, or if there
is no data for that class, or if the given class was used as an
interface (and isn't a mixin).
@feature
This makes it work like C++ dynamic_cast<> operator, so that
the return value will be NULL if the object is not an instance
of the given class.
In case of efl_super() we don't do it as efl_super is used A LOT
inside EFL itself (all constructors & destructors, for a start)
and efl_isa is in fact a bit expensive. efl_cast isn't really used
and is intended to be something like dynamic_cast.
For @cedric :)
There have been cases where the logic of _event_callback_call break'ed
too early in the event submission.
Reason for that was the line ((const unsigned char *) desc -
(const unsigned char *) it->desc) producing a overflow.
This means the if statement
if (!legacy_compare &&
((const unsigned char *) desc - (const unsigned char *) it->desc) < 0)
was true while the pointer desc was smaller than it->desc, which means
the event subscription got aborted, even if it should not.
This turned out on two 32 bit maschines. And led to not rendering apps
anymore.
It was introduced by commit in 605fec91ee.
@fix
Carsten Haitzler
Jean-Philippe Andre
Marcel Hollerbach
Andy Williams
Cedric Bail
Daniel Kolesa
Lauro Moura
Marcel Hollerbach
Vincent Torri
Jean Guyomarc'h
Gustavo Sverzut Barbieri
Guilherme Iscaro
Daniel Zaoui