On Wed, May 5, 2021 at 9:34 AM Christopher Creutzig <ccreu...@mathworks.com>
wrote:
> Hi Michal,
>
>
>
> Unfortunately, without memory barriers, DCLP as found in these places is
> not correct on x64 either, and I would not want to rely on
> std::lock::lock() containing fences.
>
I would rather say that it is not correct in C++.
> As I said, the worst thing likely to happen in practice is that two
> encoding objects are allocated, as writing a pointer to aligned memory is
> going to be an atomic operation in virtually all cases.
>
No, this cannot happen because allocation is covered by mutex (unless
PODOFO_MULTI_THREAD is not defined). In practice nowadays nothing bad will
happen on x64. But on ARM it can happen that some thread will access object
properties before it is fully initialised. Because it could see the
object address in cache before another writes into that object because of
missing synchronized-with relationship. In other words its view of order of
memory operations can be different (if first check is not at least
"consume" operation).
I would prefer lazy initialisation. There are encodings which are not used
by everyone. And maybe it would be better to also fix the pre-C++11 version
by removing the first check (or perhaps use version 2 on pre-C++11?). It is
problematic also for older C++ versions on processors like ARM.
>
> Personally, I would either use the safe form where available or just
> initialize all the encoding classes statically:
>
>
>
> Option 1:
>
>
>
> const PdfEncoding* PdfEncodingFactory::GlobalPdfDocEncodingInstance()
>
> {
>
> #if __cplusplus >= 201103L
>
>
>
> static PdfDocEncoding docEncoding;
>
> return &docEncoding;
>
>
>
> #else // pre-C++11
>
>
>
> if(!s_pDocEncoding) // First check
>
> {
>
> Util::PdfMutexWrapper wrapper( PdfEncodingFactory::s_mutex );
>
>
>
> if(!s_pDocEncoding) // Double check
>
> s_pDocEncoding = new PdfDocEncoding();
>
> }
>
>
>
> return s_pDocEncoding;
>
>
>
> #endif
>
> }
>
>
>
>
> Option 2:
>
>
>
> class PODOFO_API PdfEncodingFactory {
>
> …
>
> private:
>
> static const PdfDocEncoding docEnconding;
>
> …
>
> }
>
>
>
> const PdfEncoding* PdfEncodingFactory::GlobalPdfDocEncodingInstance()
>
> {
>
> return &docEncoding;
>
> }
>
>
>
>
> Cheers,
>
> Christopher
>
>
>
> *From:* Michal Sudolsky <sudols...@gmail.com>
> *Sent:* Tuesday, May 4, 2021 17:08
> *To:* Christopher Creutzig <ccreu...@mathworks.com>
> *Cc:* podofo-users@lists.sourceforge.net
> *Subject:* Re: [Podofo-users] DCLP in PoDoFo
>
>
>
> Hi,
>
>
>
> See: https://sourceforge.net/p/podofo/mailman/message/35915862/
>
>
>
> On Mon, May 3, 2021 at 2:05 PM Christopher Creutzig <
> ccreu...@mathworks.com> wrote:
>
> Hi list,
>
>
>
> PdfEncodingFactory.cpp uses a broken form of double-checked locking
> <https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/>
> to initialize its encoding instances. Not a big deal, as these objects
> don’t do much; technically, that is a data race and can lead to undefined
> behaviour, but realistically, I would be surprised to see anything worse
> than a small memory leak, if even that.
>
>
>
> It is undefined behaviour in C++. Actually it should cause problems only
> on weakly ordered processors like ARM. So you should never see it on x64.
>
>
>
>
>
> Does PoDoFo require C++11 or newer (where there are simple fixes
> available)? Will it ever?
>
>
>
> If podofo cannot depend on C++11 then I would suggest to use
> "single"-checked locking (just remove the first check):
>
>
>
> //if(!s_pWinAnsiEncoding) // First check
> //{
> Util::PdfMutexWrapper wrapper( PdfEncodingFactory::s_mutex );
>
> if(!s_pWinAnsiEncoding) // Double check
> s_pWinAnsiEncoding = new PdfWinAnsiEncoding();
> //}
>
> return s_pWinAnsiEncoding;
>
>
>
> Another solution would be to use some additional library that provides
> atomic primitives for older C++ but I do not think that it would be worth
> it in this case.
>
>
>
>
>
>
>
>
>
> Cheers,
>
> Christopher
>
>
>
> The MathWorks GmbH | Friedlandstr.18 | 52064 Aachen | District Court
> Aachen | HRB 8082 | Managing Directors: Bertrand Dissler, Steven D. Barbo,
> Jeanne O’Keefe
>
>
>
> _______________________________________________
> Podofo-users mailing list
> Podofo-users@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/podofo-users
>
> _______________________________________________
> Podofo-users mailing list
> Podofo-users@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/podofo-users
>
_______________________________________________
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users
