[poppler] Compiling poppler with clang

Adam Reichold adamreichold at myopera.com
Sun Nov 18 23:16:59 PST 2012 

-----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 poppler at lists.freedesktop.org
>>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>
>>>>>>>>>>>
>>>>>>>>>>> 
> _______________________________________________ poppler
> 
>>>>>>>>>> mailing list poppler at lists.freedesktop.org 
>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 
.
>>>>>>>>> 
>>>>>>>>> _______________________________________________ 
>>>>>>>>> poppler mailing list poppler at lists.freedesktop.org
>>>>>>>>>  
>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 
_______________________________________________ poppler
>>>>>>>> mailing list poppler at lists.freedesktop.org 
>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> 
_______________________________________________ poppler
>>>>>>> mailing list poppler at lists.freedesktop.org 
>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>> 
>>>> _______________________________________________ poppler 
>>>> mailing list poppler at lists.freedesktop.org 
>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> 
> _______________________________________________ poppler mailing 
> list poppler at lists.freedesktop.org 
> 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-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: function_copy_constructor.patch
Type: text/x-patch
Size: 7995 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20121119/7ac23790/attachment.bin>

More information about the poppler mailing list