Add binary_format to rustc target specs

[Pyr0de]

Added binary format field to TargetOptions

Fixes #135724

r? @Noratrieb

[Pyr0de]

@rustbot label -O-apple +A-target-specs

[bors]

☔ The latest upstream changes (presumably #136684) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

[Noratrieb]

Thanks, this is great. It looks good, but I'll take a closer look to ensure that every single target is still correct after you address my feedback below. Then we can merge it.

[Pyr0de]

There were 2 places that used this adhoc logic for the binary format

This one, I have changed to use the binary format field from Target

This one requires the object::read::BinaryFormat to pass to another function

rust/compiler/rustc_codegen_ssa/src/back/metadata.rs

Lines 255 to 265 in 905b1bf

	let binary_format = if sess.target.is_like_osx {
	BinaryFormat::MachO
	} else if sess.target.is_like_windows {
	BinaryFormat::Coff
	} else if sess.target.is_like_aix {
	BinaryFormat::Xcoff
	} else {
	BinaryFormat::Elf
	};
	let mut file = write::Object::new(binary_format, architecture, endianness);

[Pyr0de]

Added Xcoff here with Elf since it wasn't taken into account before and AsmBinaryFormat defaulted to elf

[Noratrieb]

Hm, I'd expect this to be wrong, but not any more wrong than before, and I don't really know anything about XCoff. Let's leave it like this and open a follow-up issue about this and ping the AIX maintainers there.

[Pyr0de]

I have created an issue about this at #137219

[folkertdev]

author of this file here: I just checked, and this is definitely wrong and needs something custom. I don't think we have any tests that would hit this target, so I think this PR can just be merged first, and then we fix naked_asm for AIX later.

I'll have a stab at a fix, and then hopefully the AIX folks can confirm that what I come up with actually works.

also in general: this is a really nice change, thanks!

[bors]

☔ The latest upstream changes (presumably #137046) made this pull request unmergeable. Please resolve the merge conflicts.

[Noratrieb]

For the other use you mentioned, I would add a method to BinaryFormat that returns the object::BinaryFormat and use that there.

I also ran the following script to compare your results against the previous logic:

#!/usr/bin/env bash
for target in $(rustc +dev3 --print target-list); do
	json=$(rustc +dev3 --print target-spec-json --target "$target" -Zunstable-options 2>/dev/null)
	fmt=$(echo "$json"| jq '."binary-format"' -r)
	
	is_like_windows=$(echo "$json"| jq '."is-like-windows"')
	is_like_osx=$(echo "$json"| jq '."is-like-osx"')
	is_like_wasm=$(echo "$json"| jq '."is-like-wasm"')

	if [ "$is_like_windows" = "true" ]; then
		if [ "$fmt" != "coff" ]; then
			echo "ERROR: $fmt $target"
		fi
	elif [ "$is_like_osx" = "true" ]; then
		if [ "$fmt" != "mach-o" ]; then
			echo "ERROR: $fmt $target"
		fi
	elif [ "$is_like_wasm" = "true" ]; then
		if [ "$fmt" != "wasm" ]; then
			echo "ERROR: $fmt $target"
		fi
	else
		if [ "$fmt" != "null" ]; then
			echo "ERROR: $fmt $target"
		fi
	fi
done

It found two results:

ERROR: xcoff powerpc64-ibm-aix
ERROR: null x86_64-pc-cygwin

XCoff for AIX is good, but ELF for cygwin is wrong. Cygwin is Windows and uses Coff, you need to adjust that as well.

[Noratrieb]

Hm, I'd expect this to be wrong, but not any more wrong than before, and I don't really know anything about XCoff. Let's leave it like this and open a follow-up issue about this and ping the AIX maintainers there.

[Noratrieb]

Thanks!
@bors r+

[bors]

📌 Commit 17f2928 has been approved by Noratrieb

It is now in the queue for this repository.