[poppler] Compiling poppler with clang

Adam Reichold adamreichold at myopera.com
Mon Nov 19 23:05:56 PST 2012 

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Albert,

I am beginning to understand why one would not touch this, if it isn't
necessary. :-(

I redid the changes and found that I missed to copy
"SampledFunction::nSamples". Sorry. Tested it against the PDF file you
linked.

On a different but related topic: I tested the first patch using the
test data repository and "make check", but the number of documents in
this set seems limited. But be more helpful (I am also thinking of
Thomas' threading patches), is they a way to get or automatically
generate the collection of PDF files covered by the bug tracker? (Or
whatever else you use for full regression testing.)

Best regards, Adam.

Am 19.11.2012 22:44, schrieb Albert Astals Cid:
> El Dilluns, 19 de novembre de 2012, a les 08:16:59, Adam Reichold
> va escriure: 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.)
> 
>> Can you have a look at
>> https://bugs.freedesktop.org/attachment.cgi?id=18330
> 
>> The dash line that divides the the green rectangular area changes
>> from grayish to black when applying your patch.
> 
>> Cheers, Albert
> 
> 
> 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
> 
> _______________________________________________ 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)

iQEcBAEBAgAGBQJQqyvUAAoJEPSSjE3STU34yhgH/3LnXGrOTPOZHOSCpi8/u4gs
Tku7C53A9bmapic70PMEZuKFsq2D9j+e1LsM4xx0lyKiu/vGXLZjGnOrAlbO2BTC
xKw7NfeIm7BJG7NuiEahbG3qKFVpNuxb50s/X1m7qsPOXxzJd2mbJmV17NSW/Je0
EgubzRE+JBpnRXAqDiU22Lf7HsfKaAzcN1qRD6Rh6P7utvS5CRNJBlU0VMucNnEq
6i/hFdeKyN6gewu9hCkAGnM0fsYMuHaxbjg2RjqxTNfDwlvkV07WHn0xYPcY3HbD
KYKOm9ZxG+eKh66AH4s6jjPHRH7hTHBTJnziNOU/rCZK1PbyH0y0QaaSNzNEXLM=
=TGDM
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: function_copy_constructor.patch
Type: text/x-patch
Size: 5602 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20121120/a75b9bb7/attachment.bin>

More information about the poppler mailing list