Re-introduce load-bearing assert, or fix niche debuginfo generation properly.

[eddyb]

See #55701 (comment), this is just one-line addition for the assert.

Currently, this example:

#![feature(repr128)]

#[repr(u128)]
pub enum Foo {
    Lo,
    Hi = 1 << 64,
    Bar = 18_446_745_000_000_000_123,
}

pub fn foo() -> Option<Foo> {
    None
}

has this in its LLVM IR (in debug mode):

!22 = !DIDerivedType(tag: DW_TAG_member, name: "None", scope: !20, file: !6, baseType: !23, size: 128, align: 64, extraData: i64 926290448508)

Note the extraData, which indicates the discriminant (niche, in this case), it should be the same value as Foo::Bar, plus 1, but is instead the truncated value (see below).

The debuginfo for Foo is also wrong, but that is caused elsewhere in the codebase (in a spot where no assertion was ever added AFAIK. also see #59509 (comment)):

!10 = !DIEnumerator(name: "Lo", value: 0)
!11 = !DIEnumerator(name: "Hi", value: 0)
!12 = !DIEnumerator(name: "Bar", value: 926290448507)

The compiler should ICE, instead, to avoid generating the wrong debuginfo.

---

Alternatively, if LLVM and DWARF support it, we should encode the 128-bit discriminant, by employing const_uint_big instead of const_u64 here:

rust/src/librustc_codegen_llvm/debuginfo/metadata.rs

Line 1817 in 237bf32

Some(value) => Some(cx.const_u64(value)),

Also, this field would need to become Option<u128>:

rust/src/librustc_codegen_llvm/debuginfo/metadata.rs

Line 924 in 237bf32

discriminant: Option<u64>,

[cuviper]

Alternatively, if LLVM and DWARF support it

I think DWARF LEB128 isn't technically bounded, but I'll bet a lot of tools will only decode 64-bit values. There's also DW_FORM_data16 though.

But it looks like LLVM is limited:

rust/src/librustc_codegen_llvm/debuginfo/metadata.rs

Lines 1541 to 1545 in 2002b4b

	Some(llvm::LLVMRustDIBuilderCreateEnumerator(
	DIB(cx),
	name.as_ptr(),
	// FIXME: what if enumeration has i128 discriminant?
	discr.val as u64))

rust/src/rustllvm/RustWrapper.cpp

Lines 866 to 870 in 2002b4b

	extern "C" LLVMMetadataRef
	LLVMRustDIBuilderCreateEnumerator(LLVMRustDIBuilderRef Builder,
	const char *Name, uint64_t Val) {
	return wrap(Builder->createEnumerator(Name, Val));
	}

https://github.com/rust-lang/llvm-project/blob/1f484cbe0e863e9e215f1b3d7198063444d60873/llvm/include/llvm/IR/DIBuilder.h#L180-L181

We probably should at least be setting that isUnsigned parameter where possible. It was added in llvm-project commit 08dc66e, D42734, since LLVM 7.

[eddyb]

Okay I was a bit confused, turns out there are two separate parts to this:

• Foo's enumeraror debuginfo, which has nothing to do with the assert that was removed, and is missing its own assert, until the problem described by @cuviper is resolved
• Option<Foo>'s variant member type debuginfo for None, is the one affected by the removed assert
  • the function creating that debuginfo metadata (LLVMRustDIBuilderCreateVariantMemberType) takes an arbitrary LLVM value, so i128 might work here
  • this is the part which rustc_codegen_llvm: support 128-bit discriminants in debuginfo. #59520 fixes, which we can try to merge now

EDIT: updated the issue description, including the part of the IR showing the issue.

[cuviper]

The compiler should ICE, instead, to avoid generating the wrong debuginfo.

Wouldn't it be better to just omit debuginfo when it can't be represented?

[eddyb]

Maybe, with a warning.

[wesleywiser]

Visited during wg-debugging triage. This assert exists today:

rust/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs

Lines 416 to 419 in 8006510

	// NOTE(eddyb) do *NOT* remove this assert, until
	// we pass the full 128-bit value to LLVM, otherwise
	// truncation will be silent and remain undetected.
	assert_eq!(value as u64 as u128, value);

so I'm going to close this issue as completed and open a new issue specifically for tracking the removal of the assert.