MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0030937Community[OCCT] OCCT:Codingpublic2019-09-03 18:252019-10-10 19:24
ReporterVico Liang 
Assigned ToVico Liang 
PrioritynormalSeverityblock 
StatusfeedbackResolutionopen 
PlatformOSOS Version
Product Version 
Target Version[OCCT] 7.5.0*Fixed in Version 
Summary0030937: Coding - TNaming_NamedShape destructor should not be inlined
DescriptionThe 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();
TagsNo tags attached.
Test case number
Attached Files

- Relationships
related to 0030238newkgv Open CASCADE Configuration - NCollection_Array1<OSD_Timer> crashes on destruction when using msvc 

-  Notes
(0086720)
kgv (developer)
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 (developer)
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 (developer)
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 (developer)
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 (manager)
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 (developer)
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 (developer)
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 (developer)
2019-09-05 06:46
edited on: 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 (developer)
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 (developer)
2019-09-09 15:39

I have filed a bug to ndk:
https://github.com/android/ndk/issues/1075 [^]
(0087725)
abv (manager)
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 (manager)
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 (developer)
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

- Issue History
Date Modified Username Field Change
2019-09-03 18:25 Vico Liang New Issue
2019-09-03 18:25 Vico Liang Assigned To => mpv
2019-09-04 11:13 kgv Relationship added related to 0030238
2019-09-04 11:13 kgv Note Added: 0086720
2019-09-04 11:14 kgv Note Edited: 0086720 View Revisions
2019-09-04 11:43 Vico Liang Note Added: 0086721
2019-09-04 11:50 Vico Liang Note Edited: 0086721 View Revisions
2019-09-04 11:50 kgv Note Added: 0086722
2019-09-04 11:50 kgv Note Edited: 0086722 View Revisions
2019-09-04 11:51 kgv Note Edited: 0086722 View Revisions
2019-09-04 11:52 kgv Note Edited: 0086722 View Revisions
2019-09-04 11:52 kgv Category OCCT:Application Framework => OCCT:Coding
2019-09-04 11:52 kgv Summary TNaming_NamedShape destructor should not be inlined => Coding - TNaming_NamedShape destructor should not be inlined
2019-09-04 11:58 Vico Liang Note Edited: 0086721 View Revisions
2019-09-04 12:07 Vico Liang Note Added: 0086725
2019-09-04 12:20 abv Note Added: 0086728
2019-09-04 12:21 abv Assigned To mpv => Vico Liang
2019-09-04 12:21 abv Status new => feedback
2019-09-04 12:21 abv Target Version 7.4.0 => 7.5.0*
2019-09-04 13:30 Vico Liang Note Added: 0086732
2019-09-04 13:31 Vico Liang Assigned To Vico Liang => abv
2019-09-04 13:37 Vico Liang Note Added: 0086733
2019-09-04 15:17 Vico Liang Note Deleted: 0086733
2019-09-04 19:18 Vico Liang Note Added: 0086754
2019-09-05 06:46 Vico Liang Note Added: 0086767
2019-09-05 06:46 Vico Liang Note Edited: 0086767 View Revisions
2019-09-05 10:44 Vico Liang Note Added: 0086784
2019-09-05 15:03 Vico Liang Note Edited: 0086754 View Revisions
2019-09-09 15:39 Vico Liang Note Added: 0086986
2019-10-02 22:02 abv Note Added: 0087725
2019-10-02 22:02 abv Assigned To abv => Vico Liang
2019-10-03 10:13 abv Note Added: 0087733
2019-10-10 19:24 Vico Liang Note Added: 0088025


Copyright © 2000 - 2019 MantisBT Team
Powered by Mantis Bugtracker