-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Because I'm having some annoying compiler warnings when compiling the subversion classical VM squeak 4.19.2, I'm checking the FloatMathPlugin.
First of all the problem is the following, about 70 errors like this:
"./platforms/Cross/plugins/FloatMathPlugin/fdlibm/fdlibm.h", line 19: warning: m acro redefined: __LITTLE_ENDIAN
The Makefile for building the FloatMathPlugin sets
./i86/FloatMathPlugin/CMakeFiles/FloatMathPlugin.dir/flags.make:C_DEFINES = -DNO _ISNAN=1 -DSQUEAK_BUILTIN_PLUGIN=1 -D__LITTLE_ENDIAN=1
The flag -D__LITTLE_ENDIAN=1 comes from cmake 's config.cmake:
In platforms/unix/plugins/FloatMathPlugin/config.cmake you can see
PLUGIN_DEFINITIONS (-D__LITTLE_ENDIAN=1)
Now at the same time the fdlibm.h header file redefines the __LITTLE_ENDIAN:
#if defined(i386) || defined(i486) || \ defined(intel) || defined(x86) || defined(i86pc) || \ defined(__alpha) || defined(__osf__) #define __LITTLE_ENDIAN #endif
So this causes lots of harmless but annoying,
"./platforms/Cross/plugins/FloatMathPlugin/fdlibm/fdlibm.h", line 19: warning: m acro redefined: __LITTLE_ENDIAN
I'd like to get rid of those warnings by fixing the fdlibm.h.
If I compare platforms/Cross/plugins/FloatMathPlugin/fdlibm/fdlibm.h in the subversion sources (for Squeak 4.19.2), and in the opensmalltalk git sources (for Squeak Cog 5.0 ), then I notice major differences.
In both cases the head of the fdlibm.h file is the same:
/* @(#)fdlibm.h 1.5 04/04/22 */ /* * ==================================================== * Copyright (C) 2004 by Sun Microsystems, Inc. All rights reserved. * * Permission to use, copy, modify, and distribute this * software is freely granted, provided that this notice * is preserved. * ==================================================== */
However if I "diff" the fdlibm.h from OpenSmalltalk and the one that is in Subversion, they seem very different.
For example the OpenSmalltalk fdlibh.h has
#ifdef __cplusplus
while that is not present in the subversion 4.19.2 sources for the fdlibm.h.
As an experiment I copied the one from OpenSmalltalk to the VM 4.19.2:
bash-4.4$ cp $HOME/src/opensmalltalk/platforms/Cross/third-party/fdlibm/fdlibm.h $HOME/src/squeak4vm/platforms/Cross/plugins/FloatMathPlugin/fdlibm/fdlibm.h
If you run then
svn diff
you gets lots of differences between the fdlibm.h from OpenSmalltalk, and the one that is in subversion.
However the OpenSmalltalk fdlibm.h is NOT compiling in the classical VM 4.19.2.
I can fix the compiler warnings by the following small patch:
bash-4.4$ svn diff fdlibm.h Index: fdlibm.h =================================================================== - --- fdlibm.h (revision 3790) +++ fdlibm.h (working copy) @@ -16,8 +16,10 @@ #if defined(i386) || defined(i486) || \ defined(intel) || defined(x86) || defined(i86pc) || \ defined(__alpha) || defined(__osf__) +#ifndef __LITTLE_ENDIAN #define __LITTLE_ENDIAN #endif +#endif
With the above patch I get a clean build for the FloatMathPlugin.
It results in a clean build for both 64bit and 32bit.
Which is important as the CMAKE comment indicates the cmake flag is set, precisely for the 64bit case (the 64bit case was not dealt with by the SunOS 2004 header file).
So what happened I think is that the cmake settings was done for 64bit, and it started to cause a lot of redefined macro warnings in the 32bit case.
Would it please be possible to commit the above small patch to the subversion sources ?
This would suppress the compiler warnings.
Thanks ! David Stes
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
In attach the patch for fdlibm.h ... Hopefully the attachement is posted, not sure whether the list accepts this.
For "subversion" sources at http://squeakvm.org/svn/squeak.
David Stes
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
If there is someone with access to http://squeakvm.org/svn/squeak (subversion), can they please have a look at the issue ?
It seems simple to fix.
Thanks, David Stes
Hi David,
On Sun, Dec 13, 2020 at 04:50:46PM +0100, stes@PANDORA.BE wrote:
If there is someone with access to http://squeakvm.org/svn/squeak (subversion), can they please have a look at the issue ?
It seems simple to fix.
Thanks, David Stes
I tried applying the patch, and it compiles and runs on my Linux box.
However if I am reading this right, the patch forces __LITTLE_ENDIAN to be defined for all platforms, so that would break big endian systems. Granted, there are no big endian platforms in common use these days, but it still doesn't seem like the right way to deal with it.
Am I right in understanding that the original code causes compiler warnings on Solaris, but that it compiles and works correctly despite the warnings? If so, I would honestly suggest just ignoring the warnings.
Otherwise if it is a functional problem, maybe there is a way that we can make the build system ensure a correct setting for __LITTLE_ENDIAN. I have not looked at this, but I suspect there will be a way that we could make it work.
Dave
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
The #ifndef __LITTLE_ENDIAN only checks whether __LITTLE_ENDIAN is NOT defined (ifndef=ifnotdefined).
Note that this is inside a code block that is #if defined(i386).
Basically they #define __LITTLE_ENDIAN in the common case of i386.
However this causes tons of warnings because at the cmake level, __LITTLE_ENDIAN is already defined (on the C compiler command line).
So this patch is not breaking the big endian case (I may try SPARC someday), and also it does not break the x64 case.
Note that the cmake config file indicates a change was made to fix the x64, which I think then had this unfortunate effect on the i386 case.
So if possible please apply the patch.
It fixes lots of warnings for me that looked quite suspicious to me ...
David Stes
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
In the cmake.config
platforms/unix/plugins/FloatMathPlugin/config.cmake
it says:
# fdlibm.h does not recognize x86_64, so set endianness here for all platforms.
TEST_BIG_ENDIAN (IS_BIG_ENDIAN) IF (NOT IS_BIG_ENDIAN) PLUGIN_DEFINITIONS (-D__LITTLE_ENDIAN=1) ENDIF ()
So basically the cmake is testing IS_BIG_ENDIAN and if not so, then it defines __LITTLE_ENDIAN.
That's fine but the problem is that the old code in the fdlibm.h header is #define __LITTLE_ENDIAN which (1) is a duplicate def and (2) defines to a different value (because it does not define it =1).
So assuming you want to keep the definition at the cmake level (for x64), then in the header file it should be conditional
#ifndef __LITTLE_ENDIAN (if not already defined on the command line)
David Stes
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
By the way, regarding the "big endian" case, such as the SPARC processor, I may ask around maybe I can find a helpful soul who compiles it on SPARC.
Could be interesting, and it may work ... it has worked in the past, so why not still today ...
In any case my patch was not intended for the big endian case, but it also does not break the big endian case.
David Stes
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
I had to run the file through tr -d '\015' < fdlibm.h to delete CR (carriage-return).
Possibly there are combinations of CR and NL in the file.
It may also be because of my download of the file.
But besides that - after deleting CR - I had a svn diff, and it is close to what I submitted.
I now get
bash-4.4$ svn diff Index: fdlibm.h =================================================================== - --- fdlibm.h (revision 3790) +++ fdlibm.h (working copy) @@ -13,11 +13,13 @@ /* Sometimes it's necessary to define __LITTLE_ENDIAN explicitly but these catch some common cases. */
+#ifndef __LITTLE_ENDIAN #if defined(i386) || defined(i486) || \ defined(intel) || defined(x86) || defined(i86pc) || \ defined(__alpha) || defined(__osf__) #define __LITTLE_ENDIAN #endif +#endif
#ifdef __LITTLE_ENDIAN
That is OK by me. This compiles OK now with the SunPRO C compiler.
I checked both 32bit and 64bit because the Makefile first descends in build and then in build64.
David Stes
On Sun, Dec 13, 2020 at 08:09:27PM +0100, stes@PANDORA.BE wrote:
I had to run the file through tr -d '\015' < fdlibm.h to delete CR (carriage-return).
Possibly there are combinations of CR and NL in the file.
It may also be because of my download of the file.
Yes it was probably just an artifact of the download.
But besides that - after deleting CR - I had a svn diff, and it is close to what I submitted.
I now get
bash-4.4$ svn diff Index: fdlibm.h ===================================================================
- --- fdlibm.h (revision 3790)
+++ fdlibm.h (working copy) @@ -13,11 +13,13 @@ /* Sometimes it's necessary to define __LITTLE_ENDIAN explicitly but these catch some common cases. */
+#ifndef __LITTLE_ENDIAN #if defined(i386) || defined(i486) || \ defined(intel) || defined(x86) || defined(i86pc) || \ defined(__alpha) || defined(__osf__) #define __LITTLE_ENDIAN #endif +#endif
#ifdef __LITTLE_ENDIAN
That is OK by me. This compiles OK now with the SunPRO C compiler.
I checked both 32bit and 64bit because the Makefile first descends in build and then in build64.
David Stes
I just committed your fix to SVN, revision 3791.
Thanks, Dave
On Sun, Dec 13, 2020 at 07:03:47PM +0100, stes@PANDORA.BE wrote:
In the cmake.config
platforms/unix/plugins/FloatMathPlugin/config.cmake
it says:
# fdlibm.h does not recognize x86_64, so set endianness here for all platforms.
TEST_BIG_ENDIAN (IS_BIG_ENDIAN) IF (NOT IS_BIG_ENDIAN) PLUGIN_DEFINITIONS (-D__LITTLE_ENDIAN=1) ENDIF ()
So basically the cmake is testing IS_BIG_ENDIAN and if not so, then it defines __LITTLE_ENDIAN.
That's fine but the problem is that the old code in the fdlibm.h header is #define __LITTLE_ENDIAN which (1) is a duplicate def and (2) defines to a different value (because it does not define it =1).
So assuming you want to keep the definition at the cmake level (for x64), then in the header file it should be conditional
#ifndef __LITTLE_ENDIAN (if not already defined on the command line)
David Stes
Oh, that is good, the config.cmake is already handling it. I like your idea of using #ifndef __LITTLE_ENDIAN to prevent redefinition in fdlibm.h. It is a minimal change to the original Sun code and it will have no adverse effect on other platforms.
I tried this on my Linux box. It compiles without warnings, and all of the FloatMathPluginTests tests are green (these are in the VMMaker package, not Squeak trunk).
It's a trivial change but I'm attaching a copy of the file I used for reference. If you can confirm that this does what is needed on Solaris, I'll push the update to SVN for you.
Thanks! Dave
vm-dev@lists.squeakfoundation.org
-
David T. Lewis -
stes＠PANDORA.BE