Skip to content

[llvm-ar] Use COFF archive format for COFF targets. #82642

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

Merged
merged 1 commit into from
Feb 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions llvm/include/llvm/Object/Archive.h
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ class Archive : public Binary {
Kind kind() const { return (Kind)Format; }
bool isThin() const { return IsThin; }
static object::Archive::Kind getDefaultKindForHost();
static object::Archive::Kind getDefaultKindForTriple(Triple &T);

child_iterator child_begin(Error &Err, bool SkipInternal = true) const;
child_iterator child_end() const;
Expand Down
15 changes: 11 additions & 4 deletions llvm/lib/Object/Archive.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -969,12 +969,19 @@ Archive::Archive(MemoryBufferRef Source, Error &Err)
Err = Error::success();
}

object::Archive::Kind Archive::getDefaultKindForTriple(Triple &T) {
if (T.isOSDarwin())
return object::Archive::K_DARWIN;
if (T.isOSAIX())
return object::Archive::K_AIXBIG;
if (T.isOSWindows())
return object::Archive::K_COFF;
Comment on lines +977 to +978
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little nervous about how this impacts the default archive format on a Windows host: our downstream uses a cross-compiler, typically hosted on Windows, but using GNU-style archives (and the objects therefore have a different triple too).

I'm going to suggest to our internal binutils team that they run some testing with this patch applied, to see if it makes any difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note the host matters only when llvm-ar can't infer the format from any other source. In practice, it means that it matters only when creating empty archives and archives who's first member is a non-symbolic files. In typical use case, when you pass object files, this PR shouldn't change anything unless you use COFF files.

I could limit impact of this PR by not changing the effect of getDefaultKindForHost, but it feels right to be consistent. If we consider those cases to be a problem, then it's also problematic for existing isOSDarwin() and isOSAIX() checks (which have even more significant format differences). If user cares about the format and uses llvm-ar on non-symbolic files, then an explicit --format argument is the only reliable way I can see to handle all cross compilation variants.

Copy link
Collaborator

@gbreynoo gbreynoo Feb 23, 2024

Choose a reason for hiding this comment

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

Thanks @jh7370, I checked and this will change our downstream ar's output in the cases @cjacek highlights above. However, due to @cjacek's points I cannot argue against it's submission.

The current behavior that default output format is defined by host does not seem good for the cross-compile use case in general. It may be preferable for the default to be based on the CMAKE option LLVM_DEFAULT_TARGET_TRIPLE or something similar to gnu binutils and the GNUTARGET environment variable. Both are likely out of the scope of this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LLVM_DEFAULT_TARGET_TRIPLE sounds like a good idea, I will create a separated PR for it, thanks.

return object::Archive::K_GNU;
}

object::Archive::Kind Archive::getDefaultKindForHost() {
Triple HostTriple(sys::getProcessTriple());
return HostTriple.isOSDarwin()
? object::Archive::K_DARWIN
: (HostTriple.isOSAIX() ? object::Archive::K_AIXBIG
: object::Archive::K_GNU);
return getDefaultKindForTriple(HostTriple);
}

Archive::child_iterator Archive::child_begin(Error &Err,
Expand Down
21 changes: 11 additions & 10 deletions llvm/lib/Object/ArchiveWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,16 @@ object::Archive::Kind NewArchiveMember::detectKindFromObject() const {
Expected<std::unique_ptr<object::ObjectFile>> OptionalObject =
object::ObjectFile::createObjectFile(MemBufferRef);

if (OptionalObject)
return isa<object::MachOObjectFile>(**OptionalObject)
? object::Archive::K_DARWIN
: (isa<object::XCOFFObjectFile>(**OptionalObject)
? object::Archive::K_AIXBIG
: object::Archive::K_GNU);
if (OptionalObject) {
if (isa<object::MachOObjectFile>(**OptionalObject))
return object::Archive::K_DARWIN;
if (isa<object::XCOFFObjectFile>(**OptionalObject))
return object::Archive::K_AIXBIG;
if (isa<object::COFFObjectFile>(**OptionalObject) ||
isa<object::COFFImportFile>(**OptionalObject))
return object::Archive::K_COFF;
return object::Archive::K_GNU;
}

// Squelch the error in case we had a non-object file.
consumeError(OptionalObject.takeError());
Expand All @@ -80,10 +84,7 @@ object::Archive::Kind NewArchiveMember::detectKindFromObject() const {
MemBufferRef, file_magic::bitcode, &Context)) {
auto &IRObject = cast<object::IRObjectFile>(**ObjOrErr);
auto TargetTriple = Triple(IRObject.getTargetTriple());
return TargetTriple.isOSDarwin()
? object::Archive::K_DARWIN
: (TargetTriple.isOSAIX() ? object::Archive::K_AIXBIG
: object::Archive::K_GNU);
return object::Archive::getDefaultKindForTriple(TargetTriple);
} else {
// Squelch the error in case this was not a SymbolicFile.
consumeError(ObjOrErr.takeError());
Expand Down
85 changes: 85 additions & 0 deletions llvm/test/tools/llvm-ar/coff-symtab.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
Verify that llvm-ar uses COFF archive format by ensuring that archive map is sorted.

RUN: rm -rf %t.dif && split-file %s %t.dir && cd %t.dir

RUN: yaml2obj coff-symtab.yaml -o coff-symtab.obj
RUN: llvm-ar crs out.a coff-symtab.obj
RUN: llvm-nm --print-armap out.a | FileCheck %s

RUN: llvm-as coff-symtab.ll -o coff-symtab.bc
RUN: llvm-ar crs out2.a coff-symtab.bc
RUN: llvm-nm --print-armap out2.a | FileCheck %s

RUN: yaml2obj elf.yaml -o coff-symtab.o
RUN: llvm-ar crs --format coff out3.a coff-symtab.o
RUN: llvm-nm --print-armap out3.a | FileCheck %s

CHECK: Archive map
CHECK-NEXT: a in coff-symtab
CHECK-NEXT: b in coff-symtab
CHECK-NEXT: c in coff-symtab
CHECK-EMPTY:

#--- coff-symtab.yaml
--- !COFF
header:
Machine: IMAGE_FILE_MACHINE_UNKNOWN
Characteristics: [ ]
sections:
- Name: .text
Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
Alignment: 4
SectionData: ''
symbols:
- Name: b
Value: 0
SectionNumber: 1
SimpleType: IMAGE_SYM_TYPE_NULL
ComplexType: IMAGE_SYM_DTYPE_FUNCTION
StorageClass: IMAGE_SYM_CLASS_EXTERNAL
- Name: c
Value: 0
SectionNumber: 1
SimpleType: IMAGE_SYM_TYPE_NULL
ComplexType: IMAGE_SYM_DTYPE_FUNCTION
StorageClass: IMAGE_SYM_CLASS_EXTERNAL
- Name: a
Value: 0
SectionNumber: 1
SimpleType: IMAGE_SYM_TYPE_NULL
ComplexType: IMAGE_SYM_DTYPE_FUNCTION
StorageClass: IMAGE_SYM_CLASS_EXTERNAL
...


#--- coff-symtab.ll
target triple = "x86_64-unknown-windows-msvc"

define void @b() { ret void }
define void @c() { ret void }
define void @a() { ret void }

#--- elf.yaml
--- !ELF
FileHeader:
Class: ELFCLASS64
Data : ELFDATA2LSB
Type: ET_REL
Machine: EM_X86_64
Sections:
- Name: .text
Type: SHT_PROGBITS
Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
AddressAlign: 0x0000000000000004
Content: ''
Symbols:
- Name: b
Binding: STB_GLOBAL
Section: .text
- Name: c
Binding: STB_GLOBAL
Section: .text
- Name: a
Binding: STB_GLOBAL
Section: .text
...
7 changes: 6 additions & 1 deletion llvm/tools/llvm-ar/llvm-ar.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ static void printArHelp(StringRef ToolName) {
=darwin - darwin
=bsd - bsd
=bigarchive - big archive (AIX OS)
=coff - coff
--plugin=<string> - ignored for compatibility
-h --help - display this help and exit
--output - the directory to extract archive members to
Expand Down Expand Up @@ -193,7 +194,7 @@ static SmallVector<const char *, 256> PositionalArgs;
static bool MRI;

namespace {
enum Format { Default, GNU, BSD, DARWIN, BIGARCHIVE, Unknown };
enum Format { Default, GNU, COFF, BSD, DARWIN, BIGARCHIVE, Unknown };
}

static Format FormatType = Default;
Expand Down Expand Up @@ -1044,6 +1045,9 @@ static void performWriteOperation(ArchiveOperation Operation,
case GNU:
Kind = object::Archive::K_GNU;
break;
case COFF:
Kind = object::Archive::K_COFF;
break;
case BSD:
if (Thin)
fail("only the gnu format has a thin mode");
Expand Down Expand Up @@ -1376,6 +1380,7 @@ static int ar_main(int argc, char **argv) {
.Case("darwin", DARWIN)
.Case("bsd", BSD)
.Case("bigarchive", BIGARCHIVE)
.Case("coff", COFF)
.Default(Unknown);
if (FormatType == Unknown)
fail(std::string("Invalid format ") + Match);
Expand Down