From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2994 invoked by alias); 11 Mar 2010 21:32:57 -0000 Received: (qmail 2981 invoked by uid 22791); 11 Mar 2010 21:32:56 -0000 X-SWARE-Spam-Status: No, hits=-7.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 11 Mar 2010 21:32:52 +0000 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o2BLWnVw012666 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 11 Mar 2010 16:32:49 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx04.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o2BLWnTL023943; Thu, 11 Mar 2010 16:32:49 -0500 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o2BLWmXp018804; Thu, 11 Mar 2010 16:32:48 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id B0F938884C2; Thu, 11 Mar 2010 14:32:47 -0700 (MST) From: Tom Tromey To: Chris Moller Cc: gdb-patches@sourceware.org Subject: Re: pr9065 patch References: <4B96B1C6.9050907@redhat.com> Reply-To: tromey@redhat.com Date: Thu, 11 Mar 2010 21:32:00 -0000 In-Reply-To: <4B96B1C6.9050907@redhat.com> (Chris Moller's message of "Tue, 09 Mar 2010 15:38:30 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 X-SW-Source: 2010-03/txt/msg00420.txt.bz2 >>>>> "Chris" == Chris Moller writes: Chris> This is the typeid thing. Chris> This patch implements typeid in gdb, letting users do things like Chris> There are some differences, though, between the gdb typeid and the c++ Chris> typeid, the biggest is that the gdb version only partially implements Chris> the type_info class: it doesn't actually implement the methods, it Chris> just fakes them, and even though you can assign things like "set tp = Chris> typeid().name()" you can't assign "set ti = Chris> typeid()" where "type_info *ti;" I think our goal ought to be no differences from the C++ compiler. It isn't actually that useful, I think, to just print a typeid in gdb. We have other, better, mechanisms for inspecting types. Instead, the reason to implement typeid in the first place is to preserve the "cut and paste" property: if the user cuts an expression from their source and pastes it into gdb, it should yield the same answer as their compiled code would. Deviations from this principle are confusing to users. Chris> Another difference is that the real typeid returns mangled names, Chris> leaving it to the user to demangle. This strikes me as a pain, so I Chris> just provide the demangled names. It should be compatible. GDB has a demangle command for this purpose. Chris> +exp : TYPEID '(' type ')' '.' name opt_args %prec UNARY Chris> + { write_exp_elt_opcode (OP_STRING); Chris> + write_exp_string ($6); Chris> + write_exp_elt_opcode (OP_STRING); I guess the problem is that type_id::name is an inlined function with no out-of-line instance in libstdc++. So, there is no way to call it. I think we should have a way to make a fake method, but I don't think it should be implemented in an ad hoc way. Instead it should be a general facility. That should be a separate patch. For the plain "introduce typeid" patch, I think it is fine if it just barfs for anything not provided by libstc++. This is what Daniel was referring to when he mentioned writing "not-outlined" methods in Python. And, I agree that this would be the way to go; for one thing it would let library implementers provide the implementations, alongside their pretty-printers and whatnot. Chris> +static struct value * Chris> +create_type_info_class(char *type_name) New functions need an introductory comment. There is a missing space before the "("; this occurs in many places in the patch. It is best to be const-correct. I assume the argument can be a const char *. Chris> + val = allocate_value (typeid_type); Chris> + Chris> + inferior_val = value_allocate_space_in_inferior (1 + strlen (type_name)); Chris> + inferior_addr = value_as_address (inferior_val); Chris> + write_memory (inferior_addr, Chris> + type_name, strlen (type_name)); Chris> + Chris> + memcpy (value_contents_raw (val) + bitpos/8, Chris> + &inferior_addr, sizeof(inferior_addr)); Chris> + rv = value_coerce_to_target (val); The call to write_memory is not copying the trailing \0. This code seems odd to me. IIUC, the type_info for a type is stored as a global symbol. This way, typeid always returns the same object for a given type, and thus the result can be compared with ==. (This is true on some platforms anyhow, there are tweaks for other cases, see libsupc++.) Making our own type_info is going to yield incorrect results where there is a single type_info object per type. Chris> +static struct value * Chris> +fake_type_info_class(char *type_name, char *field_name, struct expression *exp, Chris> + int is_pointer, int is_function, int field_has_args) This should probably just go away. As above, I don't think this is the way we want to handle the missing methods. Chris> + We're going to make the assumption that class typeinfo Chris> + isn't going to change. Given that, we need to handle: This is a reasonable assumption for the current ABI, but it is not reasonable to assume it universally. If this sort of thing is still needed, you have to hide it behind the appropriate API, see cp-abi.h. Chris> +static struct value * Chris> +evaluate_subexp_for_typeid_type (struct expression *exp, int *pos) Chris> + __attribute__ ((noinline)); Just remove this. For future reference, you can't use __attribute__ unconditionally in (most of) gdb, you have to hide it behind a define. See defs.h. Chris> + if (TYPE_CODE_STRUCT == TYPE_CODE (type)) Chris> + { Chris> +#define STRUCT_LBL "struct " Chris> + char * tmp = alloca (1 + strlen (STRUCT_LBL) + strlen (type_name)); Chris> + strcpy (tmp, STRUCT_LBL); I think it is nice to #undef such local defines after their use. That way, it is almost like they have proper scoping. Tom