From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from alt-proxy28.mail.unifiedlayer.com (alt-proxy28.mail.unifiedlayer.com [74.220.216.123]) by sourceware.org (Postfix) with ESMTPS id E3C7E3858C54 for ; Thu, 11 May 2023 13:41:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E3C7E3858C54 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tromey.com Received: from cmgw10.mail.unifiedlayer.com (unknown [10.0.90.125]) by progateway1.mail.pro1.eigbox.com (Postfix) with ESMTP id 23FD01003BDE2 for ; Thu, 11 May 2023 13:41:07 +0000 (UTC) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTP id x6Xbp8yc4r3KWx6XbpmSsJ; Thu, 11 May 2023 13:41:07 +0000 X-Authority-Reason: nr=8 X-Authority-Analysis: v=2.4 cv=IeVC5Uma c=1 sm=1 tr=0 ts=645cf073 a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=dLZJa+xiwSxG16/P+YVxDGlgEgI=:19 a=P0xRbXHiH_UA:10:nop_rcvd_month_year a=Qbun_eYptAEA:10:endurance_base64_authed_username_1 a=LWDUg-46AAAA:8 a=5n9g_TVbE-LuAL6GnVcA:9 a=-LZYelZiTLCs7MrZPmXS:22 a=0m5oFGktkVSl59jdpf0v:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date:References :Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=4wMOXopl7ZJEz8BKSQ2A65dIN5sProVXrXGKFGYG1Nw=; b=U8ymno3CaNOb1TAORKNdL4+5KH teUMDq1p4gR9aUk9sNccUS2KkU5A/C2ar/x+apb8D4aN1V2M7viIHuMxK2+ERCWIw7FbWkBYzRbhZ VCmCgwoRkWKaX+GqE4Tnfdb0V; Received: from 75-166-157-55.hlrn.qwest.net ([75.166.157.55]:52628 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1px6Xa-002yEq-PR; Thu, 11 May 2023 07:41:06 -0600 From: Tom Tromey To: Mark Harmstone Cc: binutils@sourceware.org Subject: Re: [PATCH 2/2] gdb: Allow reading of enum definitions in PDB files References: <20230509003247.24156-1-mark@harmstone.com> <20230509003247.24156-2-mark@harmstone.com> X-Attribution: Tom Date: Thu, 11 May 2023 07:41:05 -0600 In-Reply-To: <20230509003247.24156-2-mark@harmstone.com> (Mark Harmstone's message of "Tue, 9 May 2023 01:32:47 +0100") Message-ID: <87y1lv3s0e.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 75.166.157.55 X-Source-L: No X-Exim-ID: 1px6Xa-002yEq-PR X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 75-166-157-55.hlrn.qwest.net (murgatroyd) [75.166.157.55]:52628 X-Source-Auth: tom+tromey.com X-Email-Count: 2 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-Spam-Status: No, score=-3019.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: >>>>> "Mark" == Mark Harmstone writes: Mark> Adds a new file pdbread.c, which parses LF_ENUM records in PDB files. Hi. Thank you for the patch. I'm happy to see this happening. Normally, gdb patches should be sent to the gdb-patches list. It's fine IMO to just CC one like this, in the middle of a series, to gdb-patches. I don't know anything about pdb, so I won't comment much on that. Mark> --- a/gdb/configure.tgt Mark> +++ b/gdb/configure.tgt Mark> @@ -335,7 +335,7 @@ i[34567]86-*-cygwin*) Mark> ;; Mark> i[34567]86-*-mingw32*) Mark> # Target: Intel 386 running win32 Mark> - gdb_target_obs="i386-windows-tdep.o windows-tdep.o" Mark> + gdb_target_obs="i386-windows-tdep.o windows-tdep.o pdbread.o" I don't think this should be handled here. Object and debuginfo readers can either be compiled in unconditionally, or by seeing whether any necessary support is present in BFD. For the latter, see how elfread.o is handled in gdb/configure.ac. Mark> +/* Return the size in bytes of a PDB builtin type. */ Mark> +static ULONGEST Mark> +pdb_builtin_type_length (uint32_t t) Mark> +{ Probably this can be removed in favor. It's only used here: + t->set_target_type (pdb_builtin_type (builtin_type (objfile), + bfd_getl32 (&en.underlying_type))); + t->set_length (pdb_builtin_type_length (bfd_getl32 (&en.underlying_type))); but this will do something weird if the builtin type and pdb_builtin_type_length disagree about the size. Instead you can do something like: struct type *targ_type = pdb_builtin_type (...); t->set_target_type (targ_type); t->set_length (targ_type->length ()); Mark> +/* Some integers, such as enum values, get stored as an extended value if Mark> + they're too large to fit into two bytes. The two bytes that would normally Mark> + be the value are instead a type indicator, and the actual value follows. */ Mark> +static bool Mark> +pdb_read_extended_value (uint16_t kind, char **ptr, uint16_t *length, Mark> + LONGEST *ret) gdb normally puts 'const' where it makes sense, so for instance 'const char **ptr'. This requires a change in the caller but I was going to request that anyway. Mark> + buf = (char *) xmalloc (length); Mark> + Mark> + if (bfd_bread (buf, length, tpi) != length) Mark> + goto end; gdb generally uses RAII now for cleanups. 'buf' could either be a unique_xmalloc_ptr, or a gdb::char_vector (or even byte_vector depending on if you want to use gdb_byte). This will ensure proper cleanup even in the face of future refactorings, and instead of 'goto end', the code can just 'return'. Mark> + /* Round to 4-byte boundary. */ Mark> + if ((ptr - buf + 2) % 4) Mark> + { Mark> + if (length < 4) Mark> + goto end; Mark> + Mark> + length -= 4 - ((ptr - buf + 2) % 4); Mark> + ptr += 4 - ((ptr - buf + 2) % 4); I thought gdb had some macro for this but I can't find it now. Mark> + case LF_INDEX: { Mark> + struct lf_index *ind; Mark> + uint32_t fl_type; Mark> + Mark> + if (length < sizeof (*ind)) Mark> + goto end; Mark> + Mark> + ind = (struct lf_index *) ptr; Not sure if this is really guaranteed to have correct alignment; if not it could just be memcpy'd out. Mark> + name_len = length - sizeof (en); Mark> + name = (char * ) xmalloc (name_len + 1); Either unique_xmalloc_ptr or std::string here. Mark> + t = type_allocator (objfile).new_type (); Mark> + t->set_code (TYPE_CODE_ENUM); Mark> + t->set_name (name); This will leak 'name'. In gdb, types are allocated on an obstack; in this case, the objfile obstack. When the objfile is destroyed, the obstack is also freed -- but this does not run any destructors or free any data associated with objects on the obstack. Instead, if a string must be preserved, it should be copied to the objfile obstack. If the string is likely to be duplicated in the objfile (if it might be needed multiple times), then objfile::intern can be used. Mark> + num_types = last_type - first_type + 1; Mark> + types = (struct pdb_type *) xmalloc (sizeof (*types) * num_types); Use std::vector here. Mark> + Mark> + for (unsigned int i = 0; i < num_types; i++) You can use C++ "foreach". Mark> + bfd_close (tpi); gdb has a complicated BFD reference-counting setup, but that might be overkill for this use. However, it's not difficult to make a deleter type that wraps bfd_close, so that explicit closes on error paths aren't needed. This would be more idiomatic in gdb. If you look for typedefs of unique_ptr, you'll find some of these. Mark> +static void Mark> +pdb_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags) Mark> +{ Mark> + buildsym_compunit builder(objfile, "", nullptr, language_c, 0); Assuming this is the first of several PDB-reading patches, it may be worth considering the overall approach. Other debug info readers in gdb work in two passes. First they make some kind of "quick" index. Then, when a symbol lookup finds a match, the full debug info for a given compilation unit is read in. The code here only does the second step. I don't know PDB, so maybe that's the only way it can be done? In gdb, the symbols are normally associated with a given compilation unit. This isn't done in this patch... I'm wondering if that's possible to do? Mark> +if {![istarget i*86-*-mingw*] Mark> + && ![istarget x86_64-*-mingw*]} { Mark> + return 0 Probably can use 'require {is_any_target ...}' Mark> +gdb_test "ptype enum enum1" "type = enum enum1 \{red, green, blue = -1, yellow = 32768, purple = 4294967296\}.*" "ptype enum enum1" Normally we'd split the lines so they fit in 80 columns. Tom