"extern type" should use opaque type in LLVM

[RalfJung]

Currently, in a situation like

extern {
    type Foo;
    static mut BAR: Foo;
}

we generate a zero-sized struct as the LLVM type for Foo. LLVM might use this to assume that BAR has size 0, which would be fatal. And anyway it seems prudent for extern types, that should match C's declared-but-not-defined types, to match what Clang does for the corresponding C types -- which is to use opaque.

See #58271 for an attempt to fix this, which ran into LLVM assertion failures.

Cc @eddyb

[eddyb]

Maybe opaque is different than what I was doing, which was to leave the body of the LLVM struct type unfilled? cc @nagisa @rkruppe

[hanna-kruppe]

If you use those words precisely in the sense LLVM documentation does, then that should do the trick. The easiest way to be sure would be to look at --emit llvm-ir output and see whether it says opaque or {}.

[nagisa]

From the assertion it appears that something somewhere is doing a GEP(i) on a pointer to an opaque type (a different thing from a {}). Since the size of an opaque element is not known, obviously GEP(i) cannot work. {} will have a size and GEP(i)s will work, although will not necessarily produce an expected result.

[eddyb]

I suspect it's caused by compiling code working with this:

rust/src/librustc/ty/mod.rs

Lines 591 to 607 in aa97448

	extern {
	/// A dummy type used to force List to by unsized without requiring fat pointers
	type OpaqueListContents;
	}
	/// A wrapper for slices with the additional invariant
	/// that the slice is interned and no other slice with
	/// the same contents can exist in the same context.
	/// This means we can use pointer for both
	/// equality comparisons and hashing.
	/// Note: `Slice` was already taken by the `Ty`.
	#[repr(C)]
	pub struct List<T> {
	len: usize,
	data: [T; 0],
	opaque: OpaqueListContents,
	}

However, that would just be just "tail-opaque" and its overall size shouldn't matter.

[neocturne]

As @eddyb suspected, the List<T> code is what triggers the issue in the rustc build. I've reduced the code to the following minimal reproducers:

#![feature(extern_types)]
#![allow(dead_code)]

extern "C" {
    pub type Opaque;
}

pub struct TailOpaque {
    a: usize,
    opaque: Opaque,
}

pub fn foo(v: &TailOpaque) -> &Opaque {
    &v.opaque
}

#![feature(extern_types)]
#![allow(dead_code)]

extern "C" {
    pub type Opaque;
}

pub struct TailOpaque {
    a: usize,
    b: usize,
    opaque: Opaque,
}

pub fn foo(v: &TailOpaque) -> usize {
    v.b
}

It seems that accessing or referencing any field of a tail-opaque struct past the first element triggers the assertion failure.

[neocturne]

Okay, digging further into this I have come to a few conclusions:

The rustc issue leading to the assertion failure is our attempt to generate a getelementptr instruction, which is invalid for unsized types. By commenting out a few sanity checks, I could get LLVM to emit IR with such an instruction, but it would then fail at a later step.

At this point, I was wondering why the issue only affects "extern type" (with PR #58271), and not all unsized types, for example a struct with a str last element. The solution is that regular unsized structs are emitted with a length-0 array, so they are sized from LLVM's point of view: A Rust struct Foo(usize, str) becomes %Foo = type { [0 x i64], i64, [0 x i8], [0 x i8] } in LLVM IR.

That leaves me with the question if there is a fundamental difference between "extern type" and existing unsized types which would require us to use opaque for one, but not for the other?

[neocturne]

One more data point, which may be relevant for unsized statics: clang (tested with 10.0.0) emits externals of unknown size as "size 0" as well:

extern const char foo[];

turns into

@foo = external global [0 x i8], align 1

My interpretation of this is that using an empty struct and not opaque in LLVM should be fine (unless LLVM somehow treats empty arrays and empty structs differently, in which case switching to an empty array for Rust external types would be the way to go?)

Unless someone has a different interpretation, I propose we go with one of the following options, so we can move to stabilize external types:

1. Keep the struct {} that is currently implemented and close this issue
2. Replace type emitted to LLVM with [0 x i8] (I can take case of the PR) and then close this issue

[RalfJung]

So the argument seems to be: clang doesn't use that pattern for opaque types, but it uses it for something, and thus the pattern is probably legal?

That seems reasonable. Before we rely on this, could someone file an LLVM bug asking them to state this explicitly in their LangRef (that extern globals may be larger than the type that is given, and that it is okay to access that larger part with getelementptr inbounds).

[neocturne]

@RalfJung I don't think we ever use getelementptr inbounds to access the opaque part of an external or tail-external type, because it's, well, opaque. The worst one can do without raw pointer arithmetic or otherwise unsafe code would be to access the first address of the opaque part - which is always valid because of the usual one-byte-past-the-end rule for pointer validity.

I would actually have expected that regular slices are a bigger problem, as getelementptr inbounds will be used for accesses into the "tail" of an unsized type, for example:

pub struct Foo(usize, str);
pub fn foo(v: &Foo) -> &u8 {
    unsafe { &v.1.as_bytes().get_unchecked(1) }
}

But there is no problem in this case either, as we emit slices as length 0 arrays - and the handling of this case is explained in detail here: https://llvm.org/docs/GetElementPtr.html#what-happens-if-an-array-index-is-out-of-bounds

[RalfJung]

The issue is with cases like

extern {
    type Foo;
    static mut BAR: Foo;
}

At least after cross-crate inlining (LTO, maybe even cross-language LTO), there can well be actual accesses to BAR happening, I think. After all opaque is not an intrinsic property of that static, it just expresses our lack of knowledge about it. Some code somewhere will know the actual dimensions of it (everything else would be rather pointless) and access that data, and with sufficient inlining that code can end up in our crate that only sees an opaque type.

[neocturne]

Okay, in that case we should switch over to [0 x i8] just to be safe - I think the explanation in https://llvm.org/docs/GetElementPtr.html#what-happens-if-an-array-index-is-out-of-bounds makes it quite clear that accessing parts of an array past the end of a type is fine - that inbounds is about the actual size of the allocation, not about the size of the LLVM type.

[hanna-kruppe]

FWIW, I don't think that page really addresses the concern. It tells us that static type information of the pointer or GEP is irrelevant and the actual allocation size matters, but the type of global variables in fact determines the actual allocation size. Certainly with static X: [u8; 4], a GEP calculating (&X[0]).offset(5) is going out of bounds. Maybe for extern globals the type isn't assumed to be authoritative, but that doesn't follow from those docs you linked.

[neocturne]

I see. I can make a request for clarification over at the LLVM bug tracker as soon as my account is approved.

[neocturne]

https://bugs.llvm.org/show_bug.cgi?id=45681

[mjbshaw]

Can https://bugs.llvm.org/show_bug.cgi?id=45681 be closed now that https://reviews.llvm.org/D78952 was merged? And are there any other clarifications required before Rust can use opaque for extern types?

[neocturne]

@mjbshaw I think the LLVM bug can be closed. As I understand it, the result of the merge is the opposite though for Rust: With the clarified LLVM spec, there should be no reason for Rust to use opaque for extern types - meaning the issues we had with our attempts to use opaque don't need to block stabilization of extern types anymore, and this issue can be closed.

(@RalfJung Please correct me if any of this is inaccurate)

[RalfJung]

Yeah that sounds right, the LLVM patch seems pretty clear about this. Thanks for pushing this through on the LLVM side!