Hi Rich,
Thanks again for your detailed reply. I spend some hours yesterday doing a
simple little change from int to ptrdiff_t in the code of
generic_pen::difference_type. The compiler complained to build in a huge
amount of parts of the code. It seems that the solution isn't that trivial
as I believed at first stage. It is a whole complete rework on the memory
access of the synfig-core to make it 32-64 bit compliant.
Currently I'm in the middle of another huge change that is add support for
Cairo libraries in the synfig-core renderer of the application. It implies
to make a new infrastructure of rendering that cannot be fully used until
finished.
The reason on trying to fix the huge image size handling bug was that it is
a blocker for the Morevna Project (what is the current proof of pudding of
Synfig) and I wanted to help them. Unfortunately the solution doesn't seems
to be trivial and so I do prefer to take it apart and continue with the
Cairo render implementation.
So I've decided to give up on the 64-32 bit compliance bug and get back
into Cairo-Synfig project.
It was a pleasure to get your feed back on this matter and I count with you
for further specific question on this and similar things.
Nice to meet you! Cheers!
2012/9/14 Rich Talbot-Watkins <[email protected]>
> Hi Carlos,
>
>
> So, following your first tip I changed the first occurrence of the
> pointer difference:
>
> - typedef int difference_type;
> + typedef ptrdiff_t difference_type;
>
> and so, the difference of the two pointers works correctly (better) in
> this code:
>
> difference_type operator-(const self_type &rhs)const
> { assert(data_); return ((char*)data_-(char*)rhs.data_-1)/pitch_+1; }
>
>
> Yes, C++ will return a type of ptrdiff_t when you subtract one pointer
> from another, so this is correct. The divide afterwards is ok, because
> internally ptrdiff_t is just a signed int. As with all of this problem,
> the reason to use it is for portability when running on different platforms
> which can address different ranges of memory.
>
>
> Neat!
>
> But, immediately I go to the following code:
>
> self_type operator+(const difference_type &rhs)const
> {
> assert(data_);
> self_type ret(*this);
> ret.inc(rhs);
> return ret;
> }
>
> Where difference_type is used.
> So I think: if difference_type is now ptrdiff_t, then the ret.inc(rhs);
> call should expect to receive the same type, right?
>
>
> Yes, exactly. inc() takes an offset as a parameter, so this too should be
> a ptrdiff_t. C++ will implicitly cast anything added to a pointer to a
> ptrdiff_t.
>
>
>
> so I looked the code of the inc function and replaced the int with the
> same type it is used to pass it:
>
> -void inc(int n) { assert(data_); data_ = (pointer)((char*)data_ +
> n*pitch_); }
> +void inc(difference_type n) { assert(data_); data_ =
> (pointer)((char*)data_ + n*pitch_); }
>
> but wait a second: inc is doing a pointer increment of n*pitch bytes,
> shouldn't it be a size_t type???
>
>
> It depends if inc is allowed to take a negative value or not. size_t and
> ptrdiff_t are obviously very closely related, but size_t should be used as
> a zero-based array index (i.e. from the start of an array), whereas
> ptrdiff_t is more for pointer arithmetic. In this case, I would leave it
> as a ptrdiff_t because it is (probably) allowed to have a negative value.
>
>
>
> Moreover, the operator[](int i) isn't doing something very similar?
> shouldn't I use size_t there?
>
> -reference operator[](int i)const { assert(data_); return *(pointer)(
> (char*)data_+pitch_*i ); }
> + reference operator[](size_t i)const { assert(data_); return *(pointer)(
> (char*)data_+pitch_*i ); }
>
>
> I would use size_t, yes. If operator [] is overloaded in the conventional
> way, i.e. to retrieve a zero-based array element (or in this case, a whole
> row), then size_t is correct.
>
>
> This confusion with ptrdiff_t and size_t is getting me crazy. In this
> particular case:
> if I use void inc(size_t) instead of void inc(ptrdiff_t) would it convert
> the values correctly?
>
>
> The easiest thing is to consider whether the value needs to be signed or
> unsigned; ptrdiff_t is signed, and size_t is unsigned.
>
>
> I've done changes in the direction of replace with ptrdiff_t ad size_t
> where I believe it should be
> in the generic_pen and everything leads to a instant crash. I'll post a
> diff later if you want but I first want to
> understand when use size_t or ptrdiff_t
>
>
> Well, even if you've changed the class, there can still be problems in any
> code that is calling the class's methods, e.g. if some code is calling
> myGenericPen[i], but i in the calling code is defined as an int, rather
> than a size_t, then this is going to break everything. If i becomes
> bigger than INT_MAX, and overflows round to a negative value, and then this
> is cast to a size_t, this will become an enormous positive index (e.g.
> 0xffffffffffffff86) which will also cause a crash. Unfortunately at this
> stage, you're going to have to fall back on a debugger. It would also be
> worth putting some range checking in the array accessors themselves, so you
> can see if some wild values are being passed in.
>
>
>
> The +1,-1 thing is very strange to me too. It seems to be some sort of
> hack to fix other rounding error
> that it might happen in the procedures that uses the generic pen. It has
> been there since 2005 and nobody has changed it.
> Removing it doesn't seem to hurt at the render but the checkerboard layer
> seems to be displaced one pixel down-right more than it is
> currently displaced (it has a bug of location of the checkerboard)
>
>
> Ufff, a hack to work around a bug. Horrible. But I see it every day in
> my work as well, so it's not really anything new :)
>
>
>
> Last word. The reason of looking the code is that when we are rendering
> one special kind of layer (Warp) it needs to render the layers below
> in a huge size and then, since a certain image size, the operator[] form
> the generic pen fails with BAD_ACCESS. The key to make me think on a code
> bug was that it
> always crashed when the size of the image in bytes is near the size of the
> maximum int value and so the int is overflow to negative and then it is
> trying to access one illegal address.
> Well, this is my theory. ^__^
>
>
> Yes, it sounds perfectly plausible! I would continue by putting range
> checks in the accessors (even just to log out-of-range values), and then
> start to look at other parts of the code which use this class, and ensure
> that they are using the correct types. Unfortunately 32/64-bit portability
> is not easy (and there is loads of information just a Google away on it),
> but at least by understanding the basic types, and not assuming anything
> about the size of a given type, you can help to make the code more
> compliant on either platform.
>
> Buena suerte!
> Rich
>
>
>
--
Carlos
http://synfig.org
------------------------------------------------------------------------------
Got visibility?
Most devs has no idea what their production app looks like.
Find out how fast your code is with AppDynamics Lite.
http://ad.doubleclick.net/clk;262219671;13503038;y?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
Synfig-devl mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/synfig-devl
