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

[LLD][COFF] Add ARM64EC support to findLineTable. #87240

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Apr 1, 2024

Fixes asserts in error messages. Use chunk machine type (instead of global one) to support x86_64 objects and treat ARM64EC objects like ARM64 ones.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 1, 2024

@llvm/pr-subscribers-lld
@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-lld-coff

Author: Jacek Caban (cjacek)

Changes

Fixes asserts in error messages. Use chunk machine type (instead of global one) to support x86_64 objects and treat ARM64EC objects like ARM64 ones.


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

2 Files Affected:

  • (modified) lld/COFF/PDB.cpp (+3-1)
  • (modified) lld/test/COFF/undefined-symbol-cv.s (+56-3)
diff --git a/lld/COFF/PDB.cpp b/lld/COFF/PDB.cpp
index 35e4c68dcda6c8..7bdcf01a6fc31d 100644
--- a/lld/COFF/PDB.cpp
+++ b/lld/COFF/PDB.cpp
@@ -1735,6 +1735,8 @@ static uint32_t getSecrelReloc(llvm::COFF::MachineTypes machine) {
   case ARMNT:
     return COFF::IMAGE_REL_ARM_SECREL;
   case ARM64:
+  case ARM64EC:
+  case ARM64X:
     return COFF::IMAGE_REL_ARM64_SECREL;
   default:
     llvm_unreachable("unknown machine type");
@@ -1752,7 +1754,7 @@ static bool findLineTable(const SectionChunk *c, uint32_t addr,
                           DebugLinesSubsectionRef &lines,
                           uint32_t &offsetInLinetable) {
   ExitOnError exitOnErr;
-  const uint32_t secrelReloc = getSecrelReloc(c->file->ctx.config.machine);
+  const uint32_t secrelReloc = getSecrelReloc(c->getMachine());
 
   for (SectionChunk *dbgC : c->file->getDebugChunks()) {
     if (dbgC->getSectionName() != ".debug$S")
diff --git a/lld/test/COFF/undefined-symbol-cv.s b/lld/test/COFF/undefined-symbol-cv.s
index 08a85826b9f968..502f28044c17f0 100644
--- a/lld/test/COFF/undefined-symbol-cv.s
+++ b/lld/test/COFF/undefined-symbol-cv.s
@@ -1,6 +1,14 @@
-# REQUIRES: x86
-# RUN: llvm-mc -triple=x86_64-windows-msvc -filetype=obj -o %t.obj %s
-# RUN: not lld-link /out:%t.exe %t.obj 2>&1 | FileCheck %s
+# REQUIRES: aarch64, x86
+# RUN: split-file %s %t.dir && cd %t.dir
+
+# RUN: llvm-mc -triple=x86_64-windows-msvc -filetype=obj -o test-x86_64.obj test-x86_64.s
+# RUN: llvm-mc -triple=aarch64-windows-msvc -filetype=obj -o test-aarch64.obj test-aarch64.s
+# RUN: llvm-mc -triple=arm64ec-windows-msvc -filetype=obj -o test-arm64ec.obj test-aarch64.s
+
+# RUN: not lld-link -out:test-x86_64.exe test-x86_64.obj 2>&1 | FileCheck %s
+# RUN: not lld-link -out:test-aarch64.exe test-aarch64.obj 2>&1 | FileCheck %s
+# RUN: not lld-link -out:test-arm64ec.exe -machine:arm64ec test-arm64ec.obj 2>&1 | FileCheck %s
+# RUN: not lld-link -out:test-arm64ec2.exe -machine:arm64ec test-x86_64.obj 2>&1 | FileCheck %s
 
 # CHECK: error: undefined symbol: int __cdecl foo(void)
 # CHECK-NEXT: >>> referenced by file1.cpp:1
@@ -18,6 +26,7 @@
 # CHECK-NEXT: >>> referenced by file1.cpp:5
 # CHECK-NEXT: >>>               {{.*}}.obj:(f2)
 
+#--- test-x86_64.s
 	.cv_file	1 "file1.cpp" "EDA15C78BB573E49E685D8549286F33C" 1
 	.cv_file	2 "file2.cpp" "EDA15C78BB573E49E685D8549286F33D" 1
 
@@ -60,3 +69,47 @@ f2:
 	.long	4
 	.cv_filechecksums
 	.cv_stringtable
+
+#--- test-aarch64.s
+	.cv_file	1 "file1.cpp" "EDA15C78BB573E49E685D8549286F33C" 1
+	.cv_file	2 "file2.cpp" "EDA15C78BB573E49E685D8549286F33D" 1
+
+        .section        .text,"xr",one_only,main
+.globl main
+main:
+	.cv_func_id 0
+	.cv_loc	0 1 1 0 is_stmt 0
+	bl	"?foo@@YAHXZ"
+	.cv_loc	0 1 2 0
+	bl	"?foo@@YAHXZ"
+	.cv_loc	0 2 3 0
+	b	"?bar@@YAHXZ"
+.Lfunc_end0:
+
+f1:
+	.cv_func_id 1
+	.cv_loc	1 1 4 0 is_stmt 0
+	bl	"?bar@@YAHXZ"
+.Lfunc_end1:
+
+        .section        .text,"xr",one_only,f2
+.globl f2
+f2:
+	.cv_func_id 2
+	.cv_loc	2 1 5 0 is_stmt 0
+	bl	"?baz@@YAHXZ"
+.Lfunc_end2:
+
+	.section	.debug$S,"dr",associative,main
+	.long	4
+	.cv_linetable	0, main, .Lfunc_end0
+	.cv_linetable	1, f1, .Lfunc_end1
+
+	.section	.debug$S,"dr",associative,f2
+	.long	4
+	.cv_linetable	2, f2, .Lfunc_end2
+
+	.section	.debug$S,"dr"
+	.long	4
+	.cv_filechecksums
+	.cv_stringtable

lld/COFF/PDB.cpp Outdated
@@ -1735,6 +1735,8 @@ static uint32_t getSecrelReloc(llvm::COFF::MachineTypes machine) {
case ARMNT:
return COFF::IMAGE_REL_ARM_SECREL;
case ARM64:
case ARM64EC:
case ARM64X:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it really make sense to handle "ARM64X" here? Even if you're linking for "arm64x", a given section should be either Arm64 (and needs arm64 relocs) or x64 (and needs x64 relocs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An object file can be ARM64X too, see https://reviews.llvm.org/D149087 for the context of allowing them:

We do expect ARM64X on input as well. Individual input object file with its machine value of ARM64X are rare and weird, but they do exist and llvm-cvtres can produce them with https://reviews.llvm.org/D148517 (...) According to my testing, ARM64X objects are accepted by link.exe for all targets: ARM64, ARM64EC and ARM64X.

Output of llvm-cvtres is not going to cause references errors, so it's indeed not very interesting in this case (it would require somehow generating such file), but adding it here for completeness seems simple enough to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking some more about it, this is a common pattern. We could reduce the places that need to explicitly care about about ARM64EC/ARM64X with a helper like getMachineArchType() and using Triple::ArchType type for the switch. I will prepare a separate PR to discuss it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created #87370 with mentioned clean up. On top of that, this PR could look like this: cjacek@e8e0b1c

@cjacek
Copy link
Contributor Author

cjacek commented Apr 4, 2024

I pushed a new version which uses getArch helper introduced by #87495.

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

LGTM

@cjacek cjacek merged commit c0211ff into llvm:main Apr 5, 2024
4 checks passed
@cjacek cjacek deleted the lld-line-table branch April 5, 2024 11:14
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

3 participants