View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0029769 | Community | OCCT:Foundation Classes | public | 2018-05-14 23:01 | 2019-09-23 08:30 |
| Reporter | Aaron Michalk | Assigned To | |||
| Priority | normal | Severity | crash | ||
| Status | closed | Resolution | fixed | ||
| Platform | VC++ 2017 64 bit | OS | Windows | ||
| Product Version | 7.2.0 | ||||
| Target Version | 7.4.0 | Fixed in Version | 7.4.0 | ||
| Summary | 0029769: Foundation Classes - Uninitialized data with BSplCLib_Cache, BSplSLib_Cache | ||||
| Description | Using malloc instead of calloc results in accessing invalid memory when evaluating spline cache. This is due to uninitialized data. | ||||
| Steps To Reproduce | 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 | ||||
| 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. | ||||
| Tags | No tags attached. | ||||
| Test case number | Not needed | ||||
| related to | 0024208 | closed | apn | Open CASCADE | Optimization of the edge-edge and edge-face intersection algorithms |
| parent of | 0030990 | closed | bugmaster | Community | Foundation Classes - unexpected change in numerical results on bsplines after 0029769 |
| child of | 0028203 | new | Open CASCADE | Coding rules - check testing with MMGT_CLEAR turned OFF | |
| child of | 0024682 | closed | bugmaster | Open CASCADE | Move out B-spline cache from curves and surfaces to dedicated classes BSplCLib_Cache and BSplSLib_Cache |
|
|
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 |
|
|
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_*. |
|
|
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. |
|
|
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. |
|
|
Reviewed. |
|
|
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 |
|
|
See attached warnings.txt for new warnings |
|
|
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 |
|
|
Branch CR29769_1 eliminates compiler warnings and is on master; please consider |
|
|
warnings.txt (31,909 bytes) |
|
|
Branch CR29769_1 has been deleted by inv. SHA-1: 85180789252059b3b2dc8dede60a072d2cdf671e |
|
|
Branch CR29769 has been deleted by inv. SHA-1: efbfdf32a21e07a8e5d1976b4645a423d4c75d42 |
|
|
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 |
|
|
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 |
|
|
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 |
|
|
CR29769.zip (49,050 bytes) |
|
|
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 |
|
|
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. |
|
|
I have created a new issue 0030990 to treat the last reported problems. |
|
|
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 |
|
occt: master 0a96e0bb 2018-06-10 19:40:12
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 | ||
| 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 |
|
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 |
|
Note Added: 0076756 | |
| 2018-06-14 09:36 |
|
Status | new => resolved |
| 2018-06-14 09:36 |
|
Assigned To | abv => azv |
| 2018-06-14 09:36 |
|
Relationship added | related to 0024208 |
| 2018-06-14 09:37 |
|
Relationship added | child of 0024682 |
| 2018-06-14 09:47 |
|
Note Added: 0076758 | |
| 2018-06-14 22:18 |
|
Note Added: 0076793 | |
| 2018-06-14 22:18 |
|
Steps to Reproduce Updated | |
| 2018-07-13 18:10 |
|
Note Added: 0077585 | |
| 2018-07-13 18:10 |
|
Assigned To | azv => bugmaster |
| 2018-07-13 18:10 |
|
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 |
|
Note Added: 0077784 | |
| 2018-07-18 15:14 |
|
Assigned To | abv => bugmaster |
| 2018-07-18 15:14 |
|
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 |
|
Note Added: 0086455 | |
| 2019-08-27 07:09 |
|
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 |
|
Relationship added | parent of 0030990 |
| 2019-09-21 10:30 |
|
Status | feedback => verified |
| 2019-09-21 10:30 |
|
Resolution | reopened => fixed |
| 2019-09-21 10:32 |
|
Note Added: 0087378 | |
| 2019-09-23 08:30 |
|
Note Added: 0087436 |
