MantisBT - VTK
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0003059VTK(No Category)public2006-03-29 16:472016-08-12 09:54
ReporterSean McBride 
Assigned ToSebastien Barre 
PrioritynormalSeverityminorReproducibilityalways
StatusclosedResolutionmoved 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version 
Project
Type
Summary0003059: Remove assumption that build and target machine are the same (ie support Mac OS Universal Binaries)
DescriptionApple is now selling Macs that use 2 kinds of CPUs: PPC and Intel. Both kinds will be out there for years to come. They have created something called a 'universal binary' which is basically a single executable file that has both PowerPC and Intel object code, and so runs natively on both types of Mac. Both PPC and Intel Macs can compile both PPC and Intel code.

Building something as universal is pretty easy, at least in the sense that it only takes a few parameters to gcc. Recent versions of cmake now also include functionality to build universal binaries.

On the other hand, building something as universal is pretty hard in the sense that you must be sure to write your code in a portable way. Specifically, one must not assume that the target platform is the same as the build platform.

vtk makes this assumption in many places. This bug is requesting that it stop. :) I realise that this is no small task. It will take time, but it is something that can be done bit by bit.

The first step, I believe, is to stop making this assumption in all new vtk code. Perhaps:
 a) adding it to the coding standards here <http://www.vtk.org/Wiki/VTK_Coding_Standards>? [^]
 b) informing all vtk developers (at kitware at least)

The next step is to find where the assumption is already made and get rid of it.

One problem is CMake's TRY_RUN functionality, which vtk uses for at least two things. Ideally, all TRY_RUN steps should be avoided, because by their nature they assume the build and target machines are of the same type.

vtk uses TRY_RUN for:

1) endianess
The PPC is big endian, the Intel is little. Building vtk tests the build machine's endianess and potentially defines VTK_WORDS_BIGENDIAN. This is no good for building universal binaries, as it will always be wrong half the time. I'm not sure what the solution is here. Some ideas:
a) there are POSIX APIs to swap bytes: ntohl, htonl, ntohs, and htons. But do they exist on windows? I guess not.
b) Apple's gcc and Intel's mac compiler automatically #define __BIG_ENDIAN__ or __LITTLE_ENDIAN__. I've read that 'most modern Unix systems define the byte order in the sys/param.h include file'. Does Windows have a similar header? If so, VTK_WORDS_BIGENDIAN could be defined by comparing to existing defines in system headers.
c) endianess could be determined at runtime, but this may not be acceptable for performance-critical code.

2) Type sizes
building vtk checks the size of various types such as char, short, int, long, float, and double on the build machine. For the particular case of Mac Universal Binaries, the sizes of these types do not change between PPC and Intel. But one day they might, as 64 bit CPUs become more popular. One type that does change size between ppc and intel is 'bool', it is 8 bit on one and 32 bit on the other, but I do not see any VTK_SIZEOF_BOOL or similar.

Lots of what vtk does with this info looks like this:

// Template ID's must be 32-bits. See .cxx file for more information.
#if VTK_SIZEOF_SHORT == 4
typedef unsigned short TemplateIDType;
#elif VTK_SIZEOF_INT == 4
typedef unsigned int TemplateIDType;
#elif VTK_SIZEOF_LONG == 4
typedef unsigned long TemplateIDType;
#endif

This kind of thing could be easily replaced using the standard integer types in stdint.h, These types are part of the C99 standard, 7 years old now. Like so:

typedef uint32_t TemplateIDType;

Then TemplateIDType will be 32bits no matter which platform it's built on. For info on these types, see:
<http://www.opengroup.org/onlinepubs/000095399/basedefs/stdint.h.html> [^]

Or are there compilers that vtk must support that still do not support stdint.h? If so, they could be special-cased.

3) Other. :)
I'm sure there are other issues as well, but I think the above is a starting point. And I want to file this bug sooner than later.
Steps To Reproduce
Additional Information
TagsNo tags attached.
Relationships
Attached Filestxt vtk_patch.txt (1,171) 1969-12-31 19:00
https://www.vtk.org/Bug/file/5731/vtk_patch.txt
Issue History
Date ModifiedUsernameFieldChange
2008-02-06 09:58Jeff BaumesAssigned ToWill Schroeder => Sebastien Barre
2008-02-06 09:58Jeff BaumesSeveritymajor => minor
2008-02-07 10:05Sean McBrideDescription Updated
2008-02-07 10:12Sebastien BarreStatustabled => closed
2008-02-07 10:12Sebastien BarreNote Added: 0010412
2008-02-07 10:12Sebastien BarreResolutionopen => fixed
2008-02-08 11:05Sean McBrideStatusclosed => @20@
2008-02-08 11:05Sean McBrideResolutionfixed => reopened
2008-02-08 11:05Sean McBrideNote Added: 0010430
2008-02-11 13:40Sebastien BarreNote Added: 0010453
2008-02-11 14:18Sean McBrideNote Added: 0010455
2008-02-11 14:54Alex NeundorfNote Added: 0010456
2008-02-11 16:27Sean McBrideNote Added: 0010457
2011-06-16 13:11Zack GalbreathCategory => (No Category)
2016-08-12 09:54Kitware RobotNote Added: 0036848
2016-08-12 09:54Kitware RobotStatusexpired => closed
2016-08-12 09:54Kitware RobotResolutionreopened => moved

Notes
(0004176)
Sean McBride   
2006-06-08 13:37   
I am willing to spend some time and effort writing and testing patches to vtk, but we need to get a discussion going in this bug on what direction kitware wants to take, if any.

I believe we should start with the endianess issue. My preference is for option 'b'. Thoughts?
(0004642)
Sean McBride   
2006-08-14 14:08   
Regarding #1, Bill Hoffman suggested to me in an email: "in the configured header file it can check for __BIG_ENDIAN__ or __LITTLE_ENDIAN__ and change the value found by cmake to the correct one".

This sounds like a great starting point to me! But I'm not sure how to proceed. How/where can I change vtkConfigure.h after cmake creates it? On my PPC Mac, the file starts like so:

/* Byte order. */
#define VTK_WORDS_BIGENDIAN

I'd like to change it to:

/* Byte order. */
#if defined(__LITTLE_ENDIAN__)
    #undef VTK_WORDS_BIGENDIAN
#elif defined(__BIG_ENDIAN__)
    #define VTK_WORDS_BIGENDIAN
#endif
(0005651)
Sean McBride   
2006-11-02 16:36   
I've attached a patch to vtkConfigure.h.in that uses __BIG_ENDIAN__ and __LITTLE_ENDIAN__, if defined, to define VTK_WORDS_BIGENDIAN.

Specifically, I changed:

#cmakedefine VTK_WORDS_BIGENDIAN

to

#if defined(__BIG_ENDIAN__)
# define VTK_WORDS_BIGENDIAN
#elif defined(__LITTLE_ENDIAN__)
# undef VTK_WORDS_BIGENDIAN
#else
#cmakedefine VTK_WORDS_BIGENDIAN
#endif

This should be safe for all OSes and compilers.

Would someone care to commit it? Thanks.
(0006297)
   
2007-01-31 10:01   
I just now checked in a partial fix. It does as Bill suggested wrt to endianess. Also does similar type sizes.
(0006298)
Sean McBride   
2007-01-31 10:02   
I just now checked in a partial fix. It does as Bill suggested wrt to endianess. Also does similar for type sizes.
(0006512)
Sean McBride   
2007-02-21 11:58   
I'm happy to say that as of the current code in cvs, things seems to be working fairly well! I am able to build a Universal VTK app on i386, and it runs properly on both i386 and ppc.

It think it would be premature to close this bug though...
(0010412)
Sebastien Barre   
2008-02-07 10:12   
Fixed in the CVS then.
(0010430)
Sean McBride   
2008-02-08 11:05   
After rereading my most recent comment (before Sebastien closed this bug as fixed), I realise I was not sufficiently clear.

I do not consider this bug fixed. OTOH, I also don't think the remaining work is vital for 5.2.

VTK currently works OK when built Universal, but only because of several hack jobs.

What I hope to see changed:
 1) stop using TRY_RUN for endianness detection.
 2) stop using TRY_RUN for checking type sizes.
 3) stop using TRY_RUN everywhere.

This would allow VTK to be cross-compiled. I realise this is a lot of work, but it can be done bit by bit.
(0010453)
Sebastien Barre   
2008-02-11 13:40   
Sean,

We are trying to hunt VTK bugs lately, so it would help if we can resolve/close this one. Here is some feedback from Alex Neundorf, I quote:

> Ideally, all TRY_RUN steps should be avoided,

That's right.

> vtk uses TRY_RUN for:
> 1) endianess

I think that's not right with cmake cvs. At least the test for endianess
coming with cmake cvs uses only a try_compile:

# on mac, if there are universal binaries built both will be true
# return the result depending on the machine on which cmake runs
IF(CMAKE_TEST_ENDIANESS_STRINGS_BE AND CMAKE_TEST_ENDIANESS_STRINGS_LE)
   IF(CMAKE_SYSTEM_PROCESSOR MATCHES powerpc)
     SET(CMAKE_TEST_ENDIANESS_STRINGS_BE TRUE)
     SET(CMAKE_TEST_ENDIANESS_STRINGS_LE FALSE)
   ELSE(CMAKE_SYSTEM_PROCESSOR MATCHES powerpc)
     SET(CMAKE_TEST_ENDIANESS_STRINGS_BE FALSE)
     SET(CMAKE_TEST_ENDIANESS_STRINGS_LE TRUE)
   ENDIF(CMAKE_SYSTEM_PROCESSOR MATCHES powerpc)
MESSAGE(STATUS "TEST_BIG_ENDIAN found different results, consider setting
                CMAKE_OSX_ARCHITECTURES or CMAKE_TRY_COMPILE_OSX_ARCHITECTURES
                to one or no architecture !")
ENDIF(CMAKE_TEST_ENDIANESS_STRINGS_BE AND CMAKE_TEST_ENDIANESS_STRINGS_LE)

The same is in CheckTypeSize.cmake:

IF(NOT "${CMAKE_CHECKTYPESIZE_FIRST_RESULT}" STREQUAL "${${VARIABLE}}")
  MESSAGE(SEND_ERROR "CHECK_TYPE_SIZE found different results, consider
                      setting CMAKE_OSX_ARCHITECTURES or
                      CMAKE_TRY_COMPILE_OSX_ARCHITECTURES to one or no
                      architecture !")
ENDIF(NOT "${CMAKE_CHECKTYPESIZE_FIRST_RESULT}" STREQUAL "${${VARIABLE}}")

So developers get a message if two different results are found.

Does this help a bit ?
(0010455)
Sean McBride   
2008-02-11 14:18   
Sebastien,

I'm afraid I do not follow... I'm actually not very familiar with CMake's syntax. This is how understand your snippit: if I set CMAKE_OSX_ARCHITECTURES to have 2 or more values, say ppc and i386, and I then use CMake's built-in endianness detection, I get an error message. Is that correct?
(0010456)
Alex Neundorf   
2008-02-11 14:54   
Yes.
If you are using cmake cvs, the test for the endianess is done using a try_compile().
If cmake detects that the created file is both big and little endian (i.e. a universal binary), it will set the result depending on what the current platform is and print an information message. This means it gives the same result as with cmake 2.4.x but informs the user that he should do something.

And I think there was somewhere an example in a config-header how to check for endianess at compile time (i.e. not at cmake time). Was it in vtk or paraview ? Sebastian, can you check ? There were some nested ifdefs I think.

For the type size, cmake also detects if there are multiple results (i.e. a universal binary), but only prints the message if the results actually differ.

About "don't use try_run() at all": yes, this would be the goal, but hasn't been reached yet.

Alex
(0010457)
Sean McBride   
2008-02-11 16:27   
I see. So, I agree that CMake 2.6 behaves better than 2.4. 2.4 builds half your executable wrong (it picks one endianness and uses it for both), 2.6 still does that but at least tells you. :) Maybe this is the best CMake can do? I guess it is the responsibility of a project that uses CMake, like VTK, to _not use_ CMake's built-in endianness check.

If you look at vtkConfigure.h.in you'll see at the top:

#if !defined(__APPLE__)
 #cmakedefine VTK_WORDS_BIGENDIAN
#elif defined(__BIG_ENDIAN__)
# define VTK_WORDS_BIGENDIAN
#endif

The check for __APPLE__ was added by me as a workaround for this bug. Basically, on Apple, we ignore CMake's built-in endianness test and instead check the __BIG_ENDIAN__ #define that the compiler provides. On the ppc pass of a universal build __BIG_ENDIAN__ is defined, and on the intel pass it is not.

Really, there isn't a need for a try_run/try_compile for endianness at all. After all, a compiler is something that changes human readable code to assembly instructions. Since it's generating the assembly, it knows all about the CPU, including the endianness!

What we are missing is a portable way to know at compile time. The __BIG_ENDIAN__ define is a gcc thing. But maybe every compiler has some sort of equivalent define?
(0036848)
Kitware Robot   
2016-08-12 09:54   
Resolving issue as `moved`.

This issue tracker is no longer used. Further discussion of this issue may take place in the current VTK Issues page linked in the banner at the top of this page.