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.

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

debuginfo: Composite type reform and enum support. #7710

Closed
wants to merge 26 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@michaelwoerister
Contributor

michaelwoerister commented Jul 11, 2013

This pull request includes various improvements:

  • Composite types (structs, tuples, boxes, etc) are now handled more cleanly by debuginfo generation. Most notably, field offsets are now extracted directly from LLVM types, as opposed to trying to reconstruct them. This leads to more stable handling of edge cases (e.g. packed structs or structs implementing drop).
  • debuginfo.rs in general has seen a major cleanup. This includes better formatting, more readable variable and function names, removal of dead code, and better factoring of functionality.
  • Handling of VariantInfo in ty.rs has been improved. That is, the type VariantInfo = @VariantInfo_ typedef has been replaced with explicit uses of @VariantInfo, and the duplicated logic for creating VariantInfo instances in ty::enum_variants() and typeck::check::mod::check_enum_variants() has been unified into a single constructor function. Both function now look nicer too :)
  • Debug info generation for enum types is now mostly supported. This includes:
    • Good support for C-style enums. Both DWARF and gdb know how to handle them.
    • Proper description of tuple- and struct-style enum variants as unions of structs.
    • Proper handling of univariant enums without discriminator field.
    • Unfortunately gdb always prints all possible interpretations of a union, so debug output of enums is verbose and unintuitive. Neither LLVM nor gdb support DWARF's DW_TAG_variant which allows to properly describe tagged unions. Adding support for this to LLVM seems doable. gdb however is another story. In the future we might be able to use gdb's Python scripting support to alleviate this problem. In agreement with @jdm this is not a high priority for now.
  • The debuginfo test suite has been extended with 14 test files including tests for packed structs (with Drop), boxed structs, boxed vecs, vec slices, c-style enums (standalone and embedded), empty enums, tuple- and struct-style enums, and various pointer types to the above.

What is not yet included is DI support for some enum edge-cases represented as described in trans::adt::NullablePointer.

Cheers,
Michael

PS: closes #7819, fixes #7712

@jdm

This comment has been minimized.

Contributor

jdm commented Jul 11, 2013

\o/

I'll work my way through this today, hopefully.

@jdm

This comment has been minimized.

Contributor

jdm commented Jul 12, 2013

One general naming comment: it seems like some functions now use a get_or_create prefix, while others just use create. Is this reflecting the caching nature of their implementation? That doesn't seem important to distinguish to me, and I think names like block_metadata and file_metadata would be descriptive of their purpose.

@jdm

This comment has been minimized.

Contributor

jdm commented Jul 12, 2013

Style nits: I'd like to move away from multi-line unsafe blocks opening on the same line as existing code.

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Jul 13, 2013

Style nits: I'd like to move away from multi-line unsafe blocks opening on the same line as existing code.

You mean something like this:

do member_name.as_c_str |member_name| { unsafe {
    llvm::LLVMDIBuilderCreateMemberType(
        DIB(cx),
        file_metadata,
        ...,
        0,
        member_type_metadata[i])
}}

would turn into the following?

do member_name.as_c_str |member_name| { 
    unsafe {
        llvm::LLVMDIBuilderCreateMemberType(
            DIB(cx),
            file_metadata,
            ...,
            0,
            member_type_metadata[i])
    }
}
@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Jul 13, 2013

...and I think names like block_metadata and file_metadata would be descriptive of their purpose.

Yeah, I definitely went for verbosity in the naming style. This was certainly influenced by my recent experiences of having to try to understand this code without much prior knowledge or documentation. My thinking behind this was that one should be able to get some insight into what's going on by just reading part of the code, because often a newcomer (or someone doing a quick fix in some code otherwise not known to them) will not be aware of the internal workings of these functions (such as that LLVM will merge duplicate metadata anyway and it is thus no semantic difference whether caching happens or not).

If you find this exceedingly ugly, we can change it of course. But otherwise I would still vote for some redundancy in the naming if it improves readability (even if it impedes writeability a bit).

That being said, I'm not completely satisfied with the naming in the module either. Maybe your approach with just file_metadata (with neither create_ nor get_or_create_ prefix) plus some good documentation in the form of comments would be the best solution.

@jdm

This comment has been minimized.

Contributor

jdm commented Jul 15, 2013

  •    return create_composite_type_metadata(cx, Type::nil(), enum_name, &[], &[], &[], span); 
    

Are the &s necessary here?

@jdm

This comment has been minimized.

Contributor

jdm commented Jul 15, 2013

With regards to the stage0 thing, I'm inclined to just hold off merging this until we get a new snapshot that makes it irrelevant.

@jdm

This comment has been minimized.

Contributor

jdm commented Jul 15, 2013

There are various instances of code like let variant_name : &str = cx.sess.str_of(variant_info.name);. The rustc style is to put the : immediately after the name.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
// For empty enums there is an early exit. Just describe it as an empty struct with the
// appropriate type name
if ty::type_is_empty(cx.tcx, enum_type) {
return create_composite_type_metadata(cx, Type::nil(), enum_name, &[], &[], &[], span);

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Are the &s necessary here?

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
adt::CEnum(*) => {
return discriminant_type_metadata;
}
adt::Univariant(ref struct_def, _destroyed_flag) => {

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

This can just be _

This comment has been minimized.

@michaelwoerister

michaelwoerister Jul 16, 2013

Contributor

I'll remove it. However, in these cases I'm never sure which version I like better. It often helps readability to know what is ignored here.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
let enum_llvm_type = type_of::type_of(cx, enum_type);
let (enum_type_size, enum_type_align) = size_and_align_of(cx, enum_llvm_type);
return do enum_name.as_c_str |enum_name| { unsafe { llvm::LLVMDIBuilderCreateUnionType(

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Next line, please.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
None => do variant_info.args.map |_| { ~"" }
};
if (discriminant_type_metadata.is_some()) {

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Unnecessary outer parentheses.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
return create_composite_type_metadata(
cx,
box_llvm_type,
"box name",

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

BoxedType, perhaps?

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
span);
// Unfortunately, we cannot assert anything but the correct types here---and not whether the
// 'next' and 'prev' pointers are in the order.

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

"are in the correct order."

@jdm

View changes

src/librustc/lib/llvm.rs Outdated
AlignInBits: c_ulonglong,
Flags: c_uint ,
Elements: ValueRef,
RunTimeLang : c_uint) -> ValueRef;

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

There are many places in this PR that have a space before the : in type signatures; rustc style is to have the : immediately follow the name.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
// Unfortunately, we cannot assert anything but the correct types here---and not whether the
// 'next' and 'prev' pointers are in the order.
fn box_layout_is_as_expected(cx: &CrateContext,

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

s/as_expected/correct/

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
let element_llvm_type = type_of::type_of(cx, element_type);
let (element_type_size, element_type_align) = size_and_align_of(cx, element_llvm_type);
let subrange = unsafe { llvm::LLVMDIBuilderGetOrCreateSubrange(

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Function call on next line.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
let mdval = box_scx.finalize();
return mdval;
let subscripts = create_DIArray(DIB(cx), [subrange]);
return unsafe { llvm::LLVMDIBuilderCreateArrayType(

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Function call on next line. I'll stop pointing these out now; please do a search for "{ llvm::" and fix any that show up.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
let (element_size, element_align) = size_and_align_of(cx, element_llvm_type);
let vec_llvm_type = Type::vec(cx.sess.targ_cfg.arch, &element_llvm_type);
let vec_type_name = &"vec";

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

This doesn't seem like a useful temporary. Just pass the string as an argument.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
scx.add_member("vec", 0, ptr_size, ptr_align, elem_ptr);
scx.add_member("length", 0, sys::size_of::<uint>(), sys::min_align_of::<uint>(), uint_type);
return scx.finalize();
// fill alloc elements

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Make these line up with the line below?

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
let member_type_metadata = &[
get_or_create_type_metadata(cx, data_ptr_type, span),
get_or_create_type_metadata(cx, ty::mk_uint(), span)
];

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Unindent.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
assert!(slice_layout_is_as_expected(cx, member_llvm_types, element_type));
let data_ptr_type = ty::mk_ptr(cx.tcx, ty::mt { ty: element_type, mutbl: ast::m_const });

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Make this m_imm instead, m_const should be going away.

@jdm

View changes

src/librustc/middle/trans/debuginfo.rs Outdated
member_type_metadata,
span);
fn slice_layout_is_as_expected(cx: &mut CrateContext,

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

slice_layout_is_correct

@jdm

View changes

src/librustc/middle/ty.rs Outdated
match variant.node.disr_expr {
Some(e) => match const_eval::eval_const_expr_partial(cx, e) {
Ok(const_eval::const_int(val)) => { discriminant = val as int; }

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Ok(const_eval::const_int(val)) => discriminant = val as int,

@jdm

View changes

src/librustc/middle/ty.rs Outdated
named_field(ident, _visibility) => ident,
unnamed_field => cx.sess.bug(
"enum_variants: all fields in struct must have a name")
}};

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Split these up, please.

@jdm

View changes

src/librustc/middle/ty.rs Outdated
let arg_tys = ty_fn_args(ctor_ty).map(|a| *a);
let arg_names = do fields.map |field| {
match field.node.kind {
named_field(ident, _visibility) => ident,

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

_ instead of _visibility

@jdm

View changes

src/librustc/middle/ty.rs Outdated
do enum_variants(cx, id).iter().transform |variant_info| {
let substd_args = variant_info.args.iter()
.transform(|aty| subst(cx, substs, *aty)).collect();
let substd_ctor_ty = subst(cx, substs, variant_info.ctor_ty);
@VariantInfo_{args: substd_args, ctor_ty: substd_ctor_ty,
@VariantInfo{args: substd_args, ctor_ty: substd_ctor_ty,

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Add a space here before the {.

@jdm

View changes

src/librustc/middle/ty.rs Outdated
match variant.node.disr_expr {
Some(e) => match const_eval::eval_const_expr_partial(cx, e) {
Ok(const_eval::const_int(val)) => { discriminant = val as int; }
_ => {}

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Previously we would ICE here, but now we'll silently use the previous discriminant.

This comment has been minimized.

@michaelwoerister

michaelwoerister Jul 16, 2013

Contributor

Yeah, this is kind of a problematic case. I too had it triggering an ICE here but that would break a compile-fail test case because the error message was duplicated (once for this method and another time in typeck::check::mod::check_enum_variants()).
But this was before I moved discriminant calculation out of VariantInfo::from_ast_variant()---I'll try to put the ICE back in and see if it passes the test.

The problem here is that the order between this method and type checking is not fixed. Most of the time the enum will be type checked before this method is called for it but in some rare cases (see #6362 and #7527) this method is called before the type check. Which in turn produces an ICE instead of an compilation error. Maybe this should be cleaned up anyway (by always running the type checking before).

This comment has been minimized.

@jdm

jdm Jul 16, 2013

Contributor

You could make it a span_err instead, perhaps? That would allow compile-fail tests to do their magic.

@jdm

View changes

src/librustc/middle/typeck/check/mod.rs Outdated
// handle, so we may still get an internal compiler error
match const_eval::eval_const_expr_partial(ccx.tcx, e) {
Ok(const_eval::const_int(val)) => { current_disr_val = val as int; }

This comment has been minimized.

@jdm

jdm Jul 15, 2013

Contributor

Ok(const_eval::const_int(val)) => current_disr_val = val as int,

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Jul 19, 2013

Rebased :)

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Jul 19, 2013

Sorry about that. Compilation seems to break on mac because some new C functions from RustWrapper.cpp cannot be found by the linker. I added their names to rustllvm.def.in in michaelwoerister@b52eb4a. I hope this fixes the issue. Please re-approve.

@jdm

This comment has been minimized.

jdm commented on b52eb4a Jul 19, 2013

r+

@bors

This comment has been minimized.

Contributor

bors commented on b52eb4a Jul 20, 2013

saw approval from jdm
at michaelwoerister@b52eb4a

This comment has been minimized.

Contributor

bors replied Jul 20, 2013

merging michaelwoerister/rust/WP4 = b52eb4a into auto

This comment has been minimized.

Contributor

bors replied Jul 20, 2013

michaelwoerister/rust/WP4 = b52eb4a merged ok, testing candidate = dfd6c6cc

debuginfo: Removed some test relying on data structure sizes hard to …
…predict for all possible platforms and configurations.
@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Jul 20, 2013

Commit michaelwoerister@a1303cc should make the test cases pass on 32 bit machines. The tests removed were no particularly good idea in the first place.

@jdm

This comment has been minimized.

jdm commented on a1303cc Jul 20, 2013

r+

@jdm

This comment has been minimized.

Contributor

jdm commented Jul 20, 2013

That makes sense.

@bors

This comment has been minimized.

Contributor

bors commented on a1303cc Jul 20, 2013

saw approval from jdm
at michaelwoerister@a1303cc

This comment has been minimized.

Contributor

bors replied Jul 20, 2013

merging michaelwoerister/rust/WP4 = a1303cc into auto

This comment has been minimized.

Contributor

bors replied Jul 20, 2013

michaelwoerister/rust/WP4 = a1303cc merged ok, testing candidate = 8aae6ed

This comment has been minimized.

Contributor

bors replied Jul 20, 2013

fast-forwarding master to auto = 8aae6ed

bors added a commit that referenced this pull request Jul 20, 2013

auto merge of #7710 : michaelwoerister/rust/WP4, r=jdm
This pull request includes various improvements:

+ Composite types (structs, tuples, boxes, etc) are now handled more cleanly by debuginfo generation. Most notably, field offsets are now extracted directly from LLVM types, as opposed to trying to reconstruct them. This leads to more stable handling of edge cases (e.g. packed structs or structs implementing drop).

+ `debuginfo.rs` in general has seen a major cleanup. This includes better formatting, more readable variable and function names, removal of dead code, and better factoring of functionality.

+ Handling of `VariantInfo` in `ty.rs` has been improved. That is, the `type VariantInfo = @VariantInfo_` typedef has been replaced with explicit uses of @VariantInfo, and the duplicated logic for creating VariantInfo instances in `ty::enum_variants()` and `typeck::check::mod::check_enum_variants()` has been unified into a single constructor function. Both function now look nicer too :)

+ Debug info generation for enum types is now mostly supported. This includes:
  + Good support for C-style enums. Both DWARF and `gdb` know how to handle them.
  + Proper description of tuple- and struct-style enum variants as unions of structs.
  + Proper handling of univariant enums without discriminator field.
  + Unfortunately `gdb` always prints all possible interpretations of a union, so debug output of enums is verbose and unintuitive. Neither `LLVM` nor `gdb` support DWARF's `DW_TAG_variant` which allows to properly describe tagged unions. Adding support for this to `LLVM` seems doable. `gdb` however is another story. In the future we might be able to use `gdb`'s Python scripting support to alleviate this problem. In agreement with @jdm this is not a high priority for now.

+ The debuginfo test suite has been extended with 14 test files including tests for packed structs (with Drop), boxed structs, boxed vecs, vec slices, c-style enums (standalone and embedded), empty enums, tuple- and struct-style enums, and various pointer types to the above.

~~What is not yet included is DI support for some enum edge-cases represented as described in `trans::adt::NullablePointer`.~~

Cheers,
Michael

PS: closes #7819,  fixes #7712

@bors bors closed this Jul 20, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment