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

Adding hot-patch compiler flag when compiling UE5 from source fails on multiple files. #56234

Closed
Hisamera opened this issue Jun 26, 2022 · 8 comments · Fixed by #87639
Closed
Labels
backend:X86 crash Prefer [crash-on-valid] or [crash-on-invalid] extension:microsoft

Comments

@Hisamera
Copy link

Hello

I would like to get this out first
I read contributing guideline, but because of Epic EULA I cannot just plop here it's entire source code, some assignee or contributor to LLVM can quite easliy become an Epic's developer here on Github and get the source code for self.

tl;dr UE5 with hotpatch flag fails to compile, please help or advise. For more information see attached files.

With very little changes (and if you disable few warnings, with none) you can compile UE5 source with Clang on Windows, to which I'm quite amazed, because when I tried it before (long time ago) it produced so many errors (because Epic used it's... "flavour" of C++ which was non-standard), that I quickly gave up.

Problem start's when you add to a "/hotpatch" flag to a clang-cl compiler in VCToolChain.cs file (I attached a dif of my source to a Epics commit d11782b9046e9d0b130309591e4efc57f4b8b037 (HEAD -> release, tag: 5.0.2-release, origin/release, origin/HEAD); Merge: 46544fa5e0aa 471880283c09; Author: UnrealBot unrealbot@noreply.users.github.com;Date: Thu May 26 15:05:56 2022 +0000; 5.0.2 release), after this dozens of files throw errors, but with literally thousands compiling just okay I think there may be some bug in frontend. I looked into source files that throw function compilation problems, but I am unable to find a pattern and fix it.

We can go by for now with re-launching editor every time our code changes, as LLVM toolset, and LLVM-compatible programs save us much more time, and we have do-it-in-Blueprints-first approach that we don't need it as much, but it certainly comes in handy.

We very much would like to see it's working as it allows to be OS-agnostic, as Clang is on Windows, natively on Linux, and Apple has it's own "custom" version of Clang.

I'm providing all .sh files, as you can see on them, whenever it is a unity or not build doesn't matter, and frontend exits on the same functions. Git diff of changes that I've performed, and both unity and non-unity compilation log, so you can precisely see how it compiled (or to be exact failed to compile).

If you are short on hands, I could maybe chip in something (This would be my first time dabbling in compiler code, So I just assume that even if I started doing something, someone more in-the-know would finish it faster than me) but to do it, I'd need to get some jumpstart, and by this I mean some links to documentation of how to read those errors, and what to keep in mind while doing this, but If I have to read C++ official documentation things could go rough, as I've never read it
Clang-issue.zip
.

@sylvain-audi
Copy link
Contributor

Hello,
FYI, I also encountered issues when trying to compile our project with /hotpatch flag.
There were some clang crashes, and some invalid generated code.

See the issue 59039 and the comments in patch review https://reviews.llvm.org/D137642

@Endilll
Copy link
Contributor

Endilll commented Sep 29, 2023

@Hisamera @sylvain-audi Do you still experience issues with Clang 17 or trunk?

@Endilll Endilll added crash Prefer [crash-on-valid] or [crash-on-invalid] extension:microsoft waiting-for-response and removed new issue labels Sep 29, 2023
@Endilll
Copy link
Contributor

Endilll commented Sep 29, 2023

If you can't share preprocessed source, you can reduce it yourself with creduce, and submit here a Compiler Explorer link with minimal reproducer. creduce also renames things by default, so names are not going to leak as well.

@sylvain-audi
Copy link
Contributor

sylvain-audi commented Sep 29, 2023

Hello, yes I see the issue is still present.
I took the repro from the comment in the issue 59039
-> https://godbolt.org/z/39v9hjx61 : the generated missing_tail_call is invalid as it's missing the expected tailcall and doesn,t even have a ret instruction.

It's just an example showing that the transformation pass for fms-hotpatch may generate bad code, there are certainly other cases.

On our side, we made a workaround in our LLVM fork, where we simply force the patching to systematically insert a 2-byte nop before the first instruction instead of trying to patch it, but this solution is probably not acceptable for upstream.

@sylvain-audi
Copy link
Contributor

My comments on this issue were about the bug we encountered with /hotpatch, but not in the context of building UE.

Also, the problems we had were at runtime: the compilation was successful but the generated executable had invalid code.

aganea added a commit to aganea/llvm-project that referenced this issue Apr 4, 2024
This fixes an edge case where functions starting with inline assembly would
assert while trying to lower that inline asm instruction.

After this commit, for now we always add 2 NOPs without considering the size
of the next inline asm instruction. We might want to revisit this in the
future.

This fixes Unreal Engine 5.3.2 compilation with clang-cl and /HOTPATCH.

Should close llvm#56234
@aganea
Copy link
Member

aganea commented Apr 4, 2024

I've managed to successfully compile and run UE 5.3.2 with clang-cl 18 and the PR referenced above. Also, integration with latest Live++ 2.4.1 works fine, assuming that you also use https://github.com/brickadia/LivePP2/tree/main and not Epic's LiveCoding (which hasn't been updated for more than 3 years).

@aganea
Copy link
Member

aganea commented Apr 9, 2024

@Hisamera The commit ec1af63 should fix your usage. If not, feel free to re-open this issue.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 9, 2024

@llvm/issue-subscribers-backend-x86

Author: Dorian (Hisamera)

Hello

I would like to get this out first
I read contributing guideline, but because of Epic EULA I cannot just plop here it's entire source code, some assignee or contributor to LLVM can quite easliy become an Epic's developer here on Github and get the source code for self.

tl;dr UE5 with hotpatch flag fails to compile, please help or advise. For more information see attached files.

With very little changes (and if you disable few warnings, with none) you can compile UE5 source with Clang on Windows, to which I'm quite amazed, because when I tried it before (long time ago) it produced so many errors (because Epic used it's... "flavour" of C++ which was non-standard), that I quickly gave up.

Problem start's when you add to a "/hotpatch" flag to a clang-cl compiler in VCToolChain.cs file (I attached a dif of my source to a Epics commit d11782b9046e9d0b130309591e4efc57f4b8b037 (HEAD -> release, tag: 5.0.2-release, origin/release, origin/HEAD); Merge: 46544fa5e0aa 471880283c09; Author: UnrealBot <unrealbot@noreply.users.github.com>;Date: Thu May 26 15:05:56 2022 +0000; 5.0.2 release), after this dozens of files throw errors, but with literally thousands compiling just okay I think there may be some bug in frontend. I looked into source files that throw function compilation problems, but I am unable to find a pattern and fix it.

We can go by for now with re-launching editor every time our code changes, as LLVM toolset, and LLVM-compatible programs save us much more time, and we have do-it-in-Blueprints-first approach that we don't need it as much, but it certainly comes in handy.

We very much would like to see it's working as it allows to be OS-agnostic, as Clang is on Windows, natively on Linux, and Apple has it's own "custom" version of Clang.

I'm providing all .sh files, as you can see on them, whenever it is a unity or not build doesn't matter, and frontend exits on the same functions. Git diff of changes that I've performed, and both unity and non-unity compilation log, so you can precisely see how it compiled (or to be exact failed to compile).

If you are short on hands, I could maybe chip in something (This would be my first time dabbling in compiler code, So I just assume that even if I started doing something, someone more in-the-know would finish it faster than me) but to do it, I'd need to get some jumpstart, and by this I mean some links to documentation of how to read those errors, and what to keep in mind while doing this, but If I have to read C++ official documentation things could go rough, as I've never read it
Clang-issue.zip
.

llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Apr 11, 2024
This fixes an edge case where functions starting with inline assembly
would assert while trying to lower that inline asm instruction.

After this PR, for now we always add a no-op (xchgw in this case) without
considering the size of the next inline asm instruction. We might want
to revisit this in the future.

This fixes Unreal Engine 5.3.2 compilation with clang-cl and /HOTPATCH.

Should close llvm#56234

(cherry picked from commit ec1af63)
tstellar pushed a commit to llvmbot/llvm-project that referenced this issue Apr 15, 2024
This fixes an edge case where functions starting with inline assembly
would assert while trying to lower that inline asm instruction.

After this PR, for now we always add a no-op (xchgw in this case) without
considering the size of the next inline asm instruction. We might want
to revisit this in the future.

This fixes Unreal Engine 5.3.2 compilation with clang-cl and /HOTPATCH.

Should close llvm#56234

(cherry picked from commit ec1af63)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 crash Prefer [crash-on-valid] or [crash-on-invalid] extension:microsoft
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants