[poppler] Compiling poppler with clang
Adam Reichold
adamreichold at myopera.com
Wed Nov 21 23:40:26 PST 2012
-----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.
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-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: function_copy_constructor.patch
Type: text/x-patch
Size: 5636 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20121122/21c7410c/attachment-0001.bin>
More information about the poppler
mailing list