function pointer address space handling (for AVR) is wrong

[RalfJung]

@eddyb points out that the handling of address space for function pointers (introduced by #73270) is fundamentally wrong: PointeeInfo can be used for optimization hints only and also only becomes relevant way too late during compilation. To properly account for function pointers having a different address space, they need to be added in the Pointer variant of the Primitive ABI type. That is the only way to fix types like Result<&i32, fn()>, which are currently going horribly wrong for Harvard architectures because the enum layout algorithm will think that &i32 and fn() are exactly the same fundamental type, and put them into one and the same field (resulting LLVM layout: { i64, ptr }).

Not sure whom to ping here, so I will pick some people that seem to be active in an around AVR: @dylanmckay @Patryk27.
Also Cc @rust-lang/compiler since this is related to some pretty fundamental compiler infrastructure

[Patryk27]

Ouch, I see - is it something that would be fixable by a person with, uhm, moderate knowledge on rustc (such as me) or rather you'd say it's something pretty big?

In any case, I can surely offer my help in testing out patches on AVR simulators and actual hardware.

[RalfJung]

I'm not entirely sure -- I have worked a bunch with Primitive and its surrounding types, but never tried to change them, so it is hard to say how big the fallout of that would be. @oli-obk will probably have a better idea.

[oli-obk]

It's going to be moderately annoying to keep rebasing, but nothing complex. Afaict there should be no issues like performance or accidental regressions/bugs coming out of this. As an MVP you could change https://doc.rust-lang.org/nightly/nightly-rustc/rustc_target/abi/enum.Primitive.html#variant.Pointer to have a PointerKind field that is ignored everywhere and only has a single variant for Memory (name bikesheddable). Once that lands, adding a Function variant should be trivial, then we just need to figure out how to use it in the llvm backend properly, which should be very self-contained.

[RalfJung]

That would however also affect all other targets, and e.g. Result<&u32, fn()> would not get the optimized representation any more on x86_64.

I think the Pointer variant should have an address space field, which will be 0 almost all the time except for function pointers on AVR. That matches what we do with size and alignment: all the target-specific information is explicit in Scalar.

[oli-obk]

I think the Pointer variant should have an address space field, which will be 0 almost all the time except for function pointers on AVR.

Hmm... I was thinking we'd make this decision in the backend, but you're right, the Primitive should contain all the relevant information and the burden of picking the right information for the target is on the producer of the Primitive, not the consumer

[eddyb]

to have a PointerKind field that is ignored everywhere and only has a single variant for Memory (name bikesheddable)

It's all memory, so that would be inappropriate (at least here) - the usual split is "data" vs "code".

However, I agree with @RalfJung - we already have a type for it, even:

rust/compiler/rustc_abi/src/lib.rs

Lines 1188 to 1202 in deba5dd

	/// An identifier that specifies the address space that some operation
	/// should operate on. Special address spaces have an effect on code generation,
	/// depending on the target and the address spaces it implements.
	#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
	pub struct AddressSpace(pub u32);
	impl AddressSpace {
	/// The default address space, corresponding to data space.
	pub const DATA: Self = AddressSpace(0);
	}
	/// Describes how values of the type are passed by target ABIs,
	/// in terms of categories of C types there are ABI rules for.
	#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)]
	#[cfg_attr(feature = "nightly", derive(HashStable_Generic))]

(though I think that can be changed from u32 to u8? not that it matters that much)

[erikdesjardins]

I'm working on this. It's natural to add AddressSpace to Primitive::Pointer.

Something that keeps coming up while doing this is code that looks "obviously wrong" because it assumes all pointers are the same size as Pointer(AddressSpace::DATA)...maybe I should fix that too. (Some targets actually do this in practice, e.g. UEFI, although it's not currently possible to create, e.g. ptr addrspace(272) [64 bit pointer on 32 bit UEFI target] via Rust code.)

though I think that can be changed from u32 to u8?

Technically LLVM allows up to 23 bits (why 23?) for the address space. We need at least u16 to parse 272 in UEFI datalayout strings; I'm not sure if any target goes beyond u16::MAX.