View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0030937CommunityOCCT:Codingpublic2019-09-03 18:252024-04-11 19:51
ReporterVico Liang Assigned Todpasukhi  
PrioritynormalSeverityblock 
Status assignedResolutionopen 
Target VersionUnscheduled 
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

Relationships

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

Activities

kgv

2019-09-04 11:13

developer   ~0086720

Last edited: 2019-09-04 11:14

 Isn't it a compiler/linker bug?
Why it should be changed in OCCT instead of reporting to CLang?

Vico Liang

2019-09-04 11:43

developer   ~0086721

Last edited: 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.

kgv

2019-09-04 11:50

developer   ~0086722

Last edited: 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.

Vico Liang

2019-09-04 12:07

developer   ~0086725

>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.

abv

2019-09-04 12:20

manager   ~0086728

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

Vico Liang

2019-09-04 13:30

developer   ~0086732

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.

Vico Liang

2019-09-04 19:18

developer   ~0086754

Last edited: 2019-09-05 15:03

 the issue persists on SDK Version: 26.1.1, NDK Version: 19.2.5345600 and 18

Vico Liang

2019-09-05 06:46

developer   ~0086767

Last edited: 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

Vico Liang

2019-09-05 10:44

developer   ~0086784

It's interesting that this issue doesn't occur on android armv7 build. so it's a bug of NDK arm64-v8.

Vico Liang

2019-09-09 15:39

developer   ~0086986

I have filed a bug to ndk:
https://github.com/android/ndk/issues/1075

abv

2019-10-02 22:02

manager   ~0087725

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

abv

2019-10-03 10:13

manager   ~0087733

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)

Vico Liang

2019-10-10 19:24

developer   ~0088025

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
2019-09-04 11:43 Vico Liang Note Added: 0086721
2019-09-04 11:50 Vico Liang Note Edited: 0086721
2019-09-04 11:50 kgv Note Added: 0086722
2019-09-04 11:50 kgv Note Edited: 0086722
2019-09-04 11:51 kgv Note Edited: 0086722
2019-09-04 11:52 kgv Note Edited: 0086722
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
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 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
2019-09-05 10:44 Vico Liang Note Added: 0086784
2019-09-05 15:03 Vico Liang Note Edited: 0086754
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
2020-09-11 15:34 utverdov Target Version 7.5.0 => 7.6.0
2021-11-01 18:14 szy Target Version 7.6.0 => 7.7.0
2022-10-24 10:39 szy Target Version 7.7.0 => 7.8.0
2023-08-02 02:00 dpasukhi Assigned To Vico Liang => dpasukhi
2023-08-02 02:00 dpasukhi Status feedback => assigned
2023-08-02 02:01 dpasukhi Target Version 7.8.0 => Unscheduled