From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13866 invoked by alias); 2 Oct 2019 02:47:22 -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 13858 invoked by uid 89); 2 Oct 2019 02:47:22 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=H*r:10.0.0, HTo:D*oracle.com, management, HContent-Transfer-Encoding:8bit X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 02 Oct 2019 02:47:21 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 6651B1E093; Tue, 1 Oct 2019 22:47:19 -0400 (EDT) Subject: Re: [PATCH v2] gdb: CTF support To: Weimin Pan , gdb-patches@sourceware.org References: <1564530195-27659-1-git-send-email-weimin.pan@oracle.com> <5377c457-52b0-583d-15b5-47024eae1f48@simark.ca> <895f47d4-3e01-4d5a-474b-43dd2dd037b4@oracle.com> From: Simon Marchi Message-ID: <0fe82814-46b8-79c2-6a25-5f5d51b158e1@simark.ca> Date: Wed, 02 Oct 2019 02:47:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: <895f47d4-3e01-4d5a-474b-43dd2dd037b4@oracle.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2019-10/txt/msg00055.txt.bz2 On 2019-09-30 8:36 p.m., Weimin Pan wrote: > Thank you very much for the thorough review. I have read through the > modified patch which looks good and will be adopted. In addition, I > have made more changes to address the issues you raised. Please see > below and the updated patch (attached). Great thanks! I noticed at least one case of leading spaces that should be tab in the attached patch, so maybe do: grep '^ ' ctfread.c to catch them. > Yes, I've made the change in several places where the allocated copy is > freed after > it's being dup'ed via obstack_strdup. Ok thanks. Actually, can you please use gdb::unique_xmalloc_pointer to manage that memory? We're trying to minimize the manual memory management in GDB. You'd do: gdb::unique_xmalloc_ptr name (ctf_type_aname_raw (fp, tid)); and use `name.get ()` instead of name. It will get free'd automatically when exiting scope. > Actually callers of these read_foo functions will call the appropriate > function, based > on the tid's kind. For example, either ctf_add_type_cb or > process_structure_type > calls read_structure_type only if tid's kind is CTF_K_STRUCT or > CTF_K_STRUCT. Ok, I was hoping it would make the code more robust in case someone does some refactor in the future and one of these functions get called with an object of the wrong type, but maybe it would be overkill. > > Hmm that looks fishy.  It looks like this is taking the address of a > > local variable for something that will be used after the current function > > will have returned. > > Sorry, but it's a "static" local variable. Oh ok, I did miss that. I think we should avoid this. What happens, for example, in this case: 1. Read psymtab for objfile 1 2. Read psymtab for objfile 2 3. Expand psymtab to symtab for objfile 1 >From what I understand, step #2 will overwrite the static variable, such that it won't be valid anymore in step #3. I think it would be better to allocate the context structure dynamically and store it as the objfile, like dwarf2read does. Thanks, Simon