MantisBT - Community
View Issue Details
0030937Community[OCCT] OCCT:Codingpublic2019-09-03 18:252019-10-10 19:24
Vico Liang 
Vico Liang 
normalblock 
feedbackopen 
 
[OCCT] 7.5.0* 
0030937: Coding - TNaming_NamedShape destructor should not be inlined
The destructor of TNaming_NamedShape as below:
~TNaming_NamedShape()
{
  Clear();
}

It will cause serious problem when building with android-clang compiler for arm64-v8. The build environment is android SDK version: 26.1.1; NDK version: 20.0.5594570.

below code snippet will reproduce the issue:
TDF_Attribute* aNSA = new TNaming_NamedShape();
TNaming_NamedShape* aNS = dynamic_cast<TNaming_NamedShape*>(aNSA);
if(!aNS )
{
 std::cout << "Serious bug occured"; // bug here
}

To fix the issue just move the destructor to cpp file. it's better to declare as virtual export function :
Standard_EXPORT virtual ~TNaming_NamedShape();
No tags attached.
related to 0030238new kgv Open CASCADE Configuration - NCollection_Array1<OSD_Timer> crashes on destruction when using msvc 
Issue History
2019-09-03 18:25Vico LiangNew Issue
2019-09-03 18:25Vico LiangAssigned To => mpv
2019-09-04 11:13kgvRelationship addedrelated to 0030238
2019-09-04 11:13kgvNote Added: 0086720
2019-09-04 11:14kgvNote Edited: 0086720bug_revision_view_page.php?bugnote_id=86720#r21720
2019-09-04 11:43Vico LiangNote Added: 0086721
2019-09-04 11:50Vico LiangNote Edited: 0086721bug_revision_view_page.php?bugnote_id=86721#r21722
2019-09-04 11:50kgvNote Added: 0086722
2019-09-04 11:50kgvNote Edited: 0086722bug_revision_view_page.php?bugnote_id=86722#r21724
2019-09-04 11:51kgvNote Edited: 0086722bug_revision_view_page.php?bugnote_id=86722#r21725
2019-09-04 11:52kgvNote Edited: 0086722bug_revision_view_page.php?bugnote_id=86722#r21726
2019-09-04 11:52kgvCategoryOCCT:Application Framework => OCCT:Coding
2019-09-04 11:52kgvSummaryTNaming_NamedShape destructor should not be inlined => Coding - TNaming_NamedShape destructor should not be inlined
2019-09-04 11:58Vico LiangNote Edited: 0086721bug_revision_view_page.php?bugnote_id=86721#r21729
2019-09-04 12:07Vico LiangNote Added: 0086725
2019-09-04 12:20abvNote Added: 0086728
2019-09-04 12:21abvAssigned Tompv => Vico Liang
2019-09-04 12:21abvStatusnew => feedback
2019-09-04 12:21abvTarget Version7.4.0 => 7.5.0*
2019-09-04 13:30Vico LiangNote Added: 0086732
2019-09-04 13:31Vico LiangAssigned ToVico Liang => abv
2019-09-04 13:37Vico LiangNote Added: 0086733
2019-09-04 15:17Vico LiangNote Deleted: 0086733
2019-09-04 19:18Vico LiangNote Added: 0086754
2019-09-05 06:46Vico LiangNote Added: 0086767
2019-09-05 06:46Vico LiangNote Edited: 0086767bug_revision_view_page.php?bugnote_id=86767#r21743
2019-09-05 10:44Vico LiangNote Added: 0086784
2019-09-05 15:03Vico LiangNote Edited: 0086754bug_revision_view_page.php?bugnote_id=86754#r21757
2019-09-09 15:39Vico LiangNote Added: 0086986
2019-10-02 22:02abvNote Added: 0087725
2019-10-02 22:02abvAssigned Toabv => Vico Liang
2019-10-03 10:13abvNote Added: 0087733
2019-10-10 19:24Vico LiangNote Added: 0088025

Notes
(0086720)
kgv   
2019-09-04 11:13   
(edited on: 2019-09-04 11:14)
Isn't it a compiler/linker bug?
Why it should be changed in OCCT instead of reporting to CLang?

(0086721)
Vico Liang   
2019-09-04 11:43   
(edited on: 2019-09-04 11:58)
Yes, I agree with you. It's a bug of compiler or linker.
Form coding best practice, it makes no sense to inline a virtual function which is accessed by pointer, but it makes sense to make better coding on OCCT side.

(0086722)
kgv   
2019-09-04 11:50   
(edited on: 2019-09-04 11:52)
> it's make no sense to inline a virtual function
It is not true - compiler is able deducing from context if object is statically typed and actually inline even virtual method call.
Not sure that this is applicable to destructors, but maybe - haven't analyzed this subject in deep.

Concerning coding practices, the inlining point here is code base compactness - always moving trivial virtual methods into .cpp would cause extra code.
OCCT code base has enormous amount of inlined virtual methods, including implicit destructors as mentioned in related bug 0030238 in MSVC.
So that if such patch will be considered necessary, it should cover more classes, not only mentioned in the bug.

(0086725)
Vico Liang   
2019-09-04 12:07   
>It is not true - compiler is able deducing from context if object is statically typed and actually inline even virtual method call.
OCCT most objects are created by handle(pointer), the inline virtual function will never expanded. It will also confuse developer.
(0086728)
abv   
2019-09-04 12:20   
Vico, the problem you have reported is quite strange, can you check that you are using the same compiler and consistent compiler flags for building OCCT and your application? Meanwhile, CLang provides some tools for checking consistency of the code at run-time, perhaps this can be used to debug the issue: see https://clang.llvm.org/docs/ControlFlowIntegrity.html [^]
(0086732)
Vico Liang   
2019-09-04 13:30   
I don't have chance to change the behavior of the compiler. I'm using the default option in android build: android-clang compiler for arm64-v8. The build environment is android SDK version: 26.1.1; NDK version: 20.0.5594570. What feedback do you want from me? I already told you the reason of the issue and the fix of the solution.
(0086754)
Vico Liang   
2019-09-04 19:18   
(edited on: 2019-09-05 15:03)
the issue persists on SDK Version: 26.1.1, NDK Version: 19.2.5345600 and 18

(0086767)
Vico Liang   
2019-09-05 06:46   
These are two issues about android ndk related to this issue:

1. std::dynamic_pointer_cast always returns a nullptr
https://github.com/android-ndk/ndk/issues/533 [^]

2. dynamic_cast form pointers is not working when linked with libc++_shared (ndk r15, r16b1)
https://github.com/android-ndk/ndk/issues/519 [^]

(0086784)
Vico Liang   
2019-09-05 10:44   
It's interesting that this issue doesn't occur on android armv7 build. so it's a bug of NDK arm64-v8.
(0086986)
Vico Liang   
2019-09-09 15:39   
I have filed a bug to ndk:
https://github.com/android/ndk/issues/1075 [^]
(0087725)
abv   
2019-10-02 22:02   
Hello Vico,

Thank you for taking effort of diagnosing and reporting the issue.

As the problem is of general nature, apparently it should not be limited to particular class (TNaming_NamedShape). Thus to fix the problem consistently on OCCT side we need to understand clearly what is wrong, and fix that in all affected places.

From your discussion with DanAlbert in NDK issue tracker (1075) I conclude that his explanations regarding missing "key function" are not relevant in your situation, since class TNaming_NamedShape definitely does have key functions. Note that most of OCCT classes inheriting Standard_Transient are defined with macro DEFINE_STANDARD_RTTIEXT that defines out-of-line virtual function DynamicType(), this should guarantee that typeinfo is exported correctly for these classes.

Thus I share opinion of Kirill that according to current symptoms it is a compiler / linker bug that cannot be fixed at OCCT side.

Can you confirm my understanding that your custom code and / or OCCT libraries are loaded dynamically into the application, i.e. by explicit call to dlopen?
If so, and if you can control parameters of dlopen() call, the problem can probably be resolved by passing RTLD_GLOBAL as the last argument to dlopen().
(I have experienced similar problem long time ago with Python modules, and such solution helped that time).

Another possibility to avoid the issue could be to link OCCT statically (provided that that does not violate LGPL terms).

Andrey
(0087733)
abv   
2019-10-03 10:13   
One more point: according to CLang documentation, it should give a diagnostic warning when a class with virtual functions has no key method: https://releases.llvm.org/8.0.0/tools/clang/docs/DiagnosticsReference.html#wweak-vtables [^]

We have never seen this warning in OCCT code, thus I suppose it should be clean (or warning is disabled for some reason)
(0088025)
Vico Liang   
2019-10-10 19:24   
Dear ABV,

>>Can you confirm my understanding that your custom code and / or OCCT libraries are loaded dynamically into the application.

Yes, it should be loaded dynamically, but it's not calling method dlopen() directly, it's calling in java System.load() method.

>>Another possibility to avoid the issue could be to link OCCT statically (provided that that does not violate LGPL terms).

Yes, this solution works. It's the best option for me. save me much of time.

Thanks for assisting.
Vico