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

Recompile libdxcompiler.so to remove depencency on libtinfo.so #47

Closed

Conversation

expenses
Copy link
Contributor

@expenses expenses commented Feb 3, 2022

At the moment, attempting to run kajiya on my arch linux install (on #46) fails with:

A turbosloth LazyWorker "kajiya_backend::shader_compiler::CompileShader" failed

Caused by:
    Failed to load library "./libdxcompiler.so": DlOpen { desc: "libtinfo.so.5: cannot open shared object file: No such file or directory" }

Running ldd libdxcompiler.so lists the following dynamic library dependencies:

        linux-vdso.so.1 (0x00007ffc0df9b000)
	librt.so.1 => /usr/lib/librt.so.1 (0x00007f28f59e1000)
	libdl.so.2 => /usr/lib/libdl.so.2 (0x00007f28f59da000)
	libtinfo.so.5 => not found
	libpthread.so.0 => /usr/lib/libpthread.so.0 (0x00007f28f59b9000)
	libz.so.1 => /usr/lib/libz.so.1 (0x00007f28f599f000)
	libm.so.6 => /usr/lib/libm.so.6 (0x00007f28f585b000)
	libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x00007f28f5643000)
	libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0x00007f28f5628000)
	libc.so.6 => /usr/lib/libc.so.6 (0x00007f28f545c000)
	/usr/lib64/ld-linux-x86-64.so.2 (0x00007f28f710d000)

The missing one, libtinfo.so.5 is not installable on arch. The core/ncurses package (version 6.3-1) provided libtinfo.6, but this newer version is not compatible. This library is literally only used for the terminalHasColors function and is useless in libdxcompiler.so which should not be printing anything. The solution to this is to either bundle libtinfo.5 which is gross, or to recompile libdxcompiler.so without terminfo like so:

git clone https://github.com/Microsoft/DirectXShaderCompiler --recursive
cd DirectXShaderCompiler
mkdir build
cd build
cmake .. -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_TERMINFO=OFF -C ../cmake/caches/PredefinedParams.cmake
ninja

The generated library no longer depends on it:

        linux-vdso.so.1 (0x00007ffdea17a000)
	librt.so.1 => /usr/lib/librt.so.1 (0x00007f0ba0958000)
	libdl.so.2 => /usr/lib/libdl.so.2 (0x00007f0ba0951000)
	libpthread.so.0 => /usr/lib/libpthread.so.0 (0x00007f0ba0930000)
	libz.so.1 => /usr/lib/libz.so.1 (0x00007f0ba0916000)
	libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x00007f0ba0700000)
	libm.so.6 => /usr/lib/libm.so.6 (0x00007f0ba05bc000)
	libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0x00007f0ba059f000)
	libc.so.6 => /usr/lib/libc.so.6 (0x00007f0ba03d3000)
	/usr/lib64/ld-linux-x86-64.so.2 (0x00007f0ba24e4000)

The compiled library is a bit bigger, but I believe that is because the original one was compiled on Windows or something along those lines.

Instead of merging in random binaries it might be better for someone at Embark to recompile it themselves.

@expenses expenses marked this pull request as ready for review February 3, 2022 23:18
@h3r2tic
Copy link
Collaborator

h3r2tic commented Feb 4, 2022

Thanks for pointing this out and showing how to fix the issue! Building it on Linux looks way easier than on Windows xD

Hmm... maybe it would be possible to set up some automated CI process to do this because you're quite right about checking in fat binaries.

Regarding libtinfo.so.5, have you tried https://aur.archlinux.org/packages/ncurses5-compat-libs? I think I may have used it on Arch last time I was testing it.

Also, is the binary in this PR a fully stripped one? That's quite a big size difference :o
(That being said, Unreal ships with a 600MB libdxcompiler.so, so what is a 7MB diff 🤷🤣)

@expenses
Copy link
Contributor Author

expenses commented Feb 4, 2022

I copied the CI for DirectXShaderCompiler and compiled it clang instead of gcc and that resulted in a bit of a size reduction:
cmake .. -GNinja -C ../cmake/caches/PredefinedParams.cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DENABLE_DXIL2SPV=ON -DLLVM_ENABLE_TERMINFO=OFF

Regarding libtinfo.so.5, have you tried https://aur.archlinux.org/packages/ncurses5-compat-libs? I think I may have used it on Arch last time I was testing it.

I did also check this package just now and it does provide libtinfo.so.5 which is nice!

@expenses
Copy link
Contributor Author

expenses commented Feb 4, 2022

None of the binaries (DirectXShaderCompiler CI artifacts or compiled by me) are stripped but that's an option if the size really matters

Copy link
Collaborator

@h3r2tic h3r2tic left a comment

Choose a reason for hiding this comment

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

Okays, this size is quite reasonable! Thanks! Will merge after testing on Arch 🙂

@MarijnS95
Copy link

MarijnS95 commented Mar 7, 2023

Upstream also disabled LLVM_ENABLE_TERMINFO by default now microsoft/DirectXShaderCompiler#4908 🥳! Note that when you update, you'll also have to bump hassle-rs to 0.10 as we finally solved vtable ABIs on Linux:

microsoft/DirectXShaderCompiler@47f3137
Traverse-Research/hassle-rs#50
https://github.com/Traverse-Research/hassle-rs/#supported-dxc-versions-on-non-windows

(I'm randomly looking around the Kajiya repo for DXC changes/updates after receiving a bug report on both these issues on the hassle-rs repo because the library was pulled from this repo 😬)

@h3r2tic
Copy link
Collaborator

h3r2tic commented Mar 7, 2023

🥳 🎉 that's great, thanks for letting us know ❤️ Sounds like an upgrade is in order!

@MarijnS95
Copy link

Yup, as long as your shaders keep compiling... 😬 😰

@MarijnS95
Copy link

@h3r2tic I see you already started pushing to https://github.com/EmbarkStudios/kajiya/compare/hassle-rs-10, nice stuff (but I don't recall us switching all our ternary if's to select()...)! Don't forget to remove the libtinfo5 dependency from the readme though as per this PR; I double-checked your branch with ldd to make sure it is indeed gone!

@h3r2tic
Copy link
Collaborator

h3r2tic commented Mar 9, 2023

Hah, nice spying @MarijnS95, and thanks for the tip xD I also checked by running kajiya on the Steam Deck which doesn't have libtinfo xD (unfortunately the mesa driver that the Deck uses crashes hard on ray tracing)

... how did you manage to dodge switching your ternary operators to select? That change was so annoying >__<'

@h3r2tic h3r2tic mentioned this pull request Mar 9, 2023
@h3r2tic
Copy link
Collaborator

h3r2tic commented Mar 9, 2023

Maybe you didn't use ternary with vector types?

@MarijnS95
Copy link

Ouuu, our RayTracing app ran on Steam Deck... half a year ago... in fact I found a Swapchain issue that may explain a gamma mismatch today and was about to retest it 😂.

... how did you manage to dodge switching your ternary operators to select? That change was so annoying >__<'

What error are you getting? We just... didn't have to do it... but we are on microsoft/DirectXShaderCompiler@0392e60 from Nov 11th so it may have been a more recent change that causes it.

Maybe you didn't use ternary with vector types?

Just checked and we definitely are. Will keep you posted after pulling in a more recent version, we want to get rid of ncurses5 as well!

@h3r2tic
Copy link
Collaborator

h3r2tic commented Mar 9, 2023

Ouuu, our RayTracing app ran on Steam Deck... half a year ago... in fact I found a Swapchain issue that may explain a gamma mismatch today and was about to retest it joy.

Wat. I'm getting horrible crashes as soon as I even try and trace shadows 😂 Depending on the RADV_PERFTEST env variable I'm either getting a TDR with a few screen blanks, and the app closing down, oooooor the screen flashing white and the whole Deck rebooting 😆

... how did you manage to dodge switching your ternary operators to select? That change was so annoying >__<'

What error are you getting? We just... didn't have to do it... but we are on microsoft/DirectXShaderCompiler@0392e60 from Nov 11th so it may have been a more recent change that causes it.

Don't remember the exact text, but basically the error literally told me to use select.

Ah, there we go, same as here: GPUOpen-Effects/FidelityFX-FSR#28

Maybe you didn't use ternary with vector types?

Just checked and we definitely are. Will keep you posted after pulling in a more recent version, we want to get rid of ncurses5 as well!

Hah, then prepare for the same fun I had!

@MarijnS95
Copy link

Wat. I'm getting horrible crashes as soon as I even try and trace shadows Depending on the RADV_PERFTEST env variable I'm either getting a TDR with a few screen blanks, and the app closing down, oooooor the screen flashing white and the whole Deck rebooting

Hah, I'll keep you posted whether I can get anything to run... But we do run on phones (Samsung Xclipse 920) too already though 😬 (probably need a fresh issue/discussion for this instead of sidetracking here).

Don't remember the exact text, but basically the error literally told me to use select.

Ah, there we go, same as here: GPUOpen-Effects/FidelityFX-FSR#28

Ooooeh, using a vector in the conditional part; I only see us using vectors in the true/false expression bit which should still be fine 😬

Hah, then prepare for the same fun I had!

No worries about fun. DXC will give me plenty. We're already starting with:

bindless.hlsl:110:46: error: variable 'original' is uninitialized when used here [-Werror,-Wuninitialized]
        buffer.InterlockedAdd(offset, value, original);
                                             ^~~~~~~~
bindless.hlsl:109:22: note: initialize the variable 'original' to silence this warning
        uint original;
                     ^
                      = 0

https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/interlockedadd the third argument is clearly annotated as out, not inout, so I don't see why it wants it to be initialized.

@h3r2tic
Copy link
Collaborator

h3r2tic commented Mar 10, 2023

Hah, I'll keep you posted whether I can get anything to run... But we do run on phones (Samsung Xclipse 920) too already though grimacing (probably need a fresh issue/discussion for this instead of sidetracking here).

Sounds good, thanks :D

Don't remember the exact text, but basically the error literally told me to use select.
Ah, there we go, same as here: GPUOpen-Effects/FidelityFX-FSR#28

Ooooeh, using a vector in the conditional part; I only see us using vectors in the true/false expression bit which should still be fine grimacing

Yeah, vectors as the conditional part was the fun thing about the old ternary :P

Hah, then prepare for the same fun I had!

No worries about fun. DXC will give me plenty. We're already starting with:

bindless.hlsl:110:46: error: variable 'original' is uninitialized when used here [-Werror,-Wuninitialized]
        buffer.InterlockedAdd(offset, value, original);
                                             ^~~~~~~~
bindless.hlsl:109:22: note: initialize the variable 'original' to silence this warning
        uint original;
                     ^
                      = 0

https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/interlockedadd the third argument is clearly annotated as out, not inout, so I don't see why it wants it to be initialized.

That's weird indeed! At least I didn't get any of those 🙈

@h3r2tic
Copy link
Collaborator

h3r2tic commented Mar 10, 2023

Closing in favor of #76

@h3r2tic h3r2tic closed this Mar 10, 2023
@MarijnS95
Copy link

Nice! Fortunately we're already working with a friendly DXC developer to get the issue fleshed out :)

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

Successfully merging this pull request may close these issues.

3 participants