iMesh and iBase handle/instance types do not correspond to specification

[Johannes Probst]

This issue concerns the typedefs in iMesh.h and iBase.h:

typedef struct iMesh_Instance_Private* iMesh_Instance;

and

typedef void* iBase_Instance;
typedef struct iBase_EntityHandle_Private* iBase_EntityHandle;
typedef struct iBase_EntitySetHandle_Private* iBase_EntitySetHandle;
typedef struct iBase_TagHandle_Private* iBase_TagHandle;
typedef struct iBase_EntityIterator_Private* iBase_EntityIterator;
typedef struct iBase_EntityArrIterator_Private* iBase_EntityArrIterator;

The only piece of official(-ish) documentation I was able to get a hold of states the following: "Handle types are typedef’d to size_t (iBase_EntityHandle, iBase_EntitySetHandle, iBase_TagHandle, iMesh_Instance)". MOAB is not following this specification.

The reason why this matters to us is that we are wrapping the C API with cgo which has very strict pointer passing rules and breaks when integers are stored in pointer-typed variables. In our case, we keep seeing irregular crashes which occur during garbage collection, when the runtime scans the heap and comes across invalid pointers.

I believe that a correct solution would be to use size_t (as per spec), or uintptr_t (which is defined as an integer large enough to hold any pointer on the target architecture) instead.

I'm filing this as "critical enhancement" because it is not a "bug" in the classic sense but still very important for us, as it causes some headache how to wire MOAB into our application. We have considered re-wrapping iMesh and iBase as a last resort but at the moment we still hope that there's an easier solution.

[Vijay M]

@jprobst_simscale I apologize about the delay in getting back to you.

We are in the process of deprecating the use of iMesh in most of our codes since the standard has been hard to maintain and there are very few codes that rely on this interface anymore. We are developing and enhancing the new iMOAB interface that is intended to be language-agnostic and should provide more flexibility when passing state between codes.

I believe that a correct solution would be to use size_t (as per spec), or uintptr_t (which is defined as an integer large enough to hold any pointer on the target architecture) instead.

Isn't this architecture specific as well ? Take a look at iBase_f.h that is written out during build time. There are several preprocessing blocks to choose the appropriate definition for the IBASE_HANDLE_T base type depending on compiler/architecture. We may need to revisit this if the implementation is flawed for certain use cases.

I am unsure about the exact reasons why MOAB typedef's the handles to void* instead of size_t as provided in the specification. @tautges thoughts ?

I understand the motivations for the proposal here, and we would be glad to take in a patch to test. Please feel free to submit a PR of your changes and we can discuss in more detail whether the change would be appropriate to go into the next release.

[Johannes Probst]

@vijaysm Thanks for your reply!

We are in the process of deprecating the use of iMesh in most of our codes since the standard has been hard to maintain and there are very few codes that rely on this interface anymore.

Does this mean that the iMesh API will be deprecated / removed from MOAB? Will the transition to iMOAB be straightforward?

Isn't this architecture specific as well ?

Yes and no. Yes because the underlying memory layout (number of bytes) of the type can vary across different architectures. However this does not force a developer to make the decision which type to choose if he wants to exploit the maximum size of an integer type on a given hardware. This task can better be done by the compiler, for example see the gcc manual. These types allow to exploit the maximum range of the target architecture whilst keeping the source code portable.

Code like this from EntityHandle.hpp

#ifdef MOAB_FORCE_64_BIT_HANDLES
  typedef uint64_t EntityHandle;
  typedef  int64_t EntityID;
#elif defined (MOAB_FORCE_32_BIT_HANDLES)
  typedef uint32_t EntityHandle;
  typedef  int32_t EntityID;
#else
# ifdef MOAB_HAVE_SIZE_T
    typedef size_t EntityHandle;
# else
    typedef unsigned long EntityHandle;
# endif
# ifdef MOAB_HAVE_PTRDIFF_T
    typedef ptrdiff_t EntityID;
# else
    typedef long EntityID;
# endif
#endif 

can be simplified to

typedef uintmax_t EntityHandle;
typedef intmax_t EntityID;

This ensures that the handle size automatically switches from 32 to 64 bits based on hardware support without making the source code aware of it.

If the ability to override the size of the handles is supposed to be kept, the definitions can be factored out into a *.h file and included from iBase.h and iMesh.h to ensure the definitions are shared and the memory layout on the stack is compatible. I'll see if I can create a PR based on this.

[Vijay M]

Does this mean that the iMesh API will be deprecated / removed from MOAB? Will the transition to iMOAB be straightforward?

Yes eventually and that's the plan since we've been transitioning some of our applications away from this interface. It has proven to be a little inflexible and verbose/clunky depending on the needs. iMOAB may not be a 100% replacement out of the box but most critical operations should have a 1-1 equivalency. We certainly can add new routines if needed as well since this is evolving with the applications.

can be simplified to

typedef uintmax_t EntityHandle; typedef intmax_t EntityID;

Agreed. The design and implementation is about 8-9 years old and the core developers who wrote the bulk of iMesh are no longer with the project now. While there is certainly a lot of refactoring and simplification we could do here, at the moment, we weren't motivated to maintain it and keep it backward compatible with major design changes.

And updates in iBase/iMesh also need to propagate across the stack (CGM/MOAB/MeshKit) which is another reason why we have abstained from some of these changes. But a PR with suitable modifications is most welcome and perhaps we could discuss further about simplifying the ITAPS implementations in all the components as well.

[Johannes Probst]

We have meanwhile resorted to porting our entire stack to the native C++ interface. This bug is therefore not relevant for us anymore. I am closing this issue.

[Johannes Probst]

Closing as won't fix because there are no plans to maintain iMesh.