MantisBT - Community
View Issue Details
0029769Community[OCCT] OCCT:Foundation Classespublic2018-05-14 23:012019-09-23 08:30
Aaron Michalk 
abv 
normalcrash 
closedfixed 
VC++ 2017 64 bitWindows10
[OCCT] 7.2.0 
[OCCT] 7.4.0[OCCT] 7.4.0 
Not needed
0029769: Foundation Classes - Uninitialized data with BSplCLib_Cache, BSplSLib_Cache
Using malloc instead of calloc results in accessing invalid memory when evaluating spline cache. This is due to uninitialized data.
1. 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
Copying the member variable initialization from the default constructor to the other constructors of BSplCLib_Cache, BSplSLib_Cache fixes the problem.
No tags attached.
related to 0024208closed apn Open CASCADE Optimization of the edge-edge and edge-face intersection algorithms 
parent of 0030990closed bugmaster Community Foundation Classes - unexpected change in numerical results on bsplines after 0029769 
child of 0028203new kgv Open CASCADE Coding rules - check testing with MMGT_CLEAR turned OFF 
child of 0024682closed bugmaster Open CASCADE Move out B-spline cache from curves and surfaces to dedicated classes BSplCLib_Cache and BSplSLib_Cache 
txt warnings.txt (31,909) 2018-07-18 19:14
https://tracker.dev.opencascade.org/
zip CR29769.zip (49,050) 2019-08-27 16:20
https://tracker.dev.opencascade.org/
Issue History
2018-05-14 23:01Aaron MichalkNew Issue
2018-05-14 23:01Aaron MichalkAssigned To => abv
2018-06-05 16:59abvTarget Version => 7.4.0
2018-06-12 22:06gitNote Added: 0076725
2018-06-13 10:11kgvRelationship addedchild of 0028203
2018-06-14 09:36abvNote Added: 0076756
2018-06-14 09:36abvStatusnew => resolved
2018-06-14 09:36abvAssigned Toabv => azv
2018-06-14 09:36abvRelationship addedrelated to 0024208
2018-06-14 09:37abvRelationship addedchild of 0024682
2018-06-14 09:47abvNote Added: 0076758
2018-06-14 22:18abvNote Added: 0076793
2018-06-14 22:18abvSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=19295#r19295
2018-07-13 18:10azvNote Added: 0077585
2018-07-13 18:10azvAssigned Toazv => bugmaster
2018-07-13 18:10azvStatusresolved => reviewed
2018-07-16 13:47bugmasterNote Added: 0077661
2018-07-16 13:47bugmasterStatusreviewed => tested
2018-07-16 13:51bugmasterTest case number => Not needed
2018-07-17 18:24bugmasterNote Added: 0077752
2018-07-17 18:24bugmasterAssigned Tobugmaster => abv
2018-07-17 18:24bugmasterStatustested => assigned
2018-07-17 22:04gitNote Added: 0077760
2018-07-18 15:14abvNote Added: 0077784
2018-07-18 15:14abvAssigned Toabv => bugmaster
2018-07-18 15:14abvStatusassigned => feedback
2018-07-18 19:14bugmasterFile Added: warnings.txt
2018-07-18 19:15bugmasterNote Edited: 0077752bug_revision_view_page.php?bugnote_id=77752#r19548
2018-07-21 18:39bugmasterChangeset attached => occt master 0a96e0bb
2018-07-21 18:39bugmasterStatusfeedback => verified
2018-07-21 18:39bugmasterResolutionopen => fixed
2018-07-21 20:02gitNote Added: 0077880
2018-07-21 20:02gitNote Added: 0077892
2018-11-14 14:40BenjaminBihlerNote Added: 0081087
2018-11-14 19:32BenjaminBihlerNote Added: 0081100
2018-11-14 19:33BenjaminBihlerAssigned Tobugmaster => abv
2018-11-14 19:33BenjaminBihlerStatusverified => feedback
2018-11-14 19:33BenjaminBihlerResolutionfixed => reopened
2019-08-27 07:08abvNote Added: 0086455
2019-08-27 07:09abvAssigned Toabv => BenjaminBihler
2019-08-27 16:20BenjaminBihlerFile Added: CR29769.zip
2019-08-27 16:20BenjaminBihlerNote Added: 0086467
2019-08-27 16:20BenjaminBihlerAssigned ToBenjaminBihler => abv
2019-08-27 16:23BenjaminBihlerNote Added: 0086468
2019-09-04 16:15kgvSummaryUninitialized data with BSplCLib_Cache, BSplSLib_Cache => Foundation Classes - Uninitialized data with BSplCLib_Cache, BSplSLib_Cache
2019-09-21 10:29abvRelationship addedparent of 0030990
2019-09-21 10:30abvStatusfeedback => verified
2019-09-21 10:30abvResolutionreopened => fixed
2019-09-21 10:32abvNote Added: 0087378
2019-09-23 08:30abvNote Added: 0087436

Notes
(0076725)
git   
2018-06-12 22:06   
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
(0076756)
abv   
2018-06-14 09:36   
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_*.
(0076758)
abv   
2018-06-14 09:47   
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.
(0076793)
abv   
2018-06-14 22:18   
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.
(0077585)
azv   
2018-07-13 18:10   
Reviewed.
(0077661)
bugmaster   
2018-07-16 13:47   
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
(0077752)
bugmaster   
2018-07-17 18:24   
(edited on: 2018-07-18 19:15)
See attached warnings.txt for new warnings

(0077760)
git   
2018-07-17 22:04   
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
(0077784)
abv   
2018-07-18 15:14   
Branch CR29769_1 eliminates compiler warnings and is on master; please consider
(0077880)
git   
2018-07-21 20:02   
Branch CR29769_1 has been deleted by inv.

SHA-1: 85180789252059b3b2dc8dede60a072d2cdf671e
(0077892)
git   
2018-07-21 20:02   
Branch CR29769 has been deleted by inv.

SHA-1: efbfdf32a21e07a8e5d1976b4645a423d4c75d42
(0081087)
BenjaminBihler   
2018-11-14 14:40   
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
(0081100)
BenjaminBihler   
2018-11-14 19:32   
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
(0086455)
abv   
2019-08-27 07:08   
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
(0086467)
BenjaminBihler   
2019-08-27 16:20   
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
(0086468)
BenjaminBihler   
2019-08-27 16:23   
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.
(0087378)
abv   
2019-09-21 10:32   
I have created a new issue 0030990 to treat the last reported problems.
(0087436)
abv   
2019-09-23 08:30   
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