View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0024533CommunityOCCT:Foundation Classespublic2014-01-15 21:352014-11-11 12:59
ReporterRoman Lygin Assigned Tobugmaster  
PrioritynormalSeverityintegration request 
Status closedResolutionfixed 
Product Version6.7.0 
Target Version6.8.0Fixed in Version6.8.0 
Summary0024533: Use 0 to check null handle instead of UndefinedHandleAccess
Descriptionsistent used specific non-null value
(e.g. 0xfefd0000 on 32 bit platforms) to check nullified handles.

This makes a lot of pain and risk for the following situations:

1. 'bool casting' -like constructs
Handle_MyType h = ReturnObject();
if (h) {
 h->DoSomething ();
}

2. comparing with explicit NULL
if (h != 0) {
  h->DoSomething()
}

0000001 and 0000002 are frequent newbies errors which are frequently reported on the forum, until someone advices to replace with !h.IsNull().

3. refactoring codes changing API from pointers to handles which can be human error-prone:
old:
MyType* p = ReturnObject();
... //many many lines below
if (p) {
  p->DoSomething();
}

updated to (e.g. due changed API of ReturnedObject()):
Handle_MyType p = ReturnObject();
... //many many lines below
if (p) { //remained unnoticed due to human error
}

#3 can be met in complex project during refactoring.


Note that boost::shared_ptr and std::shared_ptr do use 0 to identify uninitialized or nullified value.

The only related comment I remember is one of Andrey dated to November 2008,
http://opencascade.blogspot.ru/2008/11/open-cascade-handles-lets-handleem-part_21.html?showComment=1227264000000#c1579880060849701201:

1. There is some utility of having UndefinedHandleAddress not equal to zero. I have seen a few cases when one static variable is constructed by reference to another static variable which might be not-yet-initialized at the moment. Now in such case you get immediately Access Violation signal at null address; if UndefinedHandleAddress were null this problem might remain unnoticed.

One idea (thanks to AGV) is to set this value to 0x01 -- this address should be bad for all platforms. Though not completely sure...

Even if there is a risk for such case, this is still a corner case, much less probable than any of 0000001, 0000002, #3 above.

So use of UndefinedHandleAddress is discontinued and replaced with 0. Moreover, comparing to zero is likely a cheaper operation
performance-wise, with respect to comparing to a constant (the latter likely invokes extra subtraction).
Given that handle access are often on critical paths, this will also be valuable.


In addition, two other changes were added:
- some minor code clean up (removal of redundant Standard_EXPORT)
- replacing return type of comparison operator from int with bool. This is to avoid warnings in instantiations of std::unordered_set, unordered_map
using handles as keys where std::equal_to class would be instantiated with Handle_Standard_Transient.
Steps To ReproduceSee enclosed test case
TagsNo tags attached.
Test case numberbugs fclasses(002) bug24533

Attached Files

  • test.cxx (533 bytes)

Activities

Roman Lygin

2014-01-15 21:35

developer  

test.cxx (533 bytes)

Roman Lygin

2014-01-15 23:19

developer   ~0027507

Fix pushed to the git repository

Roman Lygin

2014-01-15 23:32

developer   ~0027508

Fix in WOK git repository has also been pushed (the package MS). The file CPPExt_Standard.edl remained unchanged as it seems to unused.

abv

2014-01-17 06:53

manager   ~0027526

Reviewed, please test. Note that I have pushed one more comment providing description of changes made and eliminating remaining definitions of _OCC64 macro.

Roman Lygin

2014-01-17 09:03

developer   ~0027527

Andrey, thanks for a further update.
I have intentionally left _OCC64 macro unchanged (outside of the modified files) to reserve it for possible OCC needs. If some 32/64-specific codes were required it would be possible to reuse it. Of course, not a big deal - one can always use OCC-invariant constructs (like those used in Standard_Macro.hxx to define _OCC64 itself).

P.S. Perhaps time to process 0022928 at last ;-) ?

Roman Lygin

2014-01-17 09:06

developer   ~0027528

By the way, you might want then to update WOK repository as well - MS Project generator seems to have _OCC64 hardcoded in WOKTclLib/templates.
Also please double-check that SALOME and other projects do not rely on it.

abv

2014-01-17 10:18

manager   ~0027529

Thank you Roman for the hint -- I did not remember that project templates have been moved to subfolder in WOK, and have not found them... I have pushed correction to WOK repo.

mkv

2014-01-23 12:47

tester   ~0027626

Dear BugMaster,

Branch CR24533 (and products from GIT master) was compiled on Linux and Windows platforms and tested.
SHA-1: 83ab8f2aa0de13d94c332635c3d710d2830dc15c

Number of compiler warnings:

occt component :
Linux: 48 (48 on master)
Windows: 1 (1 on master)

products component :
Linux: 12 (12 on master)
Windows: 2 (2 on master)

Regressions/Differences:
No regressions/differences

Testing cases:
bugs fclasses(002) bug24533 - OK.

Testing on Linux:
Total MEMORY difference: 370680548 / 366211348
Total CPU difference: 44012.140000000276 / 44012.79000000015

Testing on Windows:
Total MEMORY difference: 416934816 / 417396220
Total CPU difference: 30946.265625 / 32472.65625

There are not differences in images found by testdiff.

bugmaster

2014-03-12 14:41

administrator   ~0028225

Fix for WOK has been integrated into master of occt-wok repository

Roman Lygin

2014-10-15 15:08

developer   ~0033134

Confirmed as closed. Cannot close myself.

Related Changesets

occt: master 4d9421a9

2014-01-23 09:37:50

Roman Lygin


Committer: bugmaster Details Diff		 0024533: Use 0 to check null handle instead of UndefinedHandleAccess

Handle classes now use 0 as invalid value for pointer instead of custom (and platform-dependent) value like 0xfefd0000.
Compiler macros UndefinedHandleAddress and _OCC64 are eliminated.		 Affected Issues
0024533
mod - CMakeLists.txt Diff File
mod - src/LDOM/LDOM_XmlReader.cxx Diff File
mod - src/OSD/OSD_signal.cxx Diff File
mod - src/Standard/Handle_Standard_Persistent.hxx Diff File
mod - src/Standard/Handle_Standard_Transient.cxx Diff File
mod - src/Standard/Handle_Standard_Transient.hxx Diff File
mod - src/Standard/Standard_Macro.hxx Diff File
mod - src/Standard/Standard_MMgrOpt.cxx Diff File

Issue History

Date Modified Username Field Change
2014-01-15 21:35 Roman Lygin New Issue
2014-01-15 21:35 Roman Lygin Assigned To => abv
2014-01-15 21:35 Roman Lygin File Added: test.cxx
2014-01-15 23:19 Roman Lygin Note Added: 0027507
2014-01-15 23:19 Roman Lygin Status new => resolved
2014-01-15 23:32 Roman Lygin Note Added: 0027508
2014-01-17 06:53 abv Note Added: 0027526
2014-01-17 06:53 abv Assigned To abv => bugmaster
2014-01-17 06:53 abv Status resolved => reviewed
2014-01-17 07:21 mkv Assigned To bugmaster => mkv
2014-01-17 09:03 Roman Lygin Note Added: 0027527
2014-01-17 09:06 Roman Lygin Note Added: 0027528
2014-01-17 10:18 abv Note Added: 0027529
2014-01-23 12:47 mkv Note Added: 0027626
2014-01-23 12:48 mkv Test case number => bugs fclasses(002) bug24533
2014-01-23 12:48 mkv Assigned To mkv => bugmaster
2014-01-23 12:48 mkv Status reviewed => tested
2014-01-24 15:35 bugmaster Changeset attached => occt master 4d9421a9
2014-01-24 15:35 bugmaster Status tested => verified
2014-01-24 15:35 bugmaster Resolution open => fixed
2014-01-27 11:41 bugmaster Target Version => 6.7.1
2014-03-12 14:41 bugmaster Note Added: 0028225
2014-04-04 12:06 abv Target Version 6.7.1 => 6.8.0
2014-10-15 15:08 Roman Lygin Assigned To bugmaster => Roman Lygin
2014-10-15 15:08 Roman Lygin Assigned To Roman Lygin => bugmaster
2014-10-15 15:08 Roman Lygin Note Added: 0033134
2014-11-11 12:44 aiv Fixed in Version => 6.8.0
2014-11-11 12:59 aiv Status verified => closed