Skip to content

Pass 128-bit C-style enum enumerator values to LLVM #102717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 21, 2022
Merged

Pass 128-bit C-style enum enumerator values to LLVM #102717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25c1c63
Pass 128-bit C-style enum enumerator values to LLVM
beetrees Oct 1, 2022
6f6eebd
Add a test for C-style repr128 enum DWARF debuginfo
beetrees Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/cpp_like.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ fn build_variant_names_type_di_node<'ll, 'tcx>(
cx,
"VariantNames",
variant_names_enum_base_type(cx),
variants.map(|(variant_index, variant_name)| (variant_name, variant_index.as_u32() as u64)),
variants.map(|(variant_index, variant_name)| (variant_name, variant_index.as_u32().into())),
containing_scope,
)
}
Expand Down
16 changes: 6 additions & 10 deletions compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,7 @@ fn build_c_style_enum_di_node<'ll, 'tcx>(
tag_base_type(cx, enum_type_and_layout),
enum_adt_def.discriminants(cx.tcx).map(|(variant_index, discr)| {
let name = Cow::from(enum_adt_def.variant(variant_index).name.as_str());
// Is there anything we can do to support 128-bit C-Style enums?
let value = discr.val as u64;
(name, value)
(name, discr.val)
}),
containing_scope,
),
Expand Down Expand Up @@ -147,36 +145,34 @@ fn tag_base_type<'ll, 'tcx>(
/// This is a helper function and does not register anything in the type map by itself.
///
/// `variants` is an iterator of (discr-value, variant-name).
///
// NOTE: Handling of discriminant values is somewhat inconsistent. They can appear as u128,
// u64, and i64. Here everything gets mapped to i64 because that's what LLVM's API expects.
fn build_enumeration_type_di_node<'ll, 'tcx>(
cx: &CodegenCx<'ll, 'tcx>,
type_name: &str,
base_type: Ty<'tcx>,
enumerators: impl Iterator<Item = (Cow<'tcx, str>, u64)>,
enumerators: impl Iterator<Item = (Cow<'tcx, str>, u128)>,
containing_scope: &'ll DIType,
) -> &'ll DIType {
let is_unsigned = match base_type.kind() {
ty::Int(_) => false,
ty::Uint(_) => true,
_ => bug!("build_enumeration_type_di_node() called with non-integer tag type."),
};
let (size, align) = cx.size_and_align_of(base_type);

let enumerator_di_nodes: SmallVec<Option<&'ll DIType>> = enumerators
.map(|(name, value)| unsafe {
let value = [value as u64, (value >> 64) as u64];
Some(llvm::LLVMRustDIBuilderCreateEnumerator(
DIB(cx),
name.as_ptr().cast(),
name.len(),
value as i64,
value.as_ptr(),
size.bits() as libc::c_uint,
is_unsigned,
))
})
.collect();

let (size, align) = cx.size_and_align_of(base_type);

unsafe {
llvm::LLVMRustDIBuilderCreateEnumerationType(
DIB(cx),
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_llvm/src/llvm/ffi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2118,7 +2118,8 @@ extern "C" {
Builder: &DIBuilder<'a>,
Name: *const c_char,
NameLen: size_t,
Value: i64,
Value: *const u64,
SizeInBits: c_uint,
Copy link
Member

@nagisa nagisa Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to prefer c_uint over u64 like in the function above?

Copy link
Contributor Author

@beetrees beetrees Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsigned int is the type of the SizeInBits parameter of the APInt constructor on the C++ side; using c_uint here means any integer truncation is visible on the Rust side rather than buried in the C++ wrapper (this shouldn't matter in practice as the maximum value of SizeInBits here is 128). I'm happy to change it to u64 if you think that would be an improvement, but using c_uint here seems like better design.

IsUnsigned: bool,
) -> &'a DIEnumerator;

Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,8 +998,9 @@ extern "C" LLVMValueRef LLVMRustDIBuilderInsertDeclareAtEnd(

extern "C" LLVMMetadataRef LLVMRustDIBuilderCreateEnumerator(
LLVMRustDIBuilderRef Builder, const char *Name, size_t NameLen,
int64_t Value, bool IsUnsigned) {
return wrap(Builder->createEnumerator(StringRef(Name, NameLen), Value, IsUnsigned));
const uint64_t Value[2], unsigned SizeInBits, bool IsUnsigned) {
return wrap(Builder->createEnumerator(StringRef(Name, NameLen),
APSInt(APInt(SizeInBits, makeArrayRef(Value, 2)), IsUnsigned)));
}

extern "C" LLVMMetadataRef LLVMRustDIBuilderCreateEnumerationType(
Expand Down
16 changes: 16 additions & 0 deletions src/test/run-make/repr128-dwarf/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# ignore-windows
# This test should be replaced with one in src/test/debuginfo once GDB or LLDB support 128-bit
# enums.

include ../../run-make-fulldeps/tools.mk

all:
$(RUSTC) -Cdebuginfo=2 lib.rs -o $(TMPDIR)/repr128.rlib
"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n U128A $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value (<0x10> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 )"
"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n U128B $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value (<0x10> 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 )"
"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n U128C $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value (<0x10> 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 )"
"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n U128D $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value (<0x10> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff )"
"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n I128A $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value (<0x10> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 )"
"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n I128B $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value (<0x10> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff )"
"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n I128C $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value (<0x10> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80 )"
"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n I128D $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value (<0x10> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 7f )"
Comment on lines +8 to +16
Copy link
Member

@nagisa nagisa Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than this (or in addition to this) lets add a debuginfo test instead. It doesn't matter particularly much what dwarfdump outputs if it turns out that the primary consumers of this data (gdb/lldb/other debuggers) can’t really stomach this kind of data anyway. E.g. if those “just” crash, then perhaps the ecosystem just isn’t ready for this and we probably shouldn't be stabilizing this feature either way.

Copy link
Contributor Author

@beetrees beetrees Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gdb and lldb don't currently crash, but neither give any useful output that could be tested for (gdb gives a nice error message instead of the current value of the enum-typed variable, lldb prints an internal assertion error and doesn't correctly process the enum-typed variable in question but otherwise keeps going). This problem with debuggers can already be exposed using stable rustc with a variable of type Option<NonZeroU128>, which has a 128-bit discriminant (gdb gives the same error message, lldb doesn't support enums with fields at all).

Since gdb and lldb don't currently support 128-bit enums, this test seems like the best option to make sure the DWARF output doesn't break. I agree that once gdb or lldb gain support for 128-bit enums, this test should be replaced with a regular debuginfo test.

23 changes: 23 additions & 0 deletions src/test/run-make/repr128-dwarf/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#![crate_type = "lib"]
#![feature(repr128)]

// Use .to_le() to ensure that the bytes are in the same order on both little- and big-endian
// platforms.

#[repr(u128)]
pub enum U128Enum {
U128A = 0_u128.to_le(),
U128B = 1_u128.to_le(),
U128C = (u64::MAX as u128 + 1).to_le(),
U128D = u128::MAX.to_le(),
}

#[repr(i128)]
pub enum I128Enum {
I128A = 0_i128.to_le(),
I128B = (-1_i128).to_le(),
I128C = i128::MIN.to_le(),
I128D = i128::MAX.to_le(),
}

pub fn f(_: U128Enum, _: I128Enum) {}