POV-Ray / povray

The Persistence of Vision Raytracer: http://www.povray.org/
GNU Affero General Public License v3.0
1.35k stars 282 forks source link

[BUILD][UNIX] Build fails with strict-aliasing violations #458

Open eli-schwartz opened 5 months ago

eli-schwartz commented 5 months ago

Summary

Tried to build povray with *FLAGS: -flto=4 -Werror=odr -Werror=lto-type-mismatch -Werror=strict-aliasing

The following error resulted:

x86_64-pc-linux-gnu-g++ -DHAVE_CONFIG_H -I. -I..  -I.. -I../source/backend -I../source/base -I../source/frontend -I../unix -I../vfe -I../vfe/unix -I/usr/include/SDL -D_GNU_SOURCE=1 -D_REENTRANT -I/usr/lib64/../include  -DPOVLIBDIR=\"/usr/share/povray\" -DPOVCONFDIR=\"/etc/povray\" -pthread -I/usr/include  -I/usr/include  -Wno-multichar -Wno-write-strings -fno-enforce-eh-specs -march=native -fstack-protector-all -O2 -pipe -fdiagnostics-color=always -frecord-gcc-switches -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-clash-protection -flto=4 -Werror=odr -Werror=lto-type-mismatch -Werror=strict-aliasing -Wformat -Werror=format-security -pthread -c -o base/povms.o base/povms.cpp
In file included from base/configbase.h:151,
                 from base/povms.cpp:42:
base/povms.cpp: In function ‘void POVMSStream_Init()’:
./base/povms.h:97:53: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
   97 |                 #define HexToPOVMSIEEEFloat(h, f) *((int *)(&f)) = h
      |                                                    ~^~~~~~~~~~~~
base/povms.cpp:1038:9: note: in expansion of macro ‘HexToPOVMSIEEEFloat’
 1038 |         HexToPOVMSIEEEFloat(0x44663355, data_ieeefloat); // 0x44663355 equals 920.802063
      |         ^~~~~~~~~~~~~~~~~~~
cc1plus: some warnings being treated as errors

POV-Ray Version

Build Environment

wfpokorny commented 5 months ago

Thank you for reporting the issue.

I'm just a user with my own fork. Plus, my version of the messaging system is passing around doubles rather than floats but, that said, I think changing the code to that below a valid solution for official versions of POV-Ray.

//  See: https://github.com/POV-Ray/povray/issues/458
//  for why HexToPOVMSIEEEFloat macro replaced with code below.
//  HexToPOVMSIEEEFloat(0x44663355u, data_ieeefloat);

    static_assert(sizeof(POVMSIEEEFloat) == 4);
    const uint32_t ByOrdTest = 0x44663355u;
    union uC {
       uint32_t buffer;
       float    value;
    } u;
    u.buffer       = ByOrdTest;
    data_ieeefloat = u.value;

Strictly, this is still type-punning, but gcc / g++ guarantee the through a union form works properly.

Other rambling. It looks to me like the code here builds up methods to unscramble bytes arbitrary scrambled. Today, on a single machine this very likely only big endian vs little endian differences which can be tested for in other ways. The messaging code was set up to unscramble what might be happening with multiple machines of differing types as well as any scrambling due network transmission. POV-Ray on the whole was never completely fleshed out to run in this many machines across a network manner - as far as I know. Do we still need the arbitrary byte unscrambling?

tribad commented 5 months ago

Hi, you can use memcpy for that.

If the optimization level is somewhat higher, I checked it with -O2, it will be reduced to an assignment.

#include <stdio.h>
#include <memory.h>
#include <stdint.h>

int main(void) {
   int32_t i =0xdeadbeaf;
   float   f;

   memcpy(&f, &i, sizeof(f));
   printf("%f\n", f);
   return 0;
}

will be optimized to :

0000000000000000 <main>:
   0:   48 83 ec 08             sub    $0x8,%rsp
   4:   48 8d 3d 00 00 00 00    lea    0x0(%rip),%rdi        # b <main+0xb>
   b:   b8 01 00 00 00          mov    $0x1,%eax
  10:   f2 0f 10 05 00 00 00    movsd  0x0(%rip),%xmm0        # 18 <main+0x18>
  17:   00 
  18:   e8 00 00 00 00          call   1d <main+0x1d>
  1d:   31 c0                   xor    %eax,%eax
  1f:   48 83 c4 08             add    $0x8,%rsp
  23:   c3                      ret

At offset

Only a hint. But write a comment that it will be optimized, if you let the compiler do so.

eli-schwartz commented 5 months ago

Yes, both unions and memcpy are the officially sanctioned way to do this without invoking UB. Both will be optimized by the compiler to the exact machine code that aliasing violations were hoping to produce.

I will be honest: I mostly submitted this bug report out of a sense of obligation and for tracking purposes. Given recent activity I did not have high hopes of it being fixed. :D Still, stranger things have happened... the original authors might come back...