From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 70998 invoked by alias); 16 Nov 2017 16:35:05 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 70676 invoked by uid 89); 16 Nov 2017 16:35:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-15.7 required=5.0 tests=BAYES_00,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KB_WAM_FROM_NAME_SINGLEWORD,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=significantly X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 16 Nov 2017 16:35:02 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A30C2B649; Thu, 16 Nov 2017 16:35:01 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id C9EF96061B; Thu, 16 Nov 2017 16:35:00 +0000 (UTC) Subject: Re: [RFA] Handle dereferencing Rust trait objects To: Tom Tromey , gdb-patches@sourceware.org References: <20171115212403.8639-1-tom@tromey.com> From: Pedro Alves Message-ID: Date: Thu, 16 Nov 2017 16:35:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20171115212403.8639-1-tom@tromey.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-11/txt/msg00313.txt.bz2 On 11/15/2017 09:24 PM, Tom Tromey wrote: > In Rust, virtual tables work a bit differently than they do in C++. In > C++, as you know, they are connected to a particular class hierarchy. > Rust, instead, can generate a virtual table for potentially any type -- > in fact, one such virtual table for each trait (a trait is similar to an > abstract class or to a Java interface) that a type implements. > > Objects that are referenced via a trait can't currently be inspected by > gdb. This patch implements the Rust equivalent of "set print object". > > gdb relies heavily on the C++ ABI to decode virtual tables; primarily to > make "set print object" work; but also "info vtbl". However, Rust does > not currently have a specified ABI, so this approach seems unwise to > emulate. > > Instead, I've changed the Rust compiler to emit some DWARF that > describes trait objects (previously their internal structure was > opaque), vtables (currently just a size -- but I hope to expand this in > the future), and the concrete type for which a vtable was emitted. > > The concrete type is expressed as a DW_AT_containing_type on the > vtable's type. This is a small extension to DWARF. > > This patch adds a new entry to quick_symbol_functions to return the > symtab that holds a data address. Previously there was no way in gdb to > look up a full (only minimal) non-text symbol by address. The psymbol > implementation of this method works by lazily filling in a map that is > added to the objfile. This avoids slowing down psymbol reading for a > feature that is likely to not be used too frequently. > > I did not update .gdb_index. My thinking here is that the DWARF 5 > indices will obsolete .gdb_index soon-ish, meaning that adding a new > feature to them is probably wasted work. If necessary I can update the > DWARF 5 index code when it lands in gdb. > > Regression tested on x86-64 Fedora 25. > > 2017-11-15 Tom Tromey > > * symtab.h (struct symbol) : New member. > (struct rust_vtable_symbol): New. > (find_symbol_at_address): Declare. > * symtab.c (find_symbol_at_address): New function. > * symfile.h (struct quick_symbol_functions) > : New member. > * symfile-debug.c (debug_qf_find_compunit_symtab_by_address): New > function. > (debug_sym_quick_functions): Link to > debug_qf_find_compunit_symtab_by_address. > * rust-lang.c (rust_get_trait_object_pointer): New function. > (rust_evaluate_subexp) : New case. Call > rust_get_trait_object_pointer. > * psymtab.c (psym_relocate): Clear psymbol_map. > (psym_fill_psymbol_map, psym_find_compunit_symtab_by_address): New > functions. > (psym_functions): Link to psym_find_compunit_symtab_by_address. > * objfiles.h (struct objfile) : New member. > * dwarf2read.c (dwarf2_gdb_index_functions): Update. > (process_die) : New case. Call read_variable. > (rust_containing_type, read_variable): New functions. > > 2017-11-15 Tom Tromey > > * gdb.rust/traits.rs: New file. > * gdb.rust/traits.exp: New file. Looks good to me, with a couple comments below. > @@ -361,6 +362,11 @@ struct objfile > > struct psymbol_bcache *psymbol_cache; > > + /* Map symbol addresses to the partial symtab that defines the > + object at that address. */ > + > + std::unordered_map psymbol_map; > + std::unordered_map + pointers (or really small objects) makes me cry. :-) That has nasty memory overhead/footprint, with unordered_map being a node container... Can you say something about the number of elements that usually ends up in this map in some reasonably sized Rust-using app (Firefox?)? I'm assuming that you end up with many contiguous symbols pointing to the same psymtab? Would an addrmap (addrmap.h/c) be a good fit here? Maybe not if we don't have the size of the psymbols handy when we build this... :-/ > /* Vectors of all partial symbols read in from file. The actual data > is stored in the objfile_obstack. */ > + CORE_ADDR vtable = value_as_address (value_field (value, vtable_field)); > + struct symbol *symbol = find_symbol_at_address (vtable); > + if (symbol == NULL || !symbol->is_rust_vtable) > + return NULL; > + > + struct rust_vtable_symbol *vtable_sym > + = reinterpret_cast (symbol); This reinterpret_cast looks like a big hammer here. Why not static_cast? > + struct type *pointer_type = lookup_pointer_type (vtable_sym->concrete_type); > + return value_cast (pointer_type, value_field (value, 1 - vtable_field)); > +} > --- a/gdb/symtab.h > +++ b/gdb/symtab.h > @@ -1061,6 +1061,11 @@ struct symbol > In this case the symbol is really a "struct template_symbol". */ > unsigned is_cplus_template_function : 1; > > + /* True if this is a Rust virtual table. In this case, the symbol > + can be downcast to "struct rust_vtable_symbol". */ > + > + unsigned is_rust_vtable : 1; A symbol can't be both is_cplus_template_function and is_rust_vtable, I think it'd be better long term if we merged the tag to a single enum bitfield (with two bits). But it's not that big a deal and can always be done later. So if we don't end up with too many symbols in the map that could increase gdb's memory significantly, this is fine with me. Fix the reinterpret_cast and you're good to go. Thanks, Pedro Alves