search

supertuxkart / stk-code

The code base of supertuxkart
Other
4.36k stars 1.03k forks source link

The bundled bullet code fails to compile with strict-aliasing violations #5035

Open eli-schwartz opened 2 months ago

eli-schwartz commented 2 months ago

Relevant: #5

Essentially https://github.com/bulletphysics/bullet3/issues/4590

tl;dr

Gentoo is trying to get all distro packages to be LTO-clean, and as part of that we are trying to compile with -flto=4 -Werror=odr -Werror=lto-type-mismatch -Werror=strict-aliasing to check for cases where the compiler can try to optimize based on assuming UB cannot happen, and miscompile code that has UB in it. strict-aliasing issues are always bad but LTO can make them even worse.

We already track bullet as an issue -- and we have a workaround, in that we can tell the compiler "well just don't optimize this code, it's too risky". The problem here is that since bullet is part of the supertuxcart source build, we can't just build supertuxcart with optimizations, but link to an unoptimized bullet (until bullet is fixed).

You should probably track the bullet ticket, but it might also be good to somehow consider unbundling bullet if possible.

I have no idea if you still need a patched copy -- it would be great if that need is obsolete. The other reason mentioned in #5 was about using the correct version of bullet:

Updating bullet to a newer version is a major undertaking, since (in the past) the physics behaviour can change drastically from one version to the next (which is why bullet recommends not to use a system library).

I think it would be quite reasonable honestly to just... require an exact version (or range of versions) of bullet that are known to work, and allow people to configure against a system copy of that and otherwise force them to use the bundled copy.

I think "it changes drastically from version to version" is overall a weak argument against the use of system libraries.

...

Reported downstream at https://bugs.gentoo.org/458894

qwertychouskie commented 2 months ago

Un-bundling Bullet is not going to happen. Even the smallest differences between versions can create massive de-synchronizations in networked multiplayer.

Although upgrading Bullet is technically possible between 1.5 (the last planned release in the 1.x series) and 2.0, it is extraordinarily unlikely to happen, given how much it could affect the feel of the game, and how much validation (playtesting, network testing across a variety of platforms and architectures, etc) would be required. Such a major undertaking would only happen if a newer Bullet provided some feature or improvement considered important to how the game plays, not just to enable a small compile optimization.

Patches to make the bundled Bullet LTO-clean could be merged if provided, but only if they can be proven to not affect physics in any way.

Alayan-stk-2 commented 1 week ago

I think it would be quite reasonable honestly to just... require an exact version (or range of versions) of bullet that are known to work, and allow people to configure against a system copy of that and otherwise force them to use the bundled copy.

I think "it changes drastically from version to version" is overall a weak argument against the use of system libraries.

It is an extremely strong argument.

The point of system libraries is twofold:

That's it.

With modern computers, the disk space argument has become worthless in most situations, being only relevant for big libraries used by a lot of software. It is certainly worthless when it comes to bullet.

As for the ease of update, it is completely negated in situations where software behavior requires a specific version of the library, and using an updated version is at risk of breaking it. This is very much the case here.

But using system libraries opens us to:

SuperTuxKart comes with bundled libraries precisely because of all the serious bugs that were reported over the years from people using system libraries. There are also some custom changes that were made by our previous maintainer after the decision to bundle bullet.

I looked at the PR meant to fix the strict-aliasing violations, and only this commit appears directly applicable: https://github.com/bulletphysics/bullet3/pull/4594/commits/fd2ec798a26da8e4f42b09ac88afdd1e3e433660

Other commits are either not applicable at all or not applicable directly because some of the surrounding code changed.

But there might be other strict-aliasing errors in our bundled lib that don't exist in more recent versions of bullet.

eli-schwartz commented 1 week ago

The point of system libraries is twofold:

  • Save some disk space from duplication.

  • Ensure the library can be updated in a timely fashion even if the software making use of it is not actively maintained. This matters mostly for libraries that can present security risks.

  1. Ensure the library can be bugfixed once and fixed everywhere.
  2. If the library is particularly large, make the applications using it spend a lot less time compiling unchanged code after updates to the application.
  3. Save some network space from duplication.

...

Point 3 is a generalization of your sysctl.h comment.

With modern computers, the disk space argument has become worthless in most situations, being only relevant for big libraries used by a lot of software. It is certainly worthless when it comes to bullet.

Your point of view here is not universally held. However, it's also something that doesn't even slightly apply when considering network bandwidth utilization, a thing that has NOT scaled over the years as well as disk space.

eli-schwartz commented 1 week ago

Either way, I will be very happy to see fixes for the aliasing issue. :)

Alayan-stk-2 commented 1 week ago

Either way, I will be very happy to see fixes for the aliasing issue. :)

I merged the commit referenced above as it doesn't cause any compatibility concerns, but I would assume there are some strict-aliasing violations remaining.

Do you have a list of all the strict-aliasing violations present in our version?