basegfx/source/polygon/b3dpolygon.cxx | 57 +++------------------------------
include/basegfx/polygon/b3dpolygon.hxx | 2 -
2 files changed, 7 insertions(+), 52 deletions(-)
New commits:
commit 115f60037409c511ae01006dedd32306ddf274ff
Author: Noel Grandin <noel.gran...@collabora.co.uk>
AuthorDate: Wed Jul 10 11:12:04 2024 +0200
Commit: Noel Grandin <noel.gran...@collabora.co.uk>
CommitDate: Wed Jul 10 19:04:37 2024 +0200
tsan: ImplB3DPolygon cannot have mutable state
because it lives inside a cow_wrapper, which means multiple threads will
see inconsistent state.
WARNING: ThreadSanitizer: data race (pid=1448100)
Read of size 1 at 0x7218002d5a48 by thread T333:
0 ImplB3DPolygon::getNormal()
basegfx/source/polygon/b3dpolygon.cxx:998 (libbasegfxlo.so+0xef47a)
1 basegfx::B3DPolygon::getNormal() const
basegfx/source/polygon/b3dpolygon.cxx:1477 (libbasegfxlo.so+0xe1834)
2
drawinglayer::processor3d::DefaultProcessor3D::impRenderPolyPolygonMaterialPrimitive3D(drawinglayer::primitive3d::PolyPolygonMaterialPrimitive3D
const&) const drawinglayer/source/processor3d/defaultprocessor3d.cxx:424
(libdrawinglayerlo.so+0x1ed388)
3
drawinglayer::processor3d::DefaultProcessor3D::processBasePrimitive3D(drawinglayer::primitive3d::BasePrimitive3D
const&) drawinglayer/source/processor3d/defaultprocessor3d.cxx:531
(libdrawinglayerlo.so+0x1edb6f)
4
drawinglayer::processor3d::BaseProcessor3D::process(drawinglayer::primitive3d::Primitive3DContainer
const&) drawinglayer/source/processor3d/baseprocessor3d.cxx:57
(libdrawinglayerlo.so+0x1e91c1)
5
drawinglayer::processor3d::DefaultProcessor3D::processBasePrimitive3D(drawinglayer::primitive3d::BasePrimitive3D
const&) drawinglayer/source/processor3d/defaultprocessor3d.cxx:543
(libdrawinglayerlo.so+0x1edbe4)
6
drawinglayer::processor3d::BaseProcessor3D::process(drawinglayer::primitive3d::Primitive3DContainer
const&) drawinglayer/source/processor3d/baseprocessor3d.cxx:57
(libdrawinglayerlo.so+0x1e91c1)
7
drawinglayer::primitive2d::ScenePrimitive2D::create2DDecomposition(drawinglayer::geometry::ViewInformation2D
const&) const::Executor::doWork()
drawinglayer/source/primitive2d/sceneprimitive2d.cxx:462
(libdrawinglayerlo.so+0x12469f)
8 comphelper::ThreadTask::exec()
comphelper/source/misc/threadpool.cxx:319 (libcomphelper.so+0x2bd587)
9 comphelper::ThreadPool::ThreadWorker::execute()
comphelper/source/misc/threadpool.cxx:85 (libcomphelper.so+0x2c3930)
10 salhelper::Thread::run() salhelper/source/thread.cxx:39
(libuno_salhelpergcc3.so.3+0x5d22)
11 non-virtual thunk to salhelper::Thread::run()
salhelper/source/thread.cxx:? (libuno_salhelpergcc3.so.3+0x5ed9)
12 threadFunc include/osl/thread.hxx:189
(libuno_salhelpergcc3.so.3+0x6b1e)
13 osl_thread_start_Impl(void*) sal/osl/unx/thread.cxx:237
(libuno_sal.so.3+0xfc937)
Previous write of size 8 at 0x7218002d5a48 by thread T334:
0 operator new(unsigned long)
/home/noel/llvm-project/compiler-rt/lib/tsan/rtl/tsan_new_delete.cpp:64
(discriminator 8) (cppunittester+0x10c8a1)
1 o3tl::cow_wrapper<ImplB3DPolygon,
o3tl::ThreadSafeRefCountingPolicy>::make_unique()
include/o3tl/cow_wrapper.hxx:304 (libbasegfxlo.so+0xfe798)
2 o3tl::cow_wrapper<ImplB3DPolygon,
o3tl::ThreadSafeRefCountingPolicy>::operator->()
include/o3tl/cow_wrapper.hxx:329 (libbasegfxlo.so+0xef0e5)
3 basegfx::B3DPolygon::getNormal() const
basegfx/source/polygon/b3dpolygon.cxx:1477 (libbasegfxlo.so+0xe1825)
4
drawinglayer::processor3d::DefaultProcessor3D::impRenderPolyPolygonMaterialPrimitive3D(drawinglayer::primitive3d::PolyPolygonMaterialPrimitive3D
const&) const drawinglayer/source/processor3d/defaultprocessor3d.cxx:424
(libdrawinglayerlo.so+0x1ed388)
5
drawinglayer::processor3d::DefaultProcessor3D::processBasePrimitive3D(drawinglayer::primitive3d::BasePrimitive3D
const&) drawinglayer/source/processor3d/defaultprocessor3d.cxx:531
(libdrawinglayerlo.so+0x1edb6f)
6
drawinglayer::processor3d::BaseProcessor3D::process(drawinglayer::primitive3d::Primitive3DContainer
const&) drawinglayer/source/processor3d/baseprocessor3d.cxx:57
(libdrawinglayerlo.so+0x1e91c1)
7
drawinglayer::processor3d::DefaultProcessor3D::processBasePrimitive3D(drawinglayer::primitive3d::BasePrimitive3D
const&) drawinglayer/source/processor3d/defaultprocessor3d.cxx:543
(libdrawinglayerlo.so+0x1edbe4)
8
drawinglayer::processor3d::BaseProcessor3D::process(drawinglayer::primitive3d::Primitive3DContainer
const&) drawinglayer/source/processor3d/baseprocessor3d.cxx:57
(libdrawinglayerlo.so+0x1e91c1)
9
drawinglayer::primitive2d::ScenePrimitive2D::create2DDecomposition(drawinglayer::geometry::ViewInformation2D
const&) const::Executor::doWork()
drawinglayer/source/primitive2d/sceneprimitive2d.cxx:462
(libdrawinglayerlo.so+0x12469f)
10 comphelper::ThreadTask::exec()
comphelper/source/misc/threadpool.cxx:319 (libcomphelper.so+0x2bd587)
11 comphelper::ThreadPool::ThreadWorker::execute()
comphelper/source/misc/threadpool.cxx:85 (libcomphelper.so+0x2c3930)
12 salhelper::Thread::run() salhelper/source/thread.cxx:39
(libuno_salhelpergcc3.so.3+0x5d22)
13 non-virtual thunk to salhelper::Thread::run()
salhelper/source/thread.cxx:? (libuno_salhelpergcc3.so.3+0x5ed9)
14 threadFunc include/osl/thread.hxx:189
(libuno_salhelpergcc3.so.3+0x6b1e)
15 osl_thread_start_Impl(void*) sal/osl/unx/thread.cxx:237
(libuno_sal.so.3+0xfc937)
Change-Id: I9a12e2178cfad07f0a23b5dc4c502ce16ef92fb1
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170275
Reviewed-by: Noel Grandin <noel.gran...@collabora.co.uk>
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <armin.le.gr...@me.com>
diff --git a/basegfx/source/polygon/b3dpolygon.cxx
b/basegfx/source/polygon/b3dpolygon.cxx
index ebd9e3f4f7ea..f17f974e2fb7 100644
--- a/basegfx/source/polygon/b3dpolygon.cxx
+++ b/basegfx/source/polygon/b3dpolygon.cxx
@@ -693,41 +693,22 @@ class ImplB3DPolygon
// and may be zero.
std::unique_ptr<TextureCoordinate2D> mpTextureCoordinates;
- // The calculated plane normal. mbPlaneNormalValid says if it's valid.
- ::basegfx::B3DVector maPlaneNormal;
-
// flag which decides if this polygon is opened or closed
bool mbIsClosed : 1;
- // flag which says if maPlaneNormal is up-to-date
- bool mbPlaneNormalValid : 1;
-
-protected:
- void invalidatePlaneNormal()
- {
- if(mbPlaneNormalValid)
- {
- mbPlaneNormalValid = false;
- }
- }
-
public:
// This constructor is only used from the static identity polygon, thus
// the RefCount is set to 1 to never 'delete' this static incarnation.
ImplB3DPolygon()
: maPoints(0),
- maPlaneNormal(::basegfx::B3DVector::getEmptyVector()),
- mbIsClosed(false),
- mbPlaneNormalValid(true)
+ mbIsClosed(false)
{
// complete initialization with defaults
}
ImplB3DPolygon(const ImplB3DPolygon& rToBeCopied)
: maPoints(rToBeCopied.maPoints),
- maPlaneNormal(rToBeCopied.maPlaneNormal),
- mbIsClosed(rToBeCopied.mbIsClosed),
- mbPlaneNormalValid(rToBeCopied.mbPlaneNormalValid)
+ mbIsClosed(rToBeCopied.mbIsClosed)
{
// complete initialization using copy
if(rToBeCopied.mpBColors && rToBeCopied.mpBColors->isUsed())
@@ -748,9 +729,7 @@ public:
ImplB3DPolygon(const ImplB3DPolygon& rToBeCopied, sal_uInt32 nIndex,
sal_uInt32 nCount)
: maPoints(rToBeCopied.maPoints, nIndex, nCount),
- maPlaneNormal(::basegfx::B3DVector::getEmptyVector()),
- mbIsClosed(rToBeCopied.mbIsClosed),
- mbPlaneNormalValid(false)
+ mbIsClosed(rToBeCopied.mbIsClosed)
{
// complete initialization using partly copy
if(rToBeCopied.mpBColors && rToBeCopied.mpBColors->isUsed())
@@ -916,7 +895,6 @@ public:
void setPoint(sal_uInt32 nIndex, const ::basegfx::B3DPoint& rValue)
{
maPoints.setCoordinate(nIndex, rValue);
- invalidatePlaneNormal();
}
void insert(sal_uInt32 nIndex, const ::basegfx::B3DPoint& rPoint,
sal_uInt32 nCount)
@@ -926,7 +904,6 @@ public:
CoordinateData3D aCoordinate(rPoint);
maPoints.insert(nIndex, aCoordinate, nCount);
- invalidatePlaneNormal();
if(mpBColors)
{
@@ -987,15 +964,9 @@ public:
mpBColors.reset();
}
- const ::basegfx::B3DVector& getNormal() const
+ ::basegfx::B3DVector getNormal() const
{
- if(!mbPlaneNormalValid)
- {
- const_cast< ImplB3DPolygon* >(this)->maPlaneNormal =
maPoints.getNormal();
- const_cast< ImplB3DPolygon* >(this)->mbPlaneNormalValid = true;
- }
-
- return maPlaneNormal;
+ return maPoints.getNormal();
}
const ::basegfx::B3DVector& getNormal(sal_uInt32 nIndex) const
@@ -1108,7 +1079,6 @@ public:
return;
maPoints.insert(nIndex, rSource.maPoints);
- invalidatePlaneNormal();
if(rSource.mpBColors && rSource.mpBColors->isUsed())
{
@@ -1168,7 +1138,6 @@ public:
return;
maPoints.remove(nIndex, nCount);
- invalidatePlaneNormal();
if(mpBColors)
{
@@ -1208,12 +1177,6 @@ public:
maPoints.flip();
- if(mbPlaneNormalValid)
- {
- // mirror plane normal
- maPlaneNormal = -maPlaneNormal;
- }
-
if(mpBColors)
{
mpBColors->flip();
@@ -1368,14 +1331,6 @@ public:
void transform(const ::basegfx::B3DHomMatrix& rMatrix)
{
maPoints.transform(rMatrix);
-
- // Here, it seems to be possible to transform a valid plane normal and
to avoid
- // invalidation, but it's not true. If the transformation contains
shears or e.g.
- // perspective projection, the orthogonality to the transformed plane
will not
- // be preserved. It may be possible to test that at the matrix to not
invalidate in
- // all cases or to extract a matrix which does not 'shear' the vector
which is
- // a normal in this case. As long as this is not sure, i will just
invalidate.
- invalidatePlaneNormal();
}
};
@@ -1459,7 +1414,7 @@ namespace basegfx
mpPolygon->clearBColors();
}
- B3DVector const & B3DPolygon::getNormal() const
+ B3DVector B3DPolygon::getNormal() const
{
return mpPolygon->getNormal();
}
diff --git a/include/basegfx/polygon/b3dpolygon.hxx
b/include/basegfx/polygon/b3dpolygon.hxx
index 13f8ed293f59..09544766e662 100644
--- a/include/basegfx/polygon/b3dpolygon.hxx
+++ b/include/basegfx/polygon/b3dpolygon.hxx
@@ -76,7 +76,7 @@ namespace basegfx
void clearBColors();
// Normals interface
- B3DVector const & getNormal() const; // plane normal
+ B3DVector getNormal() const; // plane normal
B3DVector const & getNormal(sal_uInt32 nIndex) const; // normal in
each point
void setNormal(sal_uInt32 nIndex, const B3DVector& rValue);
void transformNormals(const B3DHomMatrix& rMatrix);
