[poppler] Compiling poppler with clang

Albert Astals Cid aacid at kde.org
Thu Nov 22 11:30:42 PST 2012 

El Dijous, 22 de novembre de 2012, a les 08:40:26, Adam Reichold va escriure:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hello,
> 
> Somehow, I saw this one coming... :-) There were two calls to base
> constructors missing. Fixed and appended.
> 
> But the linked document still crashes Poppler. And for me, it does so
> with my patch applied or not. Master itself seems to crash in Splash,
> for example at:
> 
> 0 __memset_x86_64 /usr/lib/libc.so.6 0x7ffff5388a2a
> 1 Splash::clear Splash.cc:1929 0x7ffff5027693
> 2 SplashOutputDev::startPage SplashOutputDev.cc:1422 0x7ffff4f563cb
> 3 Gfx::Gfx Gfx.cc:564 0x7ffff4fa02e9
> 4 Page::createGfx Page.cc:486 0x7ffff4fd964b
> 5 Page::displaySlice Page.cc:515 0x7ffff4fd9898
> 6 Poppler::Page::renderToImage poppler-page.cc:264 0x7ffff7ba7220
> 
> This is
> 
> if (bitmap->alpha) {
>   memset(bitmap->alpha, alpha, bitmap->width * bitmap->height);
> }
> 
> in Splash::clear. I also encountered other crashes connected to the
> Splash bitmap data. I'd say we should figure out this problem before I
> continue to fool around with compiler warnings.

Interesting, the file works fine for me :S

Can you run valgrind on it?

Cheers,
  Albert

> 
> Regards, Adam.
> 
> Am 21.11.2012 23:27, schrieb Albert Astals Cid:
> > El Dimarts, 20 de novembre de 2012, a les 08:05:56, Adam Reichold
> > va escriure: 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.
> > 
> >> Now crashes in http://www.it.piaggio.com/media/mp3hybdepita.pdf
> >> 
> >> Cheers, Albert
> > 
> > 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
> > 
> > _______________________________________________ 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)
> 
> iQEcBAEBAgAGBQJQrdbqAAoJEPSSjE3STU34XlgH/A6URICsLt1L+ovPJMYwrNJm
> 4gaGM/a5TV4x3scmHrieSHpAWxaX+ra9y2dHZCz6DWLuJnh84sGyuTiN2qlXUH3z
> 1LdUaB8NX04gAr7R2Vqn+qC0/LKeMak55udc0ZD10ROSqkzO6pE8b9T4Hf1a+hPe
> ZzAcH6wva8CVRYxdILCzJSvOLn8tQoPPOMQN63ePBQ+JgzVS8Dnp4uXn+4jDON1V
> a550RXEO3wJvuifh4DBsYpIoaOlOBDw7k2tJqJGdaXjRmAuulXNql97aFM80XOJ0
> T1MWesOH+MBS51f+ai16Z7rhP7UW3d14Vhv19N0BoaLwnYPLKWDZCbh/M3smVlI=
> =MiU1
> -----END PGP SIGNATURE-----

More information about the poppler mailing list