-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hello Albert,
I am sorry about that. It seems like my editor messed up with the spaces since it seems to apply using "--ignore-space-change". I tried to reformat the patch using said option, hopefully this helps. (It still has some whitespace warnings but no more errors.) Regards, Adam. On 18.11.2012 23:01, Albert Astals Cid wrote: > El Dimecres, 14 de novembre de 2012, a les 18:48:26, Adam Reichold > va escriure: Hello again, > > The attached patch should rework the function copy constructors. > >> Doesn't seem to apply on current master, can you please rebase >> it and make sure it applies? > >> Cheers, Albert > > > Best regards, Adam. > > On 14.11.2012 12:58, Albert Astals Cid wrote: >>>> El Dimecres, 14 de novembre de 2012, a les 07:28:21, Adam >>>> Reichold va escriure: Hello Albert, >>>> >>>> Before I try the other classes, could you have a look at the >>>> attached patch for SampledFunction and tell if this is what >>>> you are looking for? >>>> >>>> Would something like memcpy(sampleSize, func->sampleSize, >>>> funcMaxInputs * sizeof(int)); be preferred to the plain >>>> for-loops? >>>> >>>>> Yes. it's what the other functions use, right? >>>> >>>> Regards, Adam. >>>> >>>> P.S.: By the way, is there a reason to use the signature >>>> Function(const Function*) for the "what would be a copy >>>> constructor" instead of "Function(const Function&)"? >>>> >>>>> No clue, ask the guy that wrote them (hint, he's not >>>>> around) >>>>> >>>>> Cheers, Albert >>>> >>>> On 13.11.2012 23:28, Albert Astals Cid wrote: >>>>>>> El Dilluns, 27 d'agost de 2012, a les 22:31:13, Albert >>>>>>> Astals Cid >>>>>>> >>>>>>> va escriure: >>>>>>>> El Dilluns, 27 d'agost de 2012, a les 08:29:40, >>>>>>>> Thomas Freitag va >>>>>>>> >>>>>>>> escriure: >>>>>>>>> On 27.08.2012 00:56, Albert Astals Cid wrote: >>>>>>>>>> El Diumenge, 26 d'agost de 2012, a les 15:48:37, >>>>>>>>>> He Liu va >>>>>>>>>> >>>>>>>>>> escriure: >>>>>>>>>>>>> 5. vtable pointer will be overwritten >>>>>>>>>>>>> Function.cc:422:10: warning: destination >>>>>>>>>>>>> for this 'memcpy' call is a pointer to >>>>>>>>>>>>> dynamic class 'SampledFunction'; vtable >>>>>>>>>>>>> pointer will be overwritten >>>>>>>>>>>>> [-Wdynamic-class-memaccess] >>>>>>>>>>>>> >>>>>>>>>>>>> memcpy(this, func, >>>>>>>>>>>>> sizeof(SampledFunction)); ~~~~~~ ^ >>>>>>>>>>>>> >>>>>>>>>>>>> Function.cc:422:10: note: explicitly cast >>>>>>>>>>>>> the pointer to silence this warning >>>>>>>>>>>>> >>>>>>>>>>>>> At least categrory 5. sound serious to me, >>>>>>>>>>>>> I would never have copied instances of C++ >>>>>>>>>>>>> objects in that way, because it depends on >>>>>>>>>>>>> the compiler and the class if this causes >>>>>>>>>>>>> problems on runtime, s. i.e. >>>>>>>>>>>>> http://weseetips.com/tag/afx_zero_init_object/, >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Note this is memset-ing to 0, not memcpy-ing a >>>>>>>>>>>> class to itself. To be honest i agree >>>>>>>>>>>> memcpy'in a SampledFunction to a >>>>>>>>>>>> SampledFunction is ugly, but i fail to see >>>>>>>>>>>> why it would not work. >>>>>>>>> >>>>>>>>> It works, at least with the actual used compilers. >>>>>>>>> But it works only, because the allocated members of >>>>>>>>> SampledFunction are overwriten after doing the >>>>>>>>> memcpy. And this behaviour makes it just more ugly >>>>>>>>> in my eyes. And this is the same with the other >>>>>>>>> memcpy's in ExponentialFunction, StitchingFunction >>>>>>>>> and PostScriptFunction. And it will work till such >>>>>>>>> time as everybody who changes the class will not >>>>>>>>> forget to do it in the same way. So if You are not >>>>>>>>> willing to change it (or let somebody else change >>>>>>>>> it, I know, never change running code), we could >>>>>>>>> use the hint: explicitly cast the pointer to >>>>>>>>> silence this warning. >>>>>>>> >>>>>>>> I'm fine with someone fixing the code. Less warnings >>>>>>>> is always good, but for master, there's no need to >>>>>>>> introduce a pontential breaker just because clang >>>>>>>> complains. >>>>>>> >>>>>>> So at the end noone volunteered to re-rewite those >>>>>>> functions not to use memcpy over classes with >>>>>>> virtuals? >>>>>>> >>>>>>> Cheers, Albert >>>>>>> >>>>>>>> Cheers, Albert >>>>>>>> >>>>>>>>> Cheers, Thomas >>>>>>>>> >>>>>>>>>>> Hi Albert, >>>>>>>>>>> >>>>>>>>>>> :-) >>>>>>>>>>> >>>>>>>>>>> A pointer of type SampleFunction* could be >>>>>>>>>>> pointing to an instance of a SampleFunction >>>>>>>>>>> sub-class, which has different vtable >>>>>>>>>>> contents. >>>>>>>>>> >>>>>>>>>> No, it could not, SampledFunction does not have >>>>>>>>>> any childs and the function doing that memcopy >>>>>>>>>> is private anyway. >>>>>>>>>> >>>>>>>>>>> As a result, one could construct a >>>>>>>>>>> SampleFunction with >>>>>>>>>>> SampleFunction(SampleFunction *) using a >>>>>>>>>>> pointer to a sub-class instance, and overwrite >>>>>>>>>>> the SampleFunction's vtable address with the >>>>>>>>>>> sub-class's vtable address. >>>>>>>>>>> >>>>>>>>>>> I am not sure if it will lead to any >>>>>>>>>>> bugs/vulnerabilities in this case, but it is >>>>>>>>>>> not safe practice in general. >>>>>>>>>> >>>>>>>>>> Sure, i never said it was. I'm just saying i >>>>>>>>>> don't see why it would not work in our case. >>>>>>>>>> >>>>>>>>>>> Since the vtable structure depends on how the >>>>>>>>>>> compiler is implemented, memcpy or memset on >>>>>>>>>>> object pointers will generally lead to >>>>>>>>>>> undefined behaviors. >>>>>>>>>> >>>>>>>>>> I'm far from a compiler expert, but one would >>>>>>>>>> hope that for a given class the compiler stores >>>>>>>>>> always the "stuff" in the same order in memory, >>>>>>>>>> so again, i fail to see why this should fail in >>>>>>>>>> our case. >>>>>>>>>> >>>>>>>>>> Cheers, >>>>>>>>>> >>>>>>>>>> Albert >>>>>>>>>> >>>>>>>>>>> Thanks. >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> poppler mailing list [email protected] >>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler > >>>>>>>>>>> >>>>>>>>>>> > _______________________________________________ poppler > >>>>>>>>>> mailing list [email protected] >>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> . >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> poppler mailing list [email protected] >>>>>>>>> >>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> _______________________________________________ poppler >>>>>>>> mailing list [email protected] >>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> _______________________________________________ poppler >>>>>>> mailing list [email protected] >>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>> >>>> _______________________________________________ poppler >>>> mailing list [email protected] >>>> http://lists.freedesktop.org/mailman/listinfo/poppler > > _______________________________________________ poppler mailing > list [email protected] > http://lists.freedesktop.org/mailman/listinfo/poppler > -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) iQEcBAEBAgAGBQJQqdzrAAoJEPSSjE3STU34RisH/iqlAtA62bwUMNG9Ym6iRQhk 7RXIqGED7gC7PNMp8gi/dGBH+ziRjVtQfag9oyq4L0QgqEkEHa4hH55WVSnaScik 8qZrRkh9w8XNcG7ROURRbWniKYM0cj8Ao24ps6nJr6GODAS82LpWycFUgz+W4v9J lA0jvkzhpJoS700L5rwUkj5xh+2X98Qy556yt3Hew1T7M1pCOAEWGc794k56/d4q je/zec2xL2oMGP8jrEr01GDqshg+z/+dAqrkg+yJ6M3CzpCvA3iB/kJEQZAMFT1g Jy6o7tn41WTk2FYlWmgPxna6bA+GvR4JT8dxrA47zOcmqTVoOtOfpa0o9+Wfvss= =4ghL -----END PGP SIGNATURE-----
>From 7b1acf78bc13b64f8a576aeb17ab9d4f98ba44d8 Mon Sep 17 00:00:00 2001 From: Adam Reichold <[email protected]> Date: Mon, 19 Nov 2012 08:14:17 +0100 Subject: [PATCH] rework the function copy constructors --- poppler/Function.cc | 73 +++++++++++++++++++++++++++++++++++++++++++++-------- poppler/Function.h | 22 ++++++++-------- 2 files changed, 74 insertions(+), 21 deletions(-) diff --git a/poppler/Function.cc b/poppler/Function.cc index d26aed8..3ce2d94 100644 --- a/poppler/Function.cc +++ b/poppler/Function.cc @@ -178,6 +178,16 @@ GBool Function::init(Dict *dict) { return gFalse; } +Function::Function(const Function* func) { + m = func->m; + n = func->n; + + memcpy(domain, func->domain, funcMaxInputs * 2 * sizeof(double)); + memcpy(range, func->range, funcMaxOutputs * 2 * sizeof(double)); + + hasRange = func->hasRange; +} + //------------------------------------------------------------------------ // IdentityFunction //------------------------------------------------------------------------ @@ -419,13 +429,32 @@ SampledFunction::~SampledFunction() { } } -SampledFunction::SampledFunction(SampledFunction *func) { - memcpy(this, func, sizeof(SampledFunction)); +SampledFunction::SampledFunction(const SampledFunction *func) : Function(func) { + // parameters + + memcpy(sampleSize, func->sampleSize, funcMaxInputs * sizeof(int)); + + memcpy(encode, func->encode, funcMaxInputs * 2 * sizeof(double)); + memcpy(decode, func->decode, funcMaxOutputs * 2 * sizeof(double)); + + memcpy(inputMul, func->inputMul, funcMaxInputs * sizeof(double)); + + ok = func->ok; + + // samples + idxOffset = (int *)gmallocn(1 << m, sizeof(int)); memcpy(idxOffset, func->idxOffset, (1 << m) * (int)sizeof(int)); + samples = (double *)gmallocn(nSamples, sizeof(double)); memcpy(samples, func->samples, nSamples * sizeof(double)); + sBuf = (double *)gmallocn(1 << m, sizeof(double)); + + // buffers + + memcpy(cacheIn, func->cacheIn, funcMaxInputs * sizeof(double)); + memcpy(cacheOut, func->cacheOut, funcMaxOutputs * sizeof(double)); } void SampledFunction::transform(double *in, double *out) { @@ -616,8 +645,14 @@ ExponentialFunction::ExponentialFunction(Object *funcObj, Dict *dict) { ExponentialFunction::~ExponentialFunction() { } -ExponentialFunction::ExponentialFunction(ExponentialFunction *func) { - memcpy(this, func, sizeof(ExponentialFunction)); +ExponentialFunction::ExponentialFunction(const ExponentialFunction *func) : Function(func) { + memcpy(c0, func->c0, funcMaxOutputs * sizeof(double)); + memcpy(c1, func->c1, funcMaxOutputs * sizeof(double)); + + e = func->e; + isLinear = func->isLinear; + + ok = func->ok; } void ExponentialFunction::transform(double *in, double *out) { @@ -761,21 +796,27 @@ StitchingFunction::StitchingFunction(Object *funcObj, Dict *dict, std::set<int> obj1.free(); } -StitchingFunction::StitchingFunction(StitchingFunction *func) { - int i; +StitchingFunction::StitchingFunction(const StitchingFunction *func) : Function(func) { + k = func->k; + ok = func->ok; + + // functions - memcpy(this, func, sizeof(StitchingFunction)); funcs = (Function **)gmallocn(k, sizeof(Function *)); - for (i = 0; i < k; ++i) { + for (int i = 0; i < k; ++i) { funcs[i] = func->funcs[i]->copy(); } + + // parameters + bounds = (double *)gmallocn(k + 1, sizeof(double)); memcpy(bounds, func->bounds, (k + 1) * sizeof(double)); + encode = (double *)gmallocn(2 * k, sizeof(double)); memcpy(encode, func->encode, 2 * k * sizeof(double)); + scale = (double *)gmallocn(k, sizeof(double)); memcpy(scale, func->scale, k * sizeof(double)); - ok = gTrue; } StitchingFunction::~StitchingFunction() { @@ -1184,11 +1225,21 @@ PostScriptFunction::PostScriptFunction(Object *funcObj, Dict *dict) { return; } -PostScriptFunction::PostScriptFunction(PostScriptFunction *func) { - memcpy(this, func, sizeof(PostScriptFunction)); +PostScriptFunction::PostScriptFunction(const PostScriptFunction *func) : Function(func) { + codeSize = func->codeSize; + ok = func->ok; + + // code + code = (PSObject *)gmallocn(codeSize, sizeof(PSObject)); memcpy(code, func->code, codeSize * sizeof(PSObject)); + codeString = func->codeString->copy(); + + // buffers + + memcpy(cacheIn, func->cacheIn, funcMaxInputs * sizeof(double)); + memcpy(cacheOut, func->cacheOut, funcMaxOutputs * sizeof(double)); } PostScriptFunction::~PostScriptFunction() { diff --git a/poppler/Function.h b/poppler/Function.h index 25df133..2cd39d8 100644 --- a/poppler/Function.h +++ b/poppler/Function.h @@ -61,7 +61,7 @@ public: // Initialize the entries common to all function types. GBool init(Dict *dict); - virtual Function *copy() = 0; + virtual Function *copy() const = 0; // Return the function type: // -1 : identity @@ -90,6 +90,8 @@ public: protected: static Function *parse(Object *funcObj, std::set<int> *usedParents); + Function(const Function *func); + int m, n; // size of input and output tuples double // min and max values for function domain domain[funcMaxInputs][2]; @@ -107,7 +109,7 @@ public: IdentityFunction(); virtual ~IdentityFunction(); - virtual Function *copy() { return new IdentityFunction(); } + virtual Function *copy() const { return new IdentityFunction(); } virtual int getType() { return -1; } virtual void transform(double *in, double *out); virtual GBool isOk() { return gTrue; } @@ -124,7 +126,7 @@ public: SampledFunction(Object *funcObj, Dict *dict); virtual ~SampledFunction(); - virtual Function *copy() { return new SampledFunction(this); } + virtual Function *copy() const { return new SampledFunction(this); } virtual int getType() { return 0; } virtual void transform(double *in, double *out); virtual GBool isOk() { return ok; } @@ -140,7 +142,7 @@ public: private: - SampledFunction(SampledFunction *func); + SampledFunction(const SampledFunction *func); int // number of samples for each domain element sampleSize[funcMaxInputs]; @@ -168,7 +170,7 @@ public: ExponentialFunction(Object *funcObj, Dict *dict); virtual ~ExponentialFunction(); - virtual Function *copy() { return new ExponentialFunction(this); } + virtual Function *copy() const { return new ExponentialFunction(this); } virtual int getType() { return 2; } virtual void transform(double *in, double *out); virtual GBool isOk() { return ok; } @@ -179,7 +181,7 @@ public: private: - ExponentialFunction(ExponentialFunction *func); + ExponentialFunction(const ExponentialFunction *func); double c0[funcMaxOutputs]; double c1[funcMaxOutputs]; @@ -197,7 +199,7 @@ public: StitchingFunction(Object *funcObj, Dict *dict, std::set<int> *usedParents); virtual ~StitchingFunction(); - virtual Function *copy() { return new StitchingFunction(this); } + virtual Function *copy() const { return new StitchingFunction(this); } virtual int getType() { return 3; } virtual void transform(double *in, double *out); virtual GBool isOk() { return ok; } @@ -210,7 +212,7 @@ public: private: - StitchingFunction(StitchingFunction *func); + StitchingFunction(const StitchingFunction *func); int k; Function **funcs; @@ -229,7 +231,7 @@ public: PostScriptFunction(Object *funcObj, Dict *dict); virtual ~PostScriptFunction(); - virtual Function *copy() { return new PostScriptFunction(this); } + virtual Function *copy() const { return new PostScriptFunction(this); } virtual int getType() { return 4; } virtual void transform(double *in, double *out); virtual GBool isOk() { return ok; } @@ -238,7 +240,7 @@ public: private: - PostScriptFunction(PostScriptFunction *func); + PostScriptFunction(const PostScriptFunction *func); GBool parseCode(Stream *str, int *codePtr); GooString *getToken(Stream *str); void resizeCode(int newSize); -- 1.8.0
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
