[poppler] Compiling poppler with clang

Albert Astals Cid aacid at kde.org
Wed Nov 21 14:08:02 PST 2012 

El Dimarts, 20 de novembre de 2012, a les 08:05:56, Adam Reichold va escriure:
> -----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.

Good, let's test against the other ones.

> 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. 

Yeah, that's worthless, they are there only for the unittests in the qt4 code

> 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.)

That'd would be useful yes, but kind of time consuming to make, if you want i 
can open an ftp to my machine and you can dowload the 1400 files i have in my 
test suite (which i can't make *really* public since i don't own the rights to 
distribute them, so if someone asks me you never got the files from me ;-))

Cheers,
  Albert

> 
> 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-----

More information about the poppler mailing list