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

[Cygwin] Cygwin X86ISelDAGToDAG.cpp #76284

Closed
wants to merge 1 commit into from

Conversation

xu-chiheng
Copy link

@xu-chiheng xu-chiheng commented Dec 23, 2023

Fix the regression caused by commit ec92d74 2023-12-14, that, in Cygwin, Clang can't build binutils 2.42.
configure:4686: checking whether we are cross compiling
configure:4694: clang -o conftest.exe -march=x86-64 -O3 -Wl,--strip-all conftest.c >&5
/cygdrive/c/Users/ADMINI~1/AppData/Local/Temp/conftest-385c4a.o:conftest.c:(.text+0x10): relocation truncated to fit: IMAGE_REL_AMD64_ADDR32 against .rdata' /cygdrive/c/Users/ADMINI~1/AppData/Local/Temp/conftest-385c4a.o:conftest.c:(.text+0x15): relocation truncated to fit: IMAGE_REL_AMD64_ADDR32 against .rdata'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
configure:4698: $? = 1
configure:4705: ./conftest.exe
../binutils/configure: line 4707: ./conftest.exe: No such file or directory
configure:4709: $? = 127
configure:4716: error: in /cygdrive/e/Note/Tool/binutils-release-build': configure:4718: error: cannot run C compiled programs. If you meant to cross compile, use --host'.
See `config.log' for more details

I have build scripts and patches at https://github.com/xu-chiheng/Note .
I can use the build scripts and patches to build and bootstrap GCC(start from 13.0.0) and Clang/LLVM(start from 16.0.0), using GCC or Clang, on Cygwin and MinGW.
If you have interests, you can look at my other PRs at https://github.com/llvm/llvm-project/pulls/xu-chiheng .

Most patches come from upstream at

https://cygwin.com/git-cygwin-packages/
https://cygwin.com/git-cygwin-packages/?p=git/cygwin-packages/clang.git;a=summary
git://cygwin.com/git/cygwin-packages/clang.git
https://cygwin.com/git-cygwin-packages/?p=git/cygwin-packages/llvm.git;a=summary
git://cygwin.com/git/cygwin-packages/llvm.git

https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-clang
https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-clang/PKGBUILD

https://src.fedoraproject.org/rpms/llvm.git

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 23, 2023

@llvm/pr-subscribers-backend-x86

Author: 徐持恒 Xu Chiheng (xu-chiheng)

Changes

Fix the regression caused by commit ec92d74 2023-12-14, that, in Cygwin, Clang can't build binutils 2.42.
configure:4686: checking whether we are cross compiling
configure:4694: clang -o conftest.exe -march=x86-64 -O3 -Wl,--strip-all conftest.c >&5
/cygdrive/c/Users/ADMINI~1/AppData/Local/Temp/conftest-385c4a.o:conftest.c:(.text+0x10): relocation truncated to fit: IMAGE_REL_AMD64_ADDR32 against .rdata' /cygdrive/c/Users/ADMINI~1/AppData/Local/Temp/conftest-385c4a.o:conftest.c:(.text+0x15): relocation truncated to fit: IMAGE_REL_AMD64_ADDR32 against .rdata'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
configure:4698: $? = 1
configure:4705: ./conftest.exe
../binutils/configure: line 4707: ./conftest.exe: No such file or directory
configure:4709: $? = 127
configure:4716: error: in /cygdrive/e/Note/Tool/binutils-release-build': configure:4718: error: cannot run C compiled programs. If you meant to cross compile, use --host'.
See `config.log' for more details


Full diff: https://github.com/llvm/llvm-project/pull/76284.diff

1 Files Affected:

  • (modified) llvm/lib/Target/X86/X86ISelDAGToDAG.cpp (+7-10)
diff --git a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
index 7ec59c74f5f58c..c93ae875b3e4bb 100644
--- a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -2947,18 +2947,15 @@ bool X86DAGToDAGISel::selectMOV64Imm32(SDValue N, SDValue &Imm) {
     return false;
 
   Imm = N;
-  // Small/medium code model can reference non-TargetGlobalAddress objects with
-  // 32 bit constants.
-  if (N->getOpcode() != ISD::TargetGlobalAddress) {
-    return TM.getCodeModel() == CodeModel::Small ||
-           TM.getCodeModel() == CodeModel::Medium;
-  }
+  if (N->getOpcode() != ISD::TargetGlobalAddress)
+    return TM.getCodeModel() == CodeModel::Small;
 
-  const GlobalValue *GV = cast<GlobalAddressSDNode>(N)->getGlobal();
-  if (std::optional<ConstantRange> CR = GV->getAbsoluteSymbolRange())
-    return CR->getUnsignedMax().ult(1ull << 32);
+  std::optional<ConstantRange> CR =
+      cast<GlobalAddressSDNode>(N)->getGlobal()->getAbsoluteSymbolRange();
+  if (!CR)
+    return TM.getCodeModel() == CodeModel::Small;
 
-  return !TM.isLargeGlobalValue(GV);
+  return CR->getUnsignedMax().ult(1ull << 32);
 }
 
 bool X86DAGToDAGISel::selectLEA64_32Addr(SDValue N, SDValue &Base,

@aeubanks
Copy link
Contributor

do you have a test showing where this broke?

@xu-chiheng xu-chiheng force-pushed the cygwin-X86ISelDAGToDAG.cpp branch 2 times, most recently from e793d34 to ed11ebb Compare December 23, 2023 18:48
Copy link

github-actions bot commented Dec 23, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp Outdated Show resolved Hide resolved
@aeubanks
Copy link
Contributor

aeubanks commented Jan 4, 2024

can you explain what actually the problem is? without further context it looks like we're just blindly pushing workarounds

@aeubanks
Copy link
Contributor

aeubanks commented Jan 4, 2024

is the configure check is using the medium code model on cygwin? how large is the linked binary in the failing configure check? is it running into relocation overflows?

providing the configure source file being compiled would be helpful

@xu-chiheng
Copy link
Author

is the configure check is using the medium code model on cygwin? how large is the linked binary in the failing configure check? is it running into relocation overflows?

providing the configure source file being compiled would be helpful

Yes, Cygwin 64 bit must use medium code model. Otherwise, there are runtime errors.

@RKSimon
Copy link
Collaborator

RKSimon commented Jan 5, 2024

is the configure check is using the medium code model on cygwin? how large is the linked binary in the failing configure check? is it running into relocation overflows?
providing the configure source file being compiled would be helpful

Yes, Cygwin 64 bit must use medium code model. Otherwise, there are runtime errors.

In which case we shouldn't be allowing the other code models to be enabled in the first place for cygwin64 triples, not trying to fixup later on - I'm pretty sure we already do this restriction in other cases.

@xu-chiheng
Copy link
Author

is the configure check is using the medium code model on cygwin? how large is the linked binary in the failing configure check? is it running into relocation overflows?
providing the configure source file being compiled would be helpful

Yes, Cygwin 64 bit must use medium code model. Otherwise, there are runtime errors.

In which case we shouldn't be allowing the other code models to be enabled in the first place for cygwin64 triples, not trying to fixup later on - I'm pretty sure we already do this restriction in other cases.

The Cygwin 64 bit runtime, cygwin1.dll assume the module is compiled as cmodel medium, so it can load and execute programs.
This assumption is hard coded.
I'm not very familiar with source code of cygwin1.dll. But my guess is that, cygwin1.dll require specific pattern of symbol table, relocation table, the set of relocation entry kinds, so it can implement fork() using Win32 API.

@xu-chiheng
Copy link
Author

In which case we shouldn't be allowing the other code models to be enabled in the first place for cygwin64 triples, not trying to fixup later on - I'm pretty sure we already do this restriction in other cases.

There should be no restriction, because if a module's code does not call fork(), the cmodel can be not medium, that's my guess.
MinGW 64 bit default to cmodel small, but can be set to medium.

@xu-chiheng xu-chiheng closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants