debuginfo: Don't emit DW_AT_address_class attribute for pointer type debuginfo.

[michaelwoerister]

Currently the compiler adds the DW_AT_address_class attribute to pointer type debuginfo. This seems to be accidental, mostly due to our LLVM bindings not allowing to omit the attribute. This PR changes this and brings the actual behavior in line with the intended behavior (as expressed in the pre-existing comments in metadata.rs).

For reference, Clang does not emit the attribute.

Only the final commit is relevant for this PR. The other two commits are from #93006, which should be merged first.

[rust-highfive]

r? @jackh726

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

[michaelwoerister]

r? @cuviper, since this involves our LLVM bindings.

[bors]

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

[michaelwoerister]

This is unblocked now.

[cuviper]

So, you're always emitting None, but we do have the concept of AddressSpace available on the Rust side. Are there places where we could or should be propagating that to debuginfo? If so, how should we decide when to emit this or not -- e.g. maybe when instruction_address_space != AddressSpace::DATA?

The change itself seems fine, but if we're never going to set a real value, we might as well hardcode that None in the FFI call.

For reference, Clang does not emit the attribute.

I see a few places in Clang that depend on the result of TargetInfo::getDWARFAddressSpace, which does default to None, but AMDGPU, NVPTX, and SPIR all have conditions that may return some value.

[michaelwoerister]

So, you're always emitting None, but we do have the concept of AddressSpace available on the Rust side. Are there places where we could or should be propagating that to debuginfo? If so, how should we decide when to emit this or not -- e.g. maybe when instruction_address_space != AddressSpace::DATA?

Is there a description of what these address spaces are exactly? Are these the same as LayoutLlvmExt::pointee_info_at() returns?

[cuviper]

Is there a description of what these address spaces are exactly?

LLVM describes it in the Data Layout:

P<addressspace>

Specifies the address space that corresponds to program memory. Harvard architectures can use this to specify what space LLVM should place things such as functions into. If omitted, the program memory space defaults to the default address space of 0, which corresponds to a Von Neumann architecture that has code and data in the same space.

G<addressspace>

Specifies the address space to be used by default when creating global variables. If omitted, the globals address space defaults to the default address space 0. Note: variable declarations without an address space are always created in address space 0, this property only affects the default value to be used when creating globals without additional contextual information (e.g. in LLVM passes).

A<addressspace>

Specifies the address space of objects created by ‘alloca’. Defaults to the default address space of 0.

The only standardized value in DWARF is DW_ADDR_none = 0, so maybe we could just key that as our ignored value that we don't bother emitting at all, as it's also LLVM's default and our AddressSpace::DATA.

The DWARF spec mentions other i386 examples like DW_ADDR_near32 and DW_ADDR_far32, but FWIW I don't see those in my local dwarf.h. In LLVM source, I do see extra DW_ADDR_* definitions specific to AMDGPU.

Are these the same as LayoutLlvmExt::pointee_info_at() returns?

I think it is PointeeInfo's field address_space, yes.

[michaelwoerister]

Thanks for digging up that information! I'll update the PR to use the value from PointeeInfo and make the compiler omit the attribute if it is zero.

[michaelwoerister]

FYI: I plan to do this but it might take a while because it's not high priority. I'm leaving the PR open, so I don't forget.

[bors]

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

[bors]

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

[JohnCSimon]

@michaelwoerister
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive