Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build fails for linuxbsd x86_32 arch #87442

Closed
roalyr opened this issue Jan 21, 2024 · 9 comments · Fixed by #87814
Closed

Build fails for linuxbsd x86_32 arch #87442

roalyr opened this issue Jan 21, 2024 · 9 comments · Fixed by #87814

Comments

@roalyr
Copy link

roalyr commented Jan 21, 2024

Tested versions

4.x at 0bcc0e9
(to be more prices - my 4.x fork at roalyr@647e40d)

System information

Linux mx-x230 5.10.0-27-amd64 #1 SMP Debian 5.10.205-2 (2023-12-31) x86_64 GNU/Linux

Issue description

Compilation fails with this error only for x86_32 (other architectures compile as intended):

In file included from ./core/os/memory.h:35,
                 from ./core/templates/local_vector.h:35,
                 from ./core/object/message_queue.h:36,
                 from ./core/object/object.h:35,
                 from ./core/variant/binder_common.h:35,
                 from ./core/object/method_bind.h:34,
                 from ./core/object/class_db.h:34,
                 from ./core/config/project_settings.h:34,
                 from platform/linuxbsd/crash_handler_linuxbsd.cpp:33:
./core/templates/safe_refcount.h:56:45: error: static assertion failed
   56 |  static_assert(alignof(SafeNumeric<m_type>) == alignof(m_type)); \
      |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
./core/templates/cowdata.h:49:1: note: in expansion of macro 'SAFE_NUMERIC_TYPE_PUN_GUARANTEES'
   49 | SAFE_NUMERIC_TYPE_PUN_GUARANTEES(uint64_t)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./core/os/memory.h:35,
                 from ./core/templates/local_vector.h:35,
                 from ./core/object/message_queue.h:36,
                 from ./core/object/object.h:35,
                 from ./core/variant/binder_common.h:35,
                 from ./core/object/method_bind.h:34,
                 from ./core/object/class_db.h:34,
                 from ./core/object/ref_counted.h:34,
                 from ./core/io/resource_uid.h:34,
                 from ./core/io/resource.h:34,
                 from ./core/input/input_event.h:35,
                 from ./core/input/input.h:34,
                 from platform/linuxbsd/joypad_linux.h:36,
                 from platform/linuxbsd/os_linuxbsd.h:35,
                 from platform/linuxbsd/godot_linuxbsd.cpp:31:
./core/templates/safe_refcount.h:56:45: error: static assertion failed
   56 |  static_assert(alignof(SafeNumeric<m_type>) == alignof(m_type)); \
      |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
./core/templates/cowdata.h:49:1: note: in expansion of macro 'SAFE_NUMERIC_TYPE_PUN_GUARANTEES'
   49 | SAFE_NUMERIC_TYPE_PUN_GUARANTEES(uint64_t)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scons: *** [platform/linuxbsd/crash_handler_linuxbsd.linuxbsd.editor.double.x86_32.o] Error 1
scons: *** [platform/linuxbsd/godot_linuxbsd.linuxbsd.editor.double.x86_32.o] Error 1
scons: building terminated because of errors.

Steps to reproduce

Compile source with these arguments:

scons -j2 platform=linuxbsd precision=double arch=x86_32
scons -j2 platform=linuxbsd precision=double target=template_debug arch=x86_32

Edit: same error without double precision.

Minimal reproduction project (MRP)

Not a project-related issue.

@AThousandShips
Copy link
Member

This is due to alignment requirements, does this actually happen without double precision? It shouldn't happen there arguably

This is due to the change in CowData to allow 64 bit data, unsure if this is something that can be managed and how, unsure how to enforce alignment here

CC @reduz

@roalyr
Copy link
Author

roalyr commented Jan 21, 2024

I will test and try to compile x86_32 without double precision on a vanilla 4.x.

@AThousandShips
Copy link
Member

Note that safe numerics with 64 bits are already used even in 32 bit builds and it's not necessarily safe without these guarantees, and this change arguably just exposed that factor, afaik

@roalyr
Copy link
Author

roalyr commented Jan 21, 2024

image
Same error on freshly-fetched 4.x both with and without precision=double.

@roalyr roalyr changed the title Double precision build fails for linuxbsd x86_32 arch Build fails for linuxbsd x86_32 arch Jan 21, 2024
@AThousandShips
Copy link
Member

Can you test this:

diff --git a/core/templates/safe_refcount.h b/core/templates/safe_refcount.h
index 20fb0c6501..b39d8e2d98 100644
--- a/core/templates/safe_refcount.h
+++ b/core/templates/safe_refcount.h
@@ -60,7 +60,7 @@
        static_assert(alignof(SafeFlag) == alignof(bool));

 template <class T>
-class SafeNumeric {
+class alignas(alignof(T)) SafeNumeric {
        std::atomic<T> value;

        static_assert(std::atomic<T>::is_always_lock_free);

or

diff --git a/core/templates/safe_refcount.h b/core/templates/safe_refcount.h
index 20fb0c6501..d231b0308d 100644
--- a/core/templates/safe_refcount.h
+++ b/core/templates/safe_refcount.h
@@ -61,7 +61,7 @@

 template <class T>
 class SafeNumeric {
-       std::atomic<T> value;
+       alignas(alignof(T)) std::atomic<T> value;

        static_assert(std::atomic<T>::is_always_lock_free);

And see if either solves it, the problem here is that uint64_t has a different alignment than std::atomic<uint64_t>, which is a problem and we really need to safeguard against that, but the question is where the problem itself is and what we might do to handle it

@AndreasVanVooren
Copy link

I did not mean to be a part of this conversation, but since I'm here anyway, I can confirm the issue is still there, even with the alignof statement. My compiler at least seems to take the strongest alignment possible for SafeNumeric, which is the alignment of the atomic, not the type itself. I've tried both statements listed above separately, and a combination of both, and all of them return the same error.

@akien-mga
Copy link
Member

akien-mga commented Jan 30, 2024

Confirmed, the build fails on the official buildsystem.

Full error message:

In file included from ./core/os/memory.h:35,
                 from ./core/templates/local_vector.h:35,
                 from ./core/object/message_queue.h:36,
                 from ./core/object/object.h:35,
                 from ./core/variant/binder_common.h:35,
                 from ./core/object/method_bind.h:34,
                 from ./core/object/class_db.h:34,
                 from ./core/object/ref_counted.h:34,  
                 from ./core/io/resource_uid.h:34,
                 from ./core/io/resource.h:34,
                 from ./core/input/input_event.h:35,
                 from ./core/input/input.h:34,
                 from platform/linuxbsd/joypad_linux.h:36,
                 from platform/linuxbsd/x11/display_server_x11.h:36,
                 from platform/linuxbsd/x11/display_server_x11.cpp:31:
./core/templates/safe_refcount.h:56:52: error: static assertion failed
   56 |         static_assert(alignof(SafeNumeric<m_type>) == alignof(m_type)); \
      |                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
./core/templates/cowdata.h:49:1: note: in expansion of macro 'SAFE_NUMERIC_TYPE_PUN_GUARANTEES'
   49 | SAFE_NUMERIC_TYPE_PUN_GUARANTEES(uint64_t)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./core/templates/safe_refcount.h:56:52: note: the comparison reduces to '(8 == 4)'
   56 |         static_assert(alignof(SafeNumeric<m_type>) == alignof(m_type)); \
      |                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
./core/templates/cowdata.h:49:1: note: in expansion of macro 'SAFE_NUMERIC_TYPE_PUN_GUARANTEES'
   49 | SAFE_NUMERIC_TYPE_PUN_GUARANTEES(uint64_t)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@akien-mga akien-mga added this to the 4.3 milestone Jan 30, 2024
@akien-mga
Copy link
Member

@AThousandShips Tested your diffs, they don't seem to work. It still raises the same error.

@AThousandShips
Copy link
Member

Gotcha, they were based on the assumption that the atomic had a smaller alignment than the underlying type, didn't expect std::atomic<uint64_t> to be more strongly aligned, with the new error trace that's more clear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants