FloatMathPlugin: patch for squeak 4.19.2 fdlibm.h header

 
-----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 SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJfzLGjAAoJEAwpOKXMq1MaYu8H/0qGt9eBZdgBADEg8tptKnSb
5mCSxSMybnSG1rRolnp9UG+8jRd8f0gwjlUuv2UvylV6bZekbGXicNFvTG/nxF6L
htP65CVftVbdwwmt+wPa9juSBhsAFy7qcYF7YZeUJ4jLq7z8fFLI3Zid1Wco3Vb9
cyPUcKMTZ+xgPQ/b9DpHlScPX0NoZQranKQe3Xa5vnczewyxcigBRQ/wtevg8gl1
ormfE218TzCjvdRPjfxCvkybtmsbzLhMdlq7Ymih71xD+4VvrFZd4rNn8YAQddPe
ScJxyw4nUXO8lLnG2DZeW15xSpGtyAujL4SlY6XLvu2E88WQI6PRYLrSimemXjU=
=0zl1
-----END PGP SIGNATURE-----

 

-----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 SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJfz7E6AAoJEAwpOKXMq1MaT5wH/3C7UnH6HaxSAE3a5AkQ4Q65
Enble1+u8YuHkJOyBGHVOgWNYQ4E09EM1XckpOGFhNpYy8IlRvtV+ReivIHXLpkC
SfSUkKbQx9CzIu/sc3JQKKZ8r4y1P13S6hOv16B5h8giPZC6TpYL18681g7hRsq7
TrXlrDj65BYEWXnTyIEL/znrTFpt5kOwMg4H0iZ7sWi0KLDRc7xumA/O1XXPmWV/
kd8oYX7pH8wpUqjMKlncrAyJUg2gQqJAvGzN/HYpRnK2xRmEfxtWCN0gQ5wnQD5a
Dq5pqgqB0gIwWKS+4J0cHq+s6vNlCh8IlvI1OMMAPJVXvhqYEDpXvrwKq/OCxdc=
=ckvr
-----END PGP SIGNATURE-----

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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJf1jgGAAoJEAwpOKXMq1MaXNIIAJoyCpMjAS9HzY8HC8wlG/Wk
VP2sqoQlEoS0x2eDT9PSKnFjOzGQaFEdgd8BfUKso18/ZbGNTUAA6TGJKrLjtEi+
LCKdPxUdkHd6sQg3tlbJDI4K9lyIeY3AB4pj7ygJ/+6OhHuowX2u/iTyXQL+kLe4
N1UQFWD8DKeHQkIVLBOIq0VOf8o3ffAr5mbYe1oWHHwbj5bKGT7s6S+XkXAEh1tG
Yx+uIhzOJcz1BOfNPF74uMZjF5wbC1S/N1lRPt7yxtOpwAxGG1Z2BCrKW9LwWkFK
8vqX4SXS0UwNzmkpNsgwqdJ1ZyB0uBjEs22KcPCNTSnDVnoKK/6zRjU7RWpyvoY=
=PG3Q
-----END PGP SIGNATURE-----

 
Hi David,

On Sun, Dec 13, 2020 at 04:50:46PM +0100, [hidden email] wrote:
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 SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJf1lWSAAoJEAwpOKXMq1Mae1gIAJu3x/immP3Jx7L6kqJRoopR
OQJQ3fltw9b37i7p1FSoZd3kdOZxe6oihREDZb6Ay8DegI0HFrGlyIib3HTKBCGU
DCPCSlw/5hFHhvhdzsjk++dqkq3XHPU9mSYPl9Wz6e4a3ceNBLxmoVCauHPZHiPu
PLLe5EDFgsMKIQtL62EU3ySXfGw60UgEHQpr/Q7l4SgNfHA3dTJjq7yoSdHBO/dr
xN9pqQQA500QeYdf8XPFrHQbonOn4iclDKEs+aDKFkumYRcKg57vLp3KiN2OwCIU
aMft14o4ISh0iK9uGe3oebdnTVNSkFb0fwQKYzEW1oOFCoYRm+ZCsNQF5UP2X5o=
=VNaO
-----END PGP SIGNATURE-----

 
-----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 SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJf1ldjAAoJEAwpOKXMq1MaQEIH/0CafOdErjdTHcJmwxSz/ZR6
NrmYXFzB2uZZpGiGETVVZl73fmC0TlQ4tBxd5jVacZxoeCV+LpU4D61PP3cGVJMj
elpjhy8UWvOinzqKhBjN8Xl7AdaJzugJLp0s8Ovb9St2Y41HIRjQZCCO47FdRB1T
zY87dehH68lPIZ9/HCc0YB5+HZvuIV0uMJMmI3YdmwsBnuMfcaMmnkhKfk65Lt0j
h9Ehp2q+NHfxHbJu0Xxab8TEfZlWVFH9aVJsOQ/GFe6Wdo+J+sq51AshLyYIbB3t
F1Tis4l0Zrghxm+oDIJ+EmvhZcrPE29Fo0eHXFJ4Tsq0wUqR9Kj/dl3hVFNGlok=
=40Mx
-----END PGP SIGNATURE-----

 
-----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 SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJf1llmAAoJEAwpOKXMq1MaNK8H/0QWfNe5r9rx9anMU4DPH4y1
My5dAA9ttJBrOHsEzpPIXOgHdLgzARV2v9gSxh89Irp3ACjF0KCfBYZ6keGYq/cG
TeHq75Y1dr3GVZkJ1F2lmvzswSvSBueDXQhfmZq891/dtFmBu0ziO6e/WPxmR4e5
YtA+elYH98jk+8JJbciJFt32UqlQZb+OPnUf/QnuUPaqAPDdK9u9RwdeWKpInc4H
sfZTcdRKgq5FLKai+JNZKd4X1Jhy+Xc05y0ttujErlbvrjKznCo/KJlxA+WUaEV2
U93doL5hiKt1yMeFkDMzfliCOAGoDbQCsPW9fMmaaJD0FGX3gRegiXkoOuqO2GQ=
=mkSh
-----END PGP SIGNATURE-----

 
On Sun, Dec 13, 2020 at 07:03:47PM +0100, [hidden email] wrote: 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
 

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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJf1mWgAAoJEAwpOKXMq1MaWtwIAI+a9/PqKASc/zfGE+FoTHvw
4QUmFwB47ssJJG9HGLwbC7XAKQvwGYekGq8jf4POfBCyI7dZpG4ISZwWGb3XTwt3
RcN4xR3QI+/raRS0qGD4ZQaNkBnUFVqpDtq+ruKEboOFPpJpZknqXMFfsM/asPKk
oxKtsxPczbL3W9OoeSgsw4LGHkTZoPzunGpgqv2Re/574OJdse4iy4ln4MO2CcVG
wWBcC2JRtZLqe/yfF+jZQ9kg7OmjBwpC/c3GPCZbeIZipRFXtz5ekvovYNlq0ywB
UC2SEIGt4TyY7tI/JCJ/xsKPkEtUZ/WVA7lXsBkwHRNPzrXNY/xTfmr77yGqhjw=
=Q7tY
-----END PGP SIGNATURE-----

 
On Sun, Dec 13, 2020 at 08:09:27PM +0100, [hidden email] 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.


I just committed your fix to SVN, revision 3791.

Thanks,
Dave