Fix tests using only-i686 to use the correct only-x86 directive

[wesleywiser]

We translate i686 to x86 which means tests marked as only-i686
never ran. Update those tests to use only-x86.

We parse the only- architecture directive here

rust/src/tools/compiletest/src/util.rs

Lines 160 to 168 in 27143a9

	pub fn get_arch(triple: &str) -> &'static str {
	let triple: Vec<_> = triple.split('-').collect();
	for &(triple_arch, arch) in ARCH_TABLE {
	if triple.contains(&triple_arch) {
	return arch;
	}
	}
	panic!("Cannot determine Architecture from triple");
	}

and we translate i686 to x86 here

rust/src/tools/compiletest/src/util.rs

Line 56 in 27143a9

("i686", "x86"),

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

Hm, how do we ensure new tests don't get added? I would have expected the panic! in that code to get triggered... maybe we can fix that as well?

[wesleywiser]

The panic isn't triggered because we do find the arch but in the calling code, it doesn't match the only- directive:

rust/src/tools/compiletest/src/header.rs

Line 689 in 27143a9

name == util::get_arch(&self.target) || // architecture

ie:

• we call util::get_arch passing in "i686-pc-windows-msvc"
• we find i686 in ARCH_TABLE and so we return "x86"
• but name is i686 so we don't match

[Mark-Simulacrum]

Right, I guess, what I'm trying to get at is that it seems like we should error on such a test that can never run. I'm not sure I'm fully understanding, but perhaps we should add a panic whenever name is present on the "left hand" of the mapping table?

Alternatively, we could also call the equivalent of get_arch on the name that maps it, but I'd prefer we don't have both only-i686 and only-x86 tests.

[wesleywiser]

Right. I agree, ideally we would be able to detect an invalid only- flag which will cause the test to never run but compiletest isn't really set up to do that right now and it would be a much larger change than just fixing this. I'm also a little hesitant to introduce validation just for architectures as I think developers would naturally expect the other kinds of flags to be validated as well.

We could certainly file an issue to improve compiletest though.

I'm not sure I'm fully understanding, but perhaps we should add a panic whenever name is present on the "left hand" of the mapping table?

We also can't do that because some names are present as both keys and values in the mapping table (arm, aarch64, avx, etc).

[Mark-Simulacrum]

Alright. r=me with an issue filed.

Did you spot check the other mappings in the table to check we don't have extra tests like this (for non-i686)?

[wesleywiser]

Yes, I didn't see any other issues.

@bors r=Mark-Simulacrum rollup

[bors]

📌 Commit 8c56ef0 has been approved by Mark-Simulacrum