This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
[release/2.1] Replace hex number representation in ASM files #26136
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Are these in any of the .S files or in files included from those? |
|
Yes. For example, there's still a But I don't know how to find a comprehensive list. Does this look okay?
|
|
The .asm files are windows only. On Unix, the .S ones are used. So it seems only the .S files in src/vm/i386 need to get fixed in addition to what you've already changed. The other .S files only have constants with "h" suffix in comments, which is fine. |
This commit fixes coreclr to build in newer versions of llvm (tested
with llvm 8 on Fedora 30).
These recent versions of llvm (as well as GCC) do not accept values like
"20h" as valid integer literals:
src/debug/ee/amd64/dbghelpers.S:32:21: error: unknown token in expression
add rsp, 20h
^
This was reported as a bug to llvm upstream and they explicitly rejected
supporting these literals: https://reviews.llvm.org/D59810
This is partial backport of cbd672e
(PR dotnet#22810), with some modifications to compile, which was about adding
compatibility with GCC 5.
omajid
changed the title
|
@janvorli Can you take a look at this again? |
|
Oh, I've just realized you are trying to merge this change into release/2.1 - that is not good, it should go to master. |
janvorli
added
the
* NO MERGE *
|
I have added "NO MERGE" attribute so that it is not accidentally merged to the release/2.1 |
|
@janvorli Sorry, but I am a bit confused. The changes should already be in master as part of #22810. Is there anything you want me to do in master first? Edit: just to confirm, I just rebuilt coreclr's |
|
Hmm, I have not realized that. So I am confused - why are you porting it to release/2.1? |
|
@janvorli I would like to be able to build .NET Core 2.1 (using source-build, which includes coreclr) on Fedora 30. If coreclr's release/2.1 branch supports llvm 8 (which it does, with this small patch), I don't have to carry any patches in source-build or in Fedora to work around this issue. |
|
Any thoughts, @janvorli ? |
|
I dont' think this change would meet the bar for porting to older release branch. @jkotas do you agree? |
|
I agree. We do not patch release branches to work with newer compilers. |
|
Thanks. I am going to close this PR now. However, I really hope this policy will be revisisted - once dotnet LTS versions get included in Linux distributions, carrying patches that affect how code is compiled might break dotnet if the patches cant be reviewed and merged into the LTS release branches. |
|
A version of RHEL with clang8 will be released within the lifetime of 2.1 (as well as many other distributions will be (already are) carrying their own patches for this.) I believe that this should really be patched here because otherwise nobody will be able to build future updates of 2.1 (unless everyone patches it on their end.) |
|
@RheaAyase Ok, let me try to run this through servicing process. |
jkotas
added
Servicing-consider
|
Approved offline by @Pilchie |
danmoseley
added
Servicing-approved
|
This does not need to ship in October. It is fine to bundle with the next real change we need to ship. |
|
OK thanks |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
.NET Core 2.1 does not build with new LLVM versions (e.g. llvm 8 on Fedora 30).
Customer Impact
Anybody who wants to build .NET Core 2.1 from source on newer distros needs to carry and maintain patch with workaround. It would be beneficial for everybody to have this patch included in release/2.1 (see comment from Red Hat below #26136 (comment))
Regression?
No.
Risk
Low. This is trivial find&replace for constant literals format in asm code.
This commit fixes coreclr to build in newer versions of llvm (tested with llvm 8 on Fedora 30).
These recent versions of llvm (as well as GCC) do not accept values like
20has valid integer literals:This was reported as a bug to llvm upstream and they explicitly rejected supporting these literals: https://reviews.llvm.org/D59810
This is partial backport of cbd672e (PR #22810), with some modifications to compile, which was about adding compatibility with GCC 5.
One open question: this is enough on my machine to compile, but there are several other uses of constants with suffix
hin various files. Should I replace them with0x-prefix style too?