View Issue Details

IDProjectCategoryView StatusLast Update
0029769CommunityOCCT:Foundation Classespublic2019-09-23 08:30
ReporterAaron Michalk Assigned Toabv 
PrioritynormalSeveritycrash 
Status closedResolutionfixed 
PlatformVC++ 2017 64 bitOSWindows 
Product Version7.2.0 
Target Version7.4.0Fixed in Version7.4.0 
Summary0029769: Foundation Classes - Uninitialized data with BSplCLib_Cache, BSplSLib_Cache
DescriptionUsing malloc instead of calloc results in accessing invalid memory when evaluating spline cache. This is due to uninitialized data.
Steps To Reproduce1. Set MMGT_CLEAR to false (to use malloc instead of calloc in Standard.cxx).

Note that in default environment script env.bat by default sets MMGT_CLEAR to 1, thus it should be edited to set it to 0.

2. Exercise the spline evaluation caching, for instance, in DRAW run:

Draw[]> test demo samples cad
Additional information
and documentation updates
Copying the member variable initialization from the default constructor to the other constructors of BSplCLib_Cache, BSplSLib_Cache fixes the problem.
TagsNo tags attached.
Test case numberNot needed

Attached Files

  • warnings.txt (31,909 bytes)
  • CR29769.zip (49,050 bytes)

Relationships

related to 0024208 closedapn Open CASCADE Optimization of the edge-edge and edge-face intersection algorithms 
parent of 0030990 closedbugmaster Community Foundation Classes - unexpected change in numerical results on bsplines after 0029769 
child of 0028203 newvpozdyayev Open CASCADE Coding rules - check testing with MMGT_CLEAR turned OFF 
child of 0024682 closedbugmaster Open CASCADE Move out B-spline cache from curves and surfaces to dedicated classes BSplCLib_Cache and BSplSLib_Cache 

Activities

git

2018-06-12 22:06

administrator   ~0076725

Branch CR29769 has been created by abv.

SHA-1: efbfdf32a21e07a8e5d1976b4645a423d4c75d42


Detailed log of new commits:

Author: abv
Date: Sun Jun 10 22:40:12 2018 +0300

    0029769: Uninitialized data with BSplCLib_Cache, BSplSLib_Cache
    
    Implementation of classes BSplCLib_Cache and BSplSLib_Cache is revised:
    - Common functionality dealing with spans along one parametric direction is separated to new struct BSplCLib_CacheParams
    - Empty constructors are removed; copying is prohibited
    - Code reconsidering degree and other parameters on each call to BuildCache() is eliminated; curve parameters must be the same in constructor and all calls to BuildCache()
    - Extra call to BuildCache() from constructor is eliminated

abv

2018-06-14 09:36

manager   ~0076756

Fix is pushed to CR29769, please review.

Jenkins tests have passed, see job CR29769-master-abv. Note that there is apparently 30-50% performance improvement on tests bugs modalg_5 bug24208_*.

abv

2018-06-14 09:47

manager   ~0076758

It should be noted that surprisingly implementation of similar methods BuildCache() in BSplSLib and BSplCLib uses different convention for definition of parametric range of the span and parameter within the span. In BSplCLib, span is defined by start parameter and span length, while in BSplSLib, it is defined by mid-parameter and half length. Local parameter is normalized to [0, 1] and [-1, 1], respectively. This perhaps should be revised for consistency.

abv

2018-06-14 22:18

manager   ~0076793

Note that this fix does not make OCCT safe to use with MMGT_CLEAR=0; there are other places where uninitialized memory is used in such setting.

azv

2018-07-13 18:10

administrator   ~0077585

Reviewed.

bugmaster

2018-07-16 13:47

administrator   ~0077661

Combination -
OCCT branch : CR29769 SHA - efbfdf32a21e07a8e5d1976b4645a423d4c75d42
Products branch : master SHA - 82570c1f4b0e27eb09789f573087eef089260f59
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian70-64:
OCCT
Total CPU difference: 16883.279999999726 / 17011.039999999866 [-0.75%]
Products
Total CPU difference: 7418.390000000034 / 7441.7800000000125 [-0.31%]
Windows-64-VC10:
OCCT
Total CPU difference: 16784.007189098527 / 16821.806231398525 [-0.22%]
Products
Total CPU difference: 8202.96938279988 / 8237.071201399893 [-0.41%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

bugmaster

2018-07-17 18:24

administrator   ~0077752

Last edited: 2018-07-18 19:15

See attached warnings.txt for new warnings

git

2018-07-17 22:04

administrator   ~0077760

Branch CR29769_1 has been created by abv.

SHA-1: 85180789252059b3b2dc8dede60a072d2cdf671e


Detailed log of new commits:

Author: abv
Date: Sun Jun 10 22:40:12 2018 +0300

    0029769: Uninitialized data with BSplCLib_Cache, BSplSLib_Cache
    
    Implementation of classes BSplCLib_Cache and BSplSLib_Cache is revised:
    - Common functionality dealing with spans along one parametric direction is separated to new struct BSplCLib_CacheParams
    - Empty constructors are removed; copying is prohibited
    - Code reconsidering degree and other parameters on each call to BuildCache() is eliminated; curve parameters must be the same in constructor and all calls to BuildCache()
    - Extra call to BuildCache() from constructor is eliminated

abv

2018-07-18 15:14

manager   ~0077784

Branch CR29769_1 eliminates compiler warnings and is on master; please consider

bugmaster

2018-07-18 19:14

administrator  

warnings.txt (31,909 bytes)

git

2018-07-21 20:02

administrator   ~0077880

Branch CR29769_1 has been deleted by inv.

SHA-1: 85180789252059b3b2dc8dede60a072d2cdf671e

git

2018-07-21 20:02

administrator   ~0077892

Branch CR29769 has been deleted by inv.

SHA-1: efbfdf32a21e07a8e5d1976b4645a423d4c75d42

BenjaminBihler

2018-11-14 14:40

developer   ~0081087

I compute geodesics on TopoDS_Faces by solving an ordinary differential equation that takes use of the second derivatives of the face parameterizations. The resulting curves should be as straight as possible on the faces, therefore I use a unit test to check their curvature.

The resolution of this issue disturbs my curve computations. The unit test reports a curve to have high in-plane curvature, which has never happened before this fix. So from my point of view this resolution seems not only to modify the OCCT behaviour, but it seems to deteriorate it (I know that this might be a false impression).

May I therefore ask a few questions?
- Is a change in the behaviour (especially the derivative computations) intentional?
- Why is it intentional, if it is? Because the former behaviour was wrong? Because of the former use of unitialized data?
- Shouldn't the former behaviour then have been somehow random? (It was very stable and has always seemed right in my test case.)

I could try to extract a test case that shows the behaviour change. But unfortunately according to my experience the "micro-behaviour" might be hardware-dependent because of computation precisions. Still I could try.

Thank you very much for some input.

Benjamin

BenjaminBihler

2018-11-14 19:32

developer   ~0081100

I have done further checks. What behaves differently here between both OCCT versions is curve concatenation with GeomConvert_CompCurveToBSplineCurve. With the changes introduced by this issue, a certain concatenated curve is slightly shorter (is has a slightly lower LastParameter()) than it has been with the previous OCCT commit. The other differences that I see in my program might be just consequences of this first difference.

I will only try to create a minimum test case showing the behaviour change, if you tell me that the behaviour should actually have stayed the same.

Benjamin

abv

2019-08-27 07:08

manager   ~0086455

Hello Benjamin,

Sorry I overlooked your last comments...
Indeed, the change by intention should have no impact on computations (and we see no differences in our tests, as expected), thus if you have some test case that demonstrates the difference, please provide it.

Andrey

BenjaminBihler

2019-08-27 16:20

developer  

CR29769.zip (49,050 bytes)

BenjaminBihler

2019-08-27 16:20

developer   ~0086467

Hello Andrey,

I was wrong with my assumption that the code changes for this issue would modify curve lengths. The observed curve length change has actually been just a consequence of another change.

I have attached the file CR29769.zip. It contains a reproducible example (Main.cpp), that reads in a face, creates a 2d spline curve on the face and converts it to a 3d curve. The latter step is where the actual difference between the OCCT revisions happens.

Some information is printed to the console and it is expected that this information stays always the same. The file 29769-1.txt contains the program output when linking against OCCT after doing

git checkout 0a96e0bb~1

and the file 29769.txt contains the program output when linking against OCCT after doing

git checkout 0a96e0bb

and building the libraries.

Please keep in mind that I use mingw-w64 with g++ 9.2.0 here. I do hope very much that you can still reproduce the changes with your compiler(s).

Benjamin

BenjaminBihler

2019-08-27 16:23

developer   ~0086468

One more remark: you should obviously do a diff to see the differences. Yes, they are very small. Still they add up to noticeable changes here.

abv

2019-09-21 10:32

manager   ~0087378

I have created a new issue 0030990 to treat the last reported problems.

abv

2019-09-23 08:30

manager   ~0087436

Hello Benjamin,

I have investigated the issue and found that results given on current master are valid while results on version before 0029769 were slightly inconsistent due to bug in b-spline evaluation. Please see 0030990 for details.

I am looking forward for your feedback in 0030990.

Andrey

Related Changesets

occt: master 0a96e0bb

2018-06-10 19:40:12

abv


Committer: bugmaster Details Diff
0029769: Uninitialized data with BSplCLib_Cache, BSplSLib_Cache

Implementation of classes BSplCLib_Cache and BSplSLib_Cache is revised:
- Common functionality dealing with spans along one parametric direction is separated to new struct BSplCLib_CacheParams
- Empty constructors are removed; copying is prohibited
- Code reconsidering degree and other parameters on each call to BuildCache() is eliminated; curve parameters must be the same in constructor and all calls to BuildCache()
- Extra call to BuildCache() from constructor is eliminated
Affected Issues
0029769
mod - src/BSplCLib/BSplCLib_Cache.cxx Diff File
mod - src/BSplCLib/BSplCLib_Cache.hxx Diff File
add - src/BSplCLib/BSplCLib_CacheParams.hxx Diff File
mod - src/BSplCLib/FILES Diff File
mod - src/BSplSLib/BSplSLib_Cache.cxx Diff File
mod - src/BSplSLib/BSplSLib_Cache.hxx Diff File
mod - src/Geom2dAdaptor/Geom2dAdaptor_Curve.cxx Diff File
mod - src/GeomAdaptor/GeomAdaptor_Curve.cxx Diff File
mod - src/GeomAdaptor/GeomAdaptor_Surface.cxx Diff File

Issue History

Date Modified Username Field Change
2018-05-14 23:01 Aaron Michalk New Issue
2018-05-14 23:01 Aaron Michalk Assigned To => abv
2018-06-05 16:59 abv Target Version => 7.4.0
2018-06-12 22:06 git Note Added: 0076725
2018-06-13 10:11 kgv Relationship added child of 0028203
2018-06-14 09:36 abv Note Added: 0076756
2018-06-14 09:36 abv Status new => resolved
2018-06-14 09:36 abv Assigned To abv => azv
2018-06-14 09:36 abv Relationship added related to 0024208
2018-06-14 09:37 abv Relationship added child of 0024682
2018-06-14 09:47 abv Note Added: 0076758
2018-06-14 22:18 abv Note Added: 0076793
2018-06-14 22:18 abv Steps to Reproduce Updated
2018-07-13 18:10 azv Note Added: 0077585
2018-07-13 18:10 azv Assigned To azv => bugmaster
2018-07-13 18:10 azv Status resolved => reviewed
2018-07-16 13:47 bugmaster Note Added: 0077661
2018-07-16 13:47 bugmaster Status reviewed => tested
2018-07-16 13:51 bugmaster Test case number => Not needed
2018-07-17 18:24 bugmaster Note Added: 0077752
2018-07-17 18:24 bugmaster Assigned To bugmaster => abv
2018-07-17 18:24 bugmaster Status tested => assigned
2018-07-17 22:04 git Note Added: 0077760
2018-07-18 15:14 abv Note Added: 0077784
2018-07-18 15:14 abv Assigned To abv => bugmaster
2018-07-18 15:14 abv Status assigned => feedback
2018-07-18 19:14 bugmaster File Added: warnings.txt
2018-07-18 19:15 bugmaster Note Edited: 0077752
2018-07-21 18:39 bugmaster Changeset attached => occt master 0a96e0bb
2018-07-21 18:39 bugmaster Status feedback => verified
2018-07-21 18:39 bugmaster Resolution open => fixed
2018-07-21 20:02 git Note Added: 0077880
2018-07-21 20:02 git Note Added: 0077892
2018-11-14 14:40 BenjaminBihler Note Added: 0081087
2018-11-14 19:32 BenjaminBihler Note Added: 0081100
2018-11-14 19:33 BenjaminBihler Assigned To bugmaster => abv
2018-11-14 19:33 BenjaminBihler Status verified => feedback
2018-11-14 19:33 BenjaminBihler Resolution fixed => reopened
2019-08-27 07:08 abv Note Added: 0086455
2019-08-27 07:09 abv Assigned To abv => BenjaminBihler
2019-08-27 16:20 BenjaminBihler File Added: CR29769.zip
2019-08-27 16:20 BenjaminBihler Note Added: 0086467
2019-08-27 16:20 BenjaminBihler Assigned To BenjaminBihler => abv
2019-08-27 16:23 BenjaminBihler Note Added: 0086468
2019-09-04 16:15 kgv Summary Uninitialized data with BSplCLib_Cache, BSplSLib_Cache => Foundation Classes - Uninitialized data with BSplCLib_Cache, BSplSLib_Cache
2019-09-21 10:29 abv Relationship added parent of 0030990
2019-09-21 10:30 abv Status feedback => verified
2019-09-21 10:30 abv Resolution reopened => fixed
2019-09-21 10:32 abv Note Added: 0087378
2019-09-23 08:30 abv Note Added: 0087436