Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Keep RTTI for Minetest IPO/LTO #268

Merged
merged 1 commit into from
Jan 6, 2024
Merged

Conversation

okias
Copy link
Contributor

@okias okias commented Jan 1, 2024

Minetest using RTTI, so we cannot apply the flag here if we want to start using IPO/LTO.

If we keep -fno-rtti here, compiler loses part of informations and cannot do efficient optimizations.

The benefit of not using RTTI on Irrlicht disappears with when Minetest and Irrlicht gets IPO/LTO linked together.

Minetest using RTTI, so we cannot apply the flag here
if we want to start using IPO/LTO.

If we keep `-fno-rtti` here, compiler loses part of informations and
cannot do efficient optimizations.

The benefit of using RTTI on Irrlicht disappears with IPO/LTO on whole binary.

Signed-off-by: David Heidelberg <[email protected]>
Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

(This also enables us to use -fsanitize=vptr UBsan, btw..)

@sfan5
Copy link
Member

sfan5 commented Jan 2, 2024

Not that it ultimately matters but this is my first time hearing of RTTI being relevant for optimization/LTO and I couldn't find anything on it.
Do you have some reference material for me?

@okias
Copy link
Contributor Author

okias commented Jan 2, 2024

Do you have some reference material for me?

The best I see is https://stackoverflow.com/questions/15101859/subclassing-class-from-shared-library-compiled-with-fno-rtti/15213718#15213718

Without LTO: We compile Irrlicht, we compile minetest, and link it together. That's all. Linker doesn't care (two types of separate objects in one file)

With LTO: Both code gets compiled and at linking time another set of optimizations is being done.
The linker sees one set of definitions from minetest (RTTI) objects and a second from Irrlicht (no-RTTI), which makes it impossible to do right optimizations + triggers ODR warnings because it sees two a bit different definitions.

@sfan5
Copy link
Member

sfan5 commented Jan 5, 2024

Hm, that's a bit weird because I'm pretty sure we don't subclass most types.

To be clear, did you check that the warnings disappear when enabling RTTI in Irrlicht?

@okias
Copy link
Contributor Author

okias commented Jan 5, 2024

To be clear, did you check that the warnings disappear when enabling RTTI in Irrlicht?

Yup.

@sfan5 sfan5 merged commit 73e62f8 into minetest:master Jan 6, 2024
14 checks passed
@okias okias deleted the drop-no-rtti branch January 6, 2024 20:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants