sumatrapdfreader / sumatrapdf

SumatraPDF reader
http://www.sumatrapdfreader.org
GNU General Public License v3.0
13.26k stars 1.69k forks source link

Building with clang-cl and icx-cl within VS. #4371

Closed R-Goc closed 1 month ago

R-Goc commented 1 month ago

Is your feature request related to a problem? Please describe. I tried building the SumatraPDF solution within VS, using the clang-cl/icx-cl toolchains. They are both supposed to be a direct replacement for MSVC. However, basically all project failed to build(23 I believe), on both of them, due to warnings during compilation. Clang and icx are both shown to be often faster than MSVC, both in terms of compilation times as well as emitted binary speed.

Describe the solution you'd like I'd like some guidance on why these are behaving differently than MSVC, and how to get them to work, so that I can make it happen. Or guidance on what I'm doing wrong.

Describe alternatives you've considered I tried with both clang 19.0.0, and icx-cl form oneAPI 2024.2.0. With clang I manually changed the property pages, with icx-cl I used the built in VS extension to switch.

Additional context Commit 05a229529ec7e2b3df9b0cbd22ddbad9ffe58445 VS 2022 17.11 preview 3, I do have MSBuild support for clang-cl installed. Windows 11 Building with MSVC does work.

kjk commented 1 month ago

warnings during compilation

I do use max warnings level and I use warnings are fatal option.

Our build system: https://www.sumatrapdfreader.org/docs/Build-system

The simplest change is to remove flags { "FatalCompileWarnings" } from premake5.lua and .\doit.bat -premake to regenerate vs code files.

Or you can modify solution file manually (its /wx Treat Warnings as error setting).

If you want to help make Sumatra build with clang-cl, submit a PR with the those changes:

kjk commented 1 month ago

Also, it's an option to create variant that builds with clang-cl, probably like this:

  filter "action:vs2022"
    location "vs2022"
  filter {}

to e.g.

  filter "action:vs2022-cl"
    location "vs2022-cl"
  filter {}

I'm not opposed to having that be part of sumatra repo. It would be nice to re-use almost all the code between premake5.lua and hypothetical premake5-cl.lua so that making changes doesn't require doing the same thing in multiple files.

It's possible it could just be done inside premake5.lua by using filter "action:vs2022-cl to add clang-specific configuration changes.

R-Goc commented 1 month ago

The thing is I don't get why it doesn't work right away. If I were to guess it's probably the pragma disable warnings not working properly. Clang-cl should just be drop-in. I'll work on it

R-Goc commented 1 month ago

Nevermind, it looks like it won't be easy. Even at W0 it doesn't compile. So i don't know if it really is feasible or not.

kjk commented 1 month ago

can you post compilation log as a gist? https://gist.github.com/

kjk commented 1 month ago

A warning is either a logic bug in code (needs to be fixed) or a harmless thing (that can be silenced with a cast or some such).

Either way those should be trivial to fix. And for external libraries they can be silenced with disablewarnings because I'm not going to maintain a fork of a library just to silence the warnings.

kjk commented 1 month ago

Maybe this change will help

R-Goc commented 1 month ago

Just changing toolchain: https://gist.github.com/R-Goc/160e901b9a22b9fb03b1defd97545ee9 W0: https://gist.github.com/R-Goc/bba7897f3059639ef28fa607a9782d23 W0 + /arch=AVX2: https://gist.github.com/R-Goc/cf6131b02d291379ed47c72d4f7b22d0 W0 + /arch=AVX2 + /EHsc https://gist.github.com/R-Goc/cba163905cf3c0ea0b5f4ebc8b7be283

R-Goc commented 1 month ago

About the aes support, not sure how to set these in MSVC. gcc style is simply -maes and -mvaes if applicable

R-Goc commented 1 month ago

this is done by setting these in property pages, trying to set toolset(clang) in premake, does some weird stuff, seems like it doesn't actually set for everything, and seems further from working

kjk commented 1 month ago

I've added premake5-clang.lua and vs2022-clang. But as you noticed, toolset "clang" doesn't generate the same stuff as changing the toolset manually.

So a different option is to implement vs2022-clang in build script in go:

You can see what changes need to be done if you diff .xml files after changing toolset manually.

You can also diff vs2022-clang vs vs2022 to maybe figure out why they are different

kjk commented 1 month ago

Ok, so when you change toolset manually in vs, it only changes it on selected project, not all projects.

Which is why it doesn't fail compilation of most projects because they are not using clang-cl toolset.

That's why if you generate it via premake, they fail.

So there's more work to be done to make them work.

Warnings are easy enough to silence:

  buildoptions { "-Wno-unused-parameter -Wno-deprecated-non-prototype -Wno-deprecated-declarations", "-Wno-ignored-qualifiers", "-Wno-macro-redefined", "-Wno-switch", "-Wno-dangling-else", "-Wno-microsoft-include" }

buildoptions passes a raw cmd-line argument to clang-cl. The same can be done for the linker.

Other stuff, like complaining about exception is probably due to a #define that doesn't get set because of some macro variable difference. Or maybe it's a compiler flag that clang-cl requires.

I fixed most of clang warnings in SumatraPDF code but I'm not planning to work more on this.

R-Goc commented 1 month ago

Well, the output i posted is for all projects changed. You can select the projects and change for all at once. When just setting toolset in premake it didn't apply to all projects. I'll see if it is even feasible.

R-Goc commented 1 month ago

Digging deeper clang doesn't allow incompatible function pointer types 4>..\ext\mupdf_load_system_font.c(494,85): error : incompatible function pointer types passing 'int (const char *, const char *) __attribute__((cdecl))' to parameter of type '_CoreCrtNonSecureSearchSortCompareFunction' (aka 'int (*)(const void *, const void *)') [-Wincompatible-function-pointer-types] Which MSVC seems to allow. This is technically UB I think, and this specific case (passing cont char to qsort which expects const void) was discussed when adding incompatible function pointer types to be error by default in C. https://reviews.llvm.org/D131351 How this doesn't get picked up by clang tidy or /W4 I'm not sure for now I did this

static int stricmp_wrapper(const void* a, const void* b) {
    const char* string1 = (const char*)a;
    const char* string2 = (const char*)b;
    return _stricmp(string1, string2);
}

maybe reinterpret cast is nicer The last error I'm getting is

9>..\src\Menu.cpp(1452,32): error : conversion from 'std::nullptr_t' to 'AutoFreeStr' (aka 'AutoFree') is ambiguous
9>..\src\utils\Scoped.h(74,5): note: candidate constructor
9>..\src\utils\Scoped.h(78,5): note: candidate constructor
R-Goc commented 1 month ago

Got it working. Changed char path = tab ? tab->filePath : nullptr; to char path = tab ? tab->filePath.Get() : nullptr; in menu.cpp:+1452. That is with warnings disabled and clang-cl set for all projects manually. Now I'll try to work on the warnings

R-Goc commented 1 month ago

I changed the toolset to clang-cl manually in al the solutions in the vs-2022 folder, among other changes. For some reason the vs2022-cl projects appear corrupted, options don't actually apply. The next step would be figuring out how to make premake generate what I want, and silencing warnings on specific projects.

R-Goc commented 1 month ago

How should the warnings be disabled for each of the projects when using clang? Just using a filter in each of them?

kjk commented 1 month ago

What we do currently (i.e. disabling warning on workspace level) works for me.

If you want to be more granular, in clang_conf you can enable them and then disable all warnings that happen in any project with -Wno-* buildoptions.

Then you can reduce the warnings suppressed on workspace level and move some suppressions down to project level, like we do for cl.exe with disablewarnings.

But like I said, it's all optional.

R-Goc commented 1 month ago

Sadly disable warning doesn't seem to work, so build options it is. I think project level is the better way, as then warnings for the main project aren't silenced.

R-Goc commented 1 month ago

What is the maximum line size you use? Disabling the warnings is taking a lot more space than with MSVC.

kjk commented 1 month ago

Haven't verified it but chatgpt says:

To disable all warnings in Clang, you can use the -w command-line option

so try buildoptions {"-w"}

For readability, you can probably format like this:

    buildoptions {
        "-fms-compatibility",
        "-fms-extensions",
        "-Wno-microsoft-include",
        "-march=x86-64-v3",
        "-maes"
    }
R-Goc commented 1 month ago

Not what I meant, but thanks. I tried using disablewarnings {} in premake but that doesn't work for clang so using buildoptions {"-Wno-warningname"}. I'll give an example of the longest completed one:

buildoptions {"-Wno-unused-parameter", "-Wno-#pragma-messages", "-Wno-int-to-void-pointer-cast", "-Wno-unused-function", "-Wno-sign-compare", "-Wno-parentheses-equality", "-Wno-unused-but-set-variable", "-Wno-deprecated-copy-with-user-provided-copy", "-Wno-misleading-indentation", "-Wno-ignored-qualifiers", "-Wno-deprecated-register", "-Wno-tautological-overlap-compare"}

In this case should I split into multiple build options, or split them to be line by line? Or do you use some kind of lua formatter?

kjk commented 1 month ago

do line-by-line as I've shown above.

I don't use any lua formatter but that's an option if such a thing exists.

R-Goc commented 1 month ago

Yes, but would probably change the whole file while at it. They are more for writing programs in lua than build scripts.

R-Goc commented 1 month ago

For which projects should I not silence all warnings? As in more than what is silenced when using MSVC, as clang has a different warning structure.

kjk commented 1 month ago

Generally speaking, for Sumatra's code I prefer to fix the warnings, than silence them. I.e. utils, test_util, PdfFilter, PdfPreview, SumatraPDF, SumatraPDF-dll, MakeLZSA.

That being said, some warnings result from .h includes of third-party code so they have to be silenced.

R-Goc commented 1 month ago

It appears as though the utils has warnings as errors set again, is that by design?

project "utils"
    kind "StaticLib"
    language "C++"
    cppdialect "C++latest"
    mixed_dbg_rel_conf()
    warnings_as_errors()
kjk commented 1 month ago

It appears as though the utils has warnings as errors set again, is that by design?

project "utils"
    kind "StaticLib"
    language "C++"
    cppdialect "C++latest"
    mixed_dbg_rel_conf()
    warnings_as_errors()

Hopefully fixed

R-Goc commented 1 month ago

Here's some errors from PDFFilter. Some of them come from both includes and Sumatra code like some unused parameter warnings. https://gist.github.com/R-Goc/f2f6b5b093b378b319164d8effe2ef3b I'll see about fixing them before silencing them.

kjk commented 1 month ago

I'll accept PRs to improve this but I'm closing the issue since I don't plan to work on this myself and it's mostly working.